Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid boxing of TState #241

Closed
jods4 opened this issue Feb 12, 2024 · 6 comments · Fixed by #242
Closed

Avoid boxing of TState #241

jods4 opened this issue Feb 12, 2024 · 6 comments · Fixed by #242

Comments

@jods4
Copy link
Contributor

jods4 commented Feb 12, 2024

[LoggerMessage] generate a struct state, in an attempt to reduce allocations.

For this to be meaningful, state should not be boxed, but it is in AsLoggableValue:

https://github.com/serilog/serilog-extensions-logging/blob/dev/src/Serilog.Extensions.Logging/Extensions/Logging/SerilogLogger.cs#L168-L174

// Current version:
static object? AsLoggableValue<TState>(TState state, Func<TState, Exception?, string>? formatter)
{
    object? stateObj = state;  // <- always boxing!!
    if (formatter != null)
        stateObj = formatter(state, null);
    return stateObj;
}

// Suggested:
static object? AsLoggableValue<TState>(TState state, Func<TState, Exception?, string>? formatter)
{
    object? stateObj = null;
    if (formatter != null)
        stateObj = formatter(state, null);
    return stateObj ?? state; // <- boxed only when needed
}

Note that in the case of [LoggerMessage] a formatter is always provided (to create the formatted string without boxing of its parameters), so stateObj will never be null and state will not be boxed.

For the curious mind, in PrepareWrite() the code if (state is IEnumerable<..> structure) does not result in boxing because JIT compiles each struct TState separately and statically optimizes these conditions and interfaces, as can be seen in this disassembly (call S.GetEnumerator):
https://sharplab.io/#v2:EYLgtghglgdgNAFxBAzmAPgAQEwEYCwAUJgAwAEmuAdAMID2ANgwKYDGCUdMKA3EaRVwAWPoSIoEAJwCu7MgGUyIQQGYAPAGlmATwBqEBtOYAFaJLWUScMnWAArNggB8TogG8iZLxRVkAkgCiMNJgzJIQCHTmWnoGRqZQ5pbWtg7sLmQA4swIQSFhEVEAFACUnt4ehN7VZAgAFpJ0AO5kMMwtAHJ0CH5gAA4soTAIzAAmAQAerMx9HFylotUAvkTlXoHBoeGRkqpU2bmbBTula2SVNd71jS1tnd29A8xDI+NTM3MwC2crhL/8vhwZBo5zOAG15HUIJI+gAZCDAKgAJWkwygoSoACkoAhsm1JFBWEUENo+sw6AAzIryEolAC6Z0wgNwADYKEIyABZNQAFScRR5ZAmJVBVRqUApZCKEzIUBQqk0On0hhMZgsuCsNnsjgyzDKYuqF0u3gpUWYEFYdSlADdoWQANbWvqymBkPVnS5G401SgATiKjr6VBiJUW3rIv0uke8vyWQA==

@jods4
Copy link
Contributor Author

jods4 commented Feb 12, 2024

Another cheap win in .net < 9.0: StartsWith(char) is quite faster than StartsWith(string) on these two lines:
https://github.com/serilog/serilog-extensions-logging/blob/dev/src/Serilog.Extensions.Logging/Extensions/Logging/SerilogLogger.cs#L106-L111

In .net 9+ there's a fast path in StartsWith(string) that checks if (prefix.Length == 1) return str.StartsWith(prefix[0]).

@jods4
Copy link
Contributor Author

jods4 commented Feb 12, 2024

BTW: it would be nice to have similar fast logging when using Serilog natively!

@nblumhardt
Copy link
Member

Thanks for all the thoughts and suggestions @jods4!

Anyone keen to investigate/PR any of these? 😎

jods4 added a commit to jods4/serilog-extensions-logging that referenced this issue Feb 12, 2024
@jods4
Copy link
Contributor Author

jods4 commented Feb 12, 2024

@nblumhardt The first changes are so trivial... here you go: #242

I should have opened a different issue for having performant code generator that works with Serilog.Log.
That's a whole lot more complicated, cannot be closed as quickly 😆

@jods4
Copy link
Contributor Author

jods4 commented Feb 12, 2024

Nothing is ever trivial 🤣
Turns out StartsWith(char) was not part of .net fx of good ole time, so I reverted.

Could have gone creative with str is ['@', ..], turns out that as optimal as it gets. Felt excessive, though 😏

@nblumhardt
Copy link
Member

Thanks again, keen to keep chipping away at these kinds of things, nice to see small gains add up over time :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants