DiagnosticListener performance optimization

[lmolkova]

Motivation

It is motivated as a performance optimization. The IsEnabled() with no arguments allow a inlinable quick check for performance reasons.

Details

Add DiagnoticListener.IsEnabled() without arguments as an initial loose check that returns true if there is a subscriber to DiagnosticListener (suggested in dotnet/corefx#15971).

API proposal

public bool IsEnabled() {return _subscriptions != null;};

/cc @vancem @stephentoub

[karelz]

@vancem if it is ready for API review, please mark it as such before 10am tomorrow (the API review meeting).

[vancem]

Note that this is motivated as a performance optimization. It allows DiagnosticSource code to avoid a virtual calls. The IsEnabled() with no arguments allow a inlinable quick check, again for performance reasons.

[karelz]

@vancem only one api-* label please ;-)

[karelz]

@vancem I updated the top post with your 'motivation' - please feel free to make such edits of top post proposal directly in future. Thanks!

[terrajobst]

The (1) looks fine.

We're not sure about the argument for sealing. Why do you think the compiler is able to emit a non-virtual call? @jaredpar, is this actually true?

[vancem]

When you have a class C and it has a definition, and that method is sealed, then the target MUST be the definition in C (because it must be the most derived version, and sealing it insures that the definition in C is the most derived version). This means that the method does NOT need to be called virtually (which means it can be inlined) I have not checked recently but the JIT used to have this exact optimization.

[benaadams]

We're not sure about the argument for sealing. Why do you think the compiler is able to emit a non-virtual call?

Hasn't been true for the 15 year lifetime of .NET; though is often cited. However... its on its way! 🎉
dotnet/coreclr#9230

[jaredpar]

There are two aspects:

1. Can sealing cause the managed compiler to emit a .call instruction vs. a .callvirt? The former in some cases is more efficient because it avoids a null check. In some cases the JIT can avoid the null check even with the .callvirt. But the managed compiler can be more flexible because it only has to obey language semantics while the JIT has to obey IL semantics (stricter).
2. Can the JIT itself emit more efficient code for a .callvirt instruction if the type is sealed?

As @benaadams points out 2 is not true yet but is on the way in CoreClr.

As for 1 we've been having a lot of discussions about this lately on the managed compiler team. Met with the JIT team last week to discuss what this would / wouldn't buy us. The conclusion is that it would likely bring some advantages but we'd need to prototype to see if it was worth the complexity and compat trade offs.

[benaadams]

What would be the pattern? If you were holding a DiagnoticSource you would need to drop to listener to get the advantages so something like?

if((source as DiagnoticListener)?.IsEnabled() ?? true)
{
    // ...
}

Or would it be recommended to always hold DiagnoticListener directly?

[lmolkova]

@benaadams it's recommended to use DiagnosticListener directly.

[vancem]

Indeed. the typical usage pattern for DiagnosicSource/DiagnosticListener is for a component (say HTTP) to itself create a static DiagnosticListener (HttpDiagnosticListener), and write to it. Thus it is very easy for that static variable to be declared as a DiagnosticListener (rather than a DiagnosticSource) and thus the new methods are available.

[terrajobst]

 // Diff between .NET Core 1 and proposed
 namespace System.Diagnostics
 {
     public class DiagnosticListener
     {
+         public bool IsEnabled() {return _subscriptions != null;};
-         public override bool IsEnabled(string name);
+         public sealed override bool IsEnabled(string name); //marked as sealed
+         public sealed override bool IsEnabled(string name, object arg1, object arg2 = null); //marked as sealed
-         public override void Write(string name, object parameters);
+         public sealed override void Write(string name, object parameters); //marked as sealed
     }
 }

Adding sealed to IsEnabed(string) and Write(string, object) would be a breaking change to .NET Core 1.0 (source and binary). It's unlikely that we would approve that (@ericstj).

Would you still do the change if you can only seal the newly added methods? We think this would make the API inconsistent and thus even worse. Thoughts?

[benaadams]

public bool IsEnabled() being non-virtual would get most of the fast "everything-disabled" advantages?

[lmolkova]

@benaadams
Correct, producer should first call IsEnabled without arguments and only then check for a particular event name to be enabled:

public void OnIncomingRequest(DiagnosticListener listener, HttpContext context)
{
   if (listener.IsEnabled() && listener.IsEnabled("Microsoft.AspNetCore.Hosting.BeginRequest"))
   {
        listener.Write("Microsoft.AspNetCore.Hosting.BeginRequest", new {Context = context})
   }
  //continue processing
}