Skip to content

Add EventSource guid ctors for non-reflection creation #28290

Description

@benaadams

Background and motivation

Public EventSource constructors perform a lot of reflection to compute the name to register EventSource with, and compute SHA1 hash to compute the registration GUID. The proposed API enables generating the name and GUID via a source generator instead.

This source generator exists as internal for Corelib today. This proposal changes internal EventSource .ctors that take Guid and name to protected

API Proposal

class EventSource
{
    protected EventSource(Guid eventSourceGuid, string eventSourceName);
    protected EventSource(Guid eventSourceGuid, string eventSourceName, EventSourceSettings settings, string[] traits = null);
}

API Usage

The API is expected to be used by source generator. Example of generated source:

        internal MyEventSource() : base(new Guid(0x0866b2b8,0x5cef,0x5db9,0x26,0x12,0x0c,0x0f,0xfd,0x81,0x4a,0x44), "System.MyEventSource") { }

Alternative Designs

No response

Risks

No response


The non Guid .ctors cause a lot of reflection to look up the Guid and name from the applied attribute and for first use a lot of Jit compilation of the reflection methods:

image

Which if it is in the startup path directly impacts startup times.

For coreclr they can use the internal .ctors dotnet/coreclr#21714, dotnet/coreclr#16054, dotnet/coreclr#16060

However there are still many corefx, aspnet and other app model EventSources that get triggered on startup and cannot use these .ctor overloads (e.g. Microsoft-Diagnostics-DiagnosticSource, Microsoft-System-Net-Sockets, Microsoft-System-Net-NameResolution, Microsoft-AspNetCore-Hosting, Microsoft-Extensions-DependencyInjection) and directly impact startup time:

image

/cc @jkotas

Activity

  1. jkotas commented on Dec 30, 2018

    @jkotas
    Member
  2. jkotas commented on Dec 30, 2018

    @jkotas
    Member

    Related discussion: dotnet/coreclr#16054 (comment)

  3. benaadams commented on Dec 30, 2018

    @benaadams
    MemberAuthor

    re the discussion; protected makes it a little less visible than public and it isn't providing anything more problematic than what is already exposed via the ability to set the Guid via the EventSourceAttribute; other than that going via the slower reflection path.

  4. vancem commented on Jan 2, 2019

    @vancem

    The fundamental problem with this change is that it promotes something (forcing people to use GUIDS in EventSources), that we really don't want people to do (GUIDS are really supposed to be hidden and uninteresting).

    Now if there was a important reason, then maybe we do this bad thing, but we should do some due-dillegence first (lets see if we can fix the perf without doing violance to the code base).

    Fundamentally, the GUID is DERIVED from the STRING name given to it (or part of the attribute), via a GenerateGuidFromName, method. Thus dropping the GUID parameter should only cost you that method (all the other reflection should be avoidable). From your traces this is a small amount, and looks like it could be made smaller with some tweeks to that method. This seems like a better approach.

    Finally, we should be very careful with startup TIMES, as often it is the case that the FIRST time for APIs is significantly larger than all others. Thus you spend time fixing one place only to have it pop up somewhere else. Moreover, many of these methods are generic (use of Guids, use of Culture, use of Reflection), where it is unlikely that non-trivial programs don't also touch them (and thus incur the FIRST time penalty).

    Thus I would like to insure we have first pursued all the options that avoid new ugly APIs. I would first review the profiles to see if ALL eventsource construction take similar time (they should, unless there are startup effects), and get a handle on the non-one-time costs. Minimize those first (which may be optimizing GenerateGuidFromName). After that determine if the one-time costs are likely to be hit anyway and it was simply EventSource that was 'first' (I am suspicious about this with respect to the reflection costs). We can then look at what our options are about eliminating those costs (since like I said the only REAL work that should be done is GenerateGuidFromName).

  5. benaadams commented on Jan 2, 2019

    @benaadams
    MemberAuthor

    The workload to consider start-up time most for optimizing would be likely be Azure Functions/AWS Lambda as first response cold start is paid close attention to here, also Azure Web Apps without "Always On" switched on as then you pay the startup cost continuously (assuming low traffic site).

    Its less significant in other areas; though new workloads like Desktop startup it may be more significant, but I imagine there is lower hanging fruit in for those.

  6. vancem commented on Jan 3, 2019

    @vancem

    To be clear, I am not doubting that there are important startup scenarios, or even that EventSource startup is important to optimize and that changes woudl be good. Only that we should look first for solutions that don't force users to trade off simplicity for performance, and that we believe that we are not just 'pushing' the cost somewhere else.

  7. benaadams commented on Jan 9, 2019

    @benaadams
    MemberAuthor

    and get a handle on the non-one-time costs. Minimize those first

    Had a go dotnet/coreclr#21832, dotnet/coreclr#21720, dotnet/coreclr#21729, dotnet/coreclr#21765 also added the Guid to RuntimeEventSource dotnet/coreclr#21714 and cleaned up some of the startup costs from EventPipe dotnet/coreclr#21718 and Environment dotnet/coreclr#21715

    Waiting for the AspNetCore repo to pick up a runtime from this year to see if any of it makes a meaningful impact to startup-time/time to first response time (from cold):

    image

  8. vancem commented on Jan 10, 2019

    @vancem

    This is good stuff @benaadams thanks for pursuing this

  9. stephentoub commented on Mar 24, 2019

    @stephentoub
    Member

    Waiting for the AspNetCore repo to pick up a runtime from this year to see if any of it makes a meaningful impact to startup-time/time to first response time (from cold)

    Anything interesting here, @benaadams?

  10. benaadams commented on Mar 25, 2019

    @benaadams
    MemberAuthor

    Seems to be going the otherway

    image

    Unfortunately the flow of the runtime into aspnet is a bit slow, so is hard to bisect what changes the increases relate to as its a very large range.

  11. benaadams commented on Mar 26, 2019

    @benaadams
    MemberAuthor

    Looks like there are other issues for the increased startup dotnet/aspnetcore#8836

  12. 16 remaining items

  13. modified the milestones: Future, 11.0.0 on Oct 15, 2025
  14. jkotas commented on Oct 15, 2025

    @jkotas
    Member

    I have edited the API proposal to follow the template and marked this as ready for review.

  15. noahfalk commented on Oct 16, 2025

    @noahfalk
    Member

    I'm fine exposing the Guid constructor.

    I want to mention invoking the public API EventSource(string eventSourceName) is sufficient to avoid most of the cost here. The difference between EventSource(string) and EventSource(string, Guid) is that the Guid constructor allows skipping GenerateGuidFromName. In @benaadams trace above GenerateGuidFromName is ~10% of the cost.

  16. bartonjs commented on Oct 28, 2025

    @bartonjs
    Member

    We flipped the string and Guid parameters (so the base() call starts with the name, not the GUID); otherwise looks good as proposed.

    namespace System.Diagnostics.Tracing;
    
    public partial class EventSource
    {
        protected EventSource(string eventSourceName, Guid eventSourceGuid);
        protected EventSource(string eventSourceName, Guid eventSourceGuid, EventSourceSettings settings, string[] traits = null);
    }
  17. added
    api-approvedAPI was approved in API review, it can be implemented
    and removed
    api-ready-for-reviewAPI is ready for review, it is NOT ready for implementation
    on Oct 28, 2025
  18. EgorBo commented on Oct 28, 2025

    @EgorBo
    Member

    @jkotas should we go ahead and re-use corelib's source generator in other BCL libs now that the ctor is approved? I believe none of our EventSources use anything other than EventSourceSettings.EtwManifestEventFormat (to address the concern raised during the API review).

    and then think about how we can expose the source generator for public without extra/new attributes? (it might require another API review?)

  19. jkotas commented on Oct 28, 2025

    @jkotas
    Member

    Sounds good to me. Do you plan to work on it?

  20. EgorBo commented on Oct 28, 2025

    @EgorBo
    Member

    Sounds good to me. Do you plan to work on it?

    if you planned to work on this - feel free to grab it, otherwise I can take a look later this week 🙂

  21. jkotas commented on Oct 28, 2025

    @jkotas
    Member

    All yours

  22. added
    in-prThere is an active PR which will close this issue when it is merged
    on Nov 5, 2025
  23. locked and limited conversation to collaborators on Dec 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

api-approvedAPI was approved in API review, it can be implementedarea-System.Diagnostics.TracingenhancementProduct code improvement that does NOT require public API changes/additionsin-prThere is an active PR which will close this issue when it is merged

Type

No type

Projects

No projects

    Milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions