Use EventSource guid ctor for ArrayPoolEventSource

[benaadams]

Generated the Guid 0866b2b8-5cef-5db9-2612-0c0ffd814a44 by adding a Console.WriteLine(Guid.ToString) to the ArrayPoolEventSource constructor 😄

Resolves https://github.com/dotnet/coreclr/issues/15954

[benaadams]

Post

Pre

[stephentoub]

cc: @vancem

[justinvp]

The name is missing the “e” at the end.

[benaadams]

fixed

[jkotas]

@vancem Is there a good reason why the lean EventSource constructor that takes Guid is not public so that everybody can use it? It is always unfortunate when we have a internal secret sauce that lets us do things efficiently, but that is not accessible to 3rd party code.

[jkotas]

cc @brianrob

[brianrob]

@jkotas, from my understanding of this, you'd end up having to specify the GUID twice - once in the custom attribute and once to the constructor. This just makes things a bit more error prone. I suspect that the attribute mechanism predated the constructor which appears to be present to support lean start-up with usage of FrameworkEventSource.

It is possible to get rid of the ~16% cost to generate the GUID from the "pre" profile above by specifying the GUID in the attribute, but that doesn't get you away from the reflection to lookup the attribute.

We can potentially do something here to make this better for EventSources that incur high costs for very lean scenarios such as this one, but we should think about a holistic way to do this (it could be to make this constructor public). We have another mechanism that gets used in some cases on ProjectN to avoid reflection, and so it would be good to pick one mechanism and make that work for both cases.

In terms of this change, I'm happy to take it as long as @vancem doesn't have any concerns. It looks like it fits into the pattern that this constructor was created to address.

[vancem]

The change is fine as it just makes ArrayPoolEventSource like FrameworkEventSource.

@vancem Is there a good reason why the lean EventSource constructor that takes Guid is not public so that everybody can use it? It is always unfortunate when we have a internal secret sauce that lets us do things efficiently, but that is not accessible to 3rd party code.

The answer is that ideally people using EventSource don't know about GUIDs at all. GUIDS are a ETW thing, not a EventSource thing, and are a source of confusion and complexity (as in this case).

Thus ideally we don't expose an EventSource constructor that has a GUID in it at all. The fact that there is a custom attribute that takes a GUID is an legacy issue. It is also a significant potential error, since users will not tend to create the 'right' (they one that WOULD Have been generated from the name) and thus mess up tools that use the name (which is pretty much all tools). It is not a great move (it makes the API trickier). It seems OK for the framework to do such dangerous things, but it is probably best that arbitrary uses don't.

So what are better solutions?

1. One option is to do basically just this PR and be done with it. The idea here is that really only the framework EventSources tend to be on critical startup paths, so fixing them is enough.

2. A second option is to make looking up custom attributes faster. That seems to be the main problem and is general goodness (EventSource can't be the only thing that will touch custom attributes on typical startup paths).

3. Finally we could add another option to the EventSourceSettings that basically says 'skip looking at custom attributes and generate the GUID yourself. This skips the custom attribute code and goes right to the code to generate it from the name. This is probably not a big part of the overhead (I have not looked at your profiles). At least this option is 'safe' (but it not clear who would use it).

None of these options make the API surface 'worse' by adding error prone GUID parameters.

MY recommendation is to explore the options in order. Doing just this PR is fine with me.