Trim NetEventSource when EventSource.IsSupported is false

[eerhardt]

Follow up to #38129. NetEventSource code was still left in even when EventSource.IsSupported is false, since all the usages of NetEventSource are keying off its own static property: NetEventSource.IsEnabled.

Remove NetEventSource.IsEnabled so the linker can trim NetEventSource code when EventSource.IsSupported is false.

cc @marek-safar

This trims a little over 8 KB of IL in the CarChecker Blazor app, when EventSource.IsSupported is false.

Build	Size
master	3,719,168 bytes
proposed change	3,710,976 bytes

[Dotnet-GitSync-Bot]

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.

[ghost]

Tagging subscribers to this area: @dotnet/ncl
Notify danmosemsft if you want to be subscribed.

[eerhardt]

Alternatively, we could just remove NetEventSource.IsEnabled altogether and replace it with NetEventSource.Log.IsEnabled(), which would achieve the same results.
I see that NetEventSource is in a few different assemblies. The alternative route would allow for trimming NetEventSource logic in all the assemblies without having to duplicate the ILLink.Substitutions.xml file for each assembly.

[alnikola]

We are currently migrating from NetEventSources to the new telemetry. It's quite possible that NetEventSources will be eventually completely deleted. Could you please make sure the trimming also works with the new telemetry types? Please, check what has been already done in System.Net.Http #37619 as well the main issue tracking all related work #37428.

[marek-safar]

As the code is located in a common location it'd be better to update the code to follow pattern which can be automatically removed

[alnikola]

@marek-safar Sorry, it seems I'm missing the point. Could you please clarify which location is a common one? All NetEventSource types are inside System.Net.* projects, so they are not shared with other projects.

[stephentoub]

All NetEventSource types are inside System.Net.* projects, so they are not shared with other projects.

There's NetEventSource.Common.cs, which is built into all of the System.Net.* projects, which then each have their own NetEventSource.SpecificName.cs partial class that provides the actual attribute with the specific name. Any code in common that uses NetEventSource is using the shared definitions.

[eerhardt]

As the code is located in a common location it'd be better to update the code to follow pattern which can be automatically removed

Another alternative is to teach the linker to trim this code automatically. I've logged dotnet/linker#1318 for this pattern. If the linker can support it, we shouldn't need to make any change here.

[alnikola]

@marek-safar Please disregard my above comment. I misunderstood you.

[marek-safar]

There's NetEventSource.Common.cs, which is built into all of the System.Net.* projects

Yep, this PR is fixing the problem for a single assembly but by quick check, it looks like same code is compiled into about 5 other assemblies. They are not Browser critical but still worth considering for other size sensitive configs.

[stephentoub]

I'm fine with us changing the if (SomeEventSource.IsEnabled) { ... } checks to instead be the more canonical if (SomeEventSource.Log.IsEnabled) { ... } checks. I don't remember why we did the static in the first place; maybe it was a misguided attempt on my part to keep things ever-so-slightly shorter at the call site.

[eerhardt]

I'm fine with us changing the if (SomeEventSource.IsEnabled) { ... } checks to instead be the more canonical if (SomeEventSource.Log.IsEnabled) { ... } checks.

I can make that change for all the EventSource classes (thanks @alnikola for the pointers on the new ones coming in) if we decide the linker change (dotnet/linker#1318) is not feasible for 5.0.

cc @vitek-karas

[eerhardt]

I've updated the PR to remove NetEventSource.IsEnabled and HttpTelemetry.IsEnabled static properties since the underlying linker issue isn't a trivial fix and it is unlikely to be fixed for 5.0. I also checked other EventSource implementations in the libraries, and these were the only two with this pattern.

New approach taken.

[stephentoub]

If the clauses here aren't swapped, will the linker end up making this if (closeTask.IsFaulted) { } or will it be able to eliminate the whole if block, regardless?

[marek-safar]

The linker will eliminate the conditionally guarded block, any conditions guarding it will be untouched.

[stephentoub]

I skimmed through the changes. At this point it's "just" changing if (NetEventSource.IsEnabled) to be if (NetEventSource.Log.IsEnabled) in the tons of places that occurs, right? Just wanted to make sure I didn't miss anything more critical. Assuming so, LGTM.

[eerhardt]

At this point it's "just" changing if (NetEventSource.IsEnabled) to be if (NetEventSource.Log.IsEnabled) in the tons of places that occurs, right?

Correct. The changes were:

1. Remove the static new bool IsEnabled properties from NetEventSource and HttpTelemetry classes.
2. Massive Find and Replace XXEventSource.IsEnabled to XXEventSource.Log.IsEnabled()
3. Fix one fake/mock NetEventSource class in the tests (this is the third commit)