[draft] Trim ActivitySource via System.Diagnostics.ActivitySource.IsSupported feature #114636
Conversation
| @@ -105,7 +105,7 @@ internal static string GetAsyncStateMachineDescription(IAsyncStateMachine stateM | |||
| [MethodImpl(MethodImplOptions.NoInlining)] | |||
| internal static void LogTraceOperationBegin(Task t, Type stateMachineType) | |||
| { | |||
| TplEventSource.Log.TraceOperationBegin(t.Id, "Async: " + stateMachineType.Name, 0); | |||
| if (EventSource.IsSupported) TplEventSource.Log.TraceOperationBegin(t.Id, "Async: " + stateMachineType.Name, 0); | |||
There was a problem hiding this comment.
The call to this method is under if (TplEventSource.Log.IsEnabled()) condition. Why do we need to duplicate the condition here?
There was a problem hiding this comment.
(Dtto for all other places that have this duplication.)
There was a problem hiding this comment.
I think this should be done by telling the trimmer to replace EventSource.IsEnabled() with false when EventSource is not supported. It does not make sense to duplicate the EventSource.IsSupported condition everywhere.
There was a problem hiding this comment.
Yeah, I'm unhappy about size of that diff too.
I need to investigate more, this is "stash it for later, it's too late tonight" .
I suspect that TplEventSource.Log.IsEnabled() is holding TplEventSource.Log alive even after trimming.
Maybe I could introduce TplEventSource.IsEnabled() instead which would call TplEventSource.Log.IsEnabled() and trim/substitute that for false. But it must not trim when EventSource.IsSupported is true. And so it must be XML and not an attribute.
There was a problem hiding this comment.
TplEventSource.Log alive even after trimming.
It is expected to be only a small dummy artifact. All EventSource methods that do any actual work have if (IsSupported) return; or similar, so all heavy lifting should be trimmed already.
There was a problem hiding this comment.
Maybe I could introduce TplEventSource.IsEnabled()
I would focus on fixes in the EventSource itself. I would try to avoid changing every actual event source out there, even if it means leaving a tiny bit of size on the table.
There was a problem hiding this comment.
The trimmer is not that smart. Any mention of some class keeps the class alive and the class bodies are usually not trimmed. For example DiagLinkedList is static of Activity class. And ball of dead code keeps rolling.
There was a problem hiding this comment.
activity1 is always null after trimming.
Activity activity1 = (Activity) null;
if (!SocketsTelemetry.IsActivitySourceSupported)
;
if (activity1 != null)
{
... bloat
}
@sbomer is this bug that would be considered for fixing, or I should seek workarounds ?
There was a problem hiding this comment.
Created #114730 with simple repro
There was a problem hiding this comment.
And maybe it's same as dotnet/linker#1284
pavelsavara
added
size-reduction
|
Tagging subscribers to this area: @tommcdon |
- MSBuild property ActivitySourceSupport
pavelsavara
changed the title
| private const string ActivitySourceName = "Experimental.System.Net.NameResolution"; | ||
| private const string ActivityName = ActivitySourceName + ".DnsLookup"; | ||
| private static readonly ActivitySource s_activitySource = new ActivitySource(ActivitySourceName); | ||
| private static readonly ActivitySource? s_activitySource; |
There was a problem hiding this comment.
| private static readonly ActivitySource? s_activitySource; | |
| private static readonly ActivitySource? s_activitySource = IsActivitySourceSupported ? new ActivitySource(ActivitySourceName) : null; |
Instead of the explicit constructor and warning disable?
|
|
||
| private const string EventSourceSuppressMessage = "Parameters to this method are primitive and are trimmer safe"; | ||
| public static readonly NetSecurityTelemetry Log = new NetSecurityTelemetry(); | ||
|
|
||
| #pragma warning disable CA1810 // remove the explicit static constructor |
System.Diagnostics.ActivitySource.IsSupportedContribute to #114635
On top of #114326