Added LoggerMessage.Define overloads to disable IsEnabled check

[gfoidl]

Fixes #45290

/cc: @davidfowl @KalleOlaviNiemitalo

[dotnet-issue-labeler]

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

[ghost]

Tagging subscribers to this area: @maryamariyan
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Fixes #45290

/cc: @davidfowl @KalleOlaviNiemitalo

Author:	gfoidl
Assignees:	-
Labels:	area-Extensions-Logging, new-api-needs-documentation
Milestone:	-

[davidfowl]

I'm assuming the codegen differences are negligible?

[gfoidl]

The methods in the closure / display-class look like

internal void <Define>b__1(ILogger logger, Exception? exception)
{
    <Define>g__Log|0(logger, exception);
}

internal void <Define>b__2(ILogger logger, Exception? exception)
{
    if (logger.IsEnabled(logLevel))
    {
        <Define>g__Log|0(logger, exception);
    }
}

internal void <Define>g__Log|0(ILogger logger, Exception? exception)
{
    logger.Log(logLevel, eventId, new LogValues(formatter), exception, LogValues.Callback);
}

so the JIT should inline <Define>g__Log|0 and in effect the codegen be the same.
Add aggressive inlining?

Before it was

internal void <Define>b__0(ILogger logger, Exception? exception)
{
    if (logger.IsEnabled(logLevel))
    {
        logger.Log(logLevel, eventId, new LogValues(formatter), exception, LogValues.Callback);
    }
}

[davidfowl]

I figured it should inline. I was gonna check but https://sharplab.io/ is down 😢

[gfoidl]

Wanted to do the same, unfortunately...we're too dependent on sharplab 😄

[davidfowl]

We all need to sponsor @ashmind

[davidfowl]

Grrr. Can you manually inline then?

[davidfowl]

Does it change if you do this:

if (skipEnabledCheck)
{
    return Log;
}

[davidfowl]

Lets change this to return the Log method directly for the skipEnabledCheck

[gfoidl]

Done with 3e547f1

The other branch (!skipEnabledCheck) is left as is. Codegen (from above) is the check for IsEnabled then the tail-call. I assume together with PGO machine-code layout will be good, so there should be no perf-penalty.
On local micro-benchmarks (with a Null-Logger) I couldn't see any difference between calling Log(...) local function or manually inlining.

With the suggested change to return Log directly for the skipEnabledCheck the IL size is smaller too and the indirection for the tail-call is gone.

[maryamariyan]

@gfoidl did you find micro benchmark use cases for which adding a skipEnabledCheck would have improved performance?

[davidfowl]

LGTM

[davidfowl]

thanks @gfoidl, stellar work as usual!

[maryamariyan]

@gfoidl we forgot to add tests for the new APIs. Would you be open to adding a PR for the code coverage?

[gfoidl]

Sure, will address this over the weekend.

[stephentoub]

What is the cost of the check this is skipping? Is it not just a compare and a branch (or is it also skipping an extra interface dispatch)? Are there benchmarks that show these overloads are worth it?

[davidfowl]

Delegate invocations can't be inlined. So the cost is a delegate invocation unless an explicit check is added on the outside. This just removes the redundant check when IsEnabled is being used

[davidfowl]

Also FWIW, the logging system needs to ask multiple providers if IsEnabled is true. It isn't a simple check. First there's a filtering at the logger factory level and then it needs to call into each provider to ask it if it's enabled. That's up to n interface dispatches per call. Repeating it is just redundant when you know you're going to check it before the call.

[stephentoub]

Thanks. Is there a typical example as would show up in a real app that highlights these costs well? Do we have benchmarks around this somewhere?

[davidfowl]

#48619