Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve startup time for runtime EventSources #45105

Closed
wants to merge 6 commits into from

Conversation

benaadams
Copy link
Member

@benaadams benaadams commented Nov 23, 2020

  • Pre-Initialize ProviderMetadata for runtime EventSources (ReadOnlySpan<byte>)
    • ArrayPoolEventSource
    • FrameworkEventSource
    • RuntimeEventSource
    • PortableThreadPoolEventSource
    • TplEventSource
  • Pre-Initialize Manifest for runtime EventSources (ReadOnlySpan<byte>)
    • ArrayPoolEventSource
    • FrameworkEventSource
    • RuntimeEventSource
    • PortableThreadPoolEventSource
    • (Not TplEventSource as it has translations)
  • Validate pre-Initialized ProviderMetadatas & Manifests in debug
  • Optimise retrieving/checking a single Attribute (GetCustomAttribute) for MemberInfo(e.g. Type) and MethodInfo
  • Don't create manifest for pre-generated, non-translatable EventSources; except in debug (to validate)
    • TplEventSource is marked as translatable so not included for Manifest

Contributes to #44598
Reduces impact of #45059

@Dotnet-GitSync-Bot
Copy link
Collaborator

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@jkotas
Copy link
Member

jkotas commented Nov 23, 2020

This should be done via source generator: #43390

@benaadams
Copy link
Member Author

This should be done via source generator

Agreed; but someone needs to go first with the source generators (even a simple one) to link them into the build infra for System.Private.CoreLib will be easier to do it from there...

@jkotas
Copy link
Member

jkotas commented Nov 23, 2020

I think this would a good candidate for first real source generator to use with dotnet/runtime.

@danmosemsft @samsp-msft We have been talking quite a bit about source generators. We need to start figuring out how to build and use them in dotnet/runtime. Could you please suggest who should be driving this? I believe that it falls close to libraries infrastructure area.

@benaadams
Copy link
Member Author

benaadams commented Nov 23, 2020

Have cherrypicked the "Optimise retrieving/checking a single Attribute (GetCustomAttribute)" commit and added it seperately #45121

@benaadams
Copy link
Member Author

It triggers the Utf8 encoding path during startup

image

@danmoseley
Copy link
Member

@danmosemsft @samsp-msft We have been talking quite a bit about source generators. We need to start figuring out how to build and use them in dotnet/runtime. Could you please suggest who should be driving this? I believe that it falls close to libraries infrastructure area.

@dotnet/runtime-infrastructure how we will develop and use source generators?

  1. We need to be able to build them in dotnet/runtime because some of them will need to share sources with code in this repo. Where should we put them? What would a sample project look like?
  2. We will also want to consume them in some cases -- is that simply a matter of adding a <ProjectReference .. OutputItemType="Analyzer"> in eng\Analyzers.props?

@ViktorHofer
Copy link
Member

ViktorHofer commented Dec 7, 2020

@dotnet/runtime-infrastructure how we will develop and use source generators?

I don't have a strong preference regarding the location but I would avoid specifying ProjectReference items anywhere globally (e.g. in Analyzers.props) as they slow down incremental builds noticeably. I would instead inline those as a ProjectReference into the projects that require them. The Analyzers.props contain only prebuilt analyzers that apply to all projects.

@jkotas
Copy link
Member

jkotas commented Dec 7, 2020

We will want to use many of these source generators in CoreLib. It creates interesting build ordering issues.

Would it make sense for the source generators to be its own partition?

@ericstj
Copy link
Member

ericstj commented Dec 7, 2020

We will also want to consume them in some cases

We also need to test them. @layomia has some experience with this. There's one take on it here: https://github.com/dotnet/runtimelab/tree/feature/JsonCodeGen/src/libraries/System.Text.Json

We will want to use many of these source generators in CoreLib. It creates interesting build ordering issues.

Today source-generators target netstandard2.0 and reference Roslyn packages. They should be able to do so without any cycles or unusual ordering but they will need to be referenced by consuming projects. I believe this should be fine for source-build as well cc @dseefeld.

We also need to think about how we ship them. I imagine we'll want to make some sort of transport package that can be consumed by either roslyn or the SDK in order to ship them with the SDK.

@danmoseley
Copy link
Member

We probably need an issue to track this source-generators-infra work.

@benaadams
Copy link
Member Author

EventSourceGenerator for .ctor and metadata (but not manifest) #45699

@ViktorHofer
Copy link
Member

// Auto-generated message

69e114c which was merged 12/7 removed the intermediate src/coreclr/src/ folder. This PR needs to be updated as it touches files in that directory which causes conflicts.

To update your commits you can use this bash script: https://gist.github.com/ViktorHofer/6d24f62abdcddb518b4966ead5ef3783. Feel free to use the comment section of the gist to improve the script for others.

@ericstj
Copy link
Member

ericstj commented Jan 8, 2021

We probably need an issue to track this source-generators-infra work.

What infrastructure work are you imagining? I don't think we need anything too special since source-generators are .NETStandard libraries. I suspect we need folks to establish some guidelines and patterns/conventions about them so that they are implemented consistently. We may need to figure some of that out as we go. There will likely be a different pivot for source generators that actually ship as part of the product vs generators that are just used internally to dotnet/runtime. The former needs tests for the generator itself.

Thoughts @jkotas @ViktorHofer @stephentoub?

@jkotas
Copy link
Member

jkotas commented Jan 8, 2021

We may need to figure some of that out as we go.

+1 . I think that the structure in #45699 is a good start.

@stephentoub
Copy link
Member

Yeah, those are the kinds of things I'd expect to figure out as we add them.

@ViktorHofer
Copy link
Member

+1 . I think that the structure in #45699 is a good start.

Agreed. Just scanned the PR. The generator's source location and how it is included to the build makes sense to me.

@ghost
Copy link

ghost commented Feb 20, 2021

Draft Pull Request was automatically closed for inactivity. It can be manually reopened in the next 30 days if the work resumes.

@ghost ghost locked as resolved and limited conversation to collaborators Mar 22, 2021
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants