Avoid string/StringBuilder/char[] allocation in LogValuesFormatter's ctor #44746
Conversation
…ctor This is producing thousands of allocations at ASP.NET startup, due to almost 1000 call sites to LoggerMessage.Define.
stephentoub
added
tenet-performance
|
Tagging subscribers to this area: @maryamariyan Issue Details
|
src/libraries/Microsoft.Extensions.Logging.Abstractions/src/LogValuesFormatter.cs
Show resolved
Hide resolved
| sb.Append(format, scanIndex, openBraceIndex - scanIndex + 1); | ||
| sb.Append(_valueNames.Count.ToString(CultureInfo.InvariantCulture)); | ||
| vsb.Append(format.AsSpan(scanIndex, openBraceIndex - scanIndex + 1)); | ||
| vsb.Append(_valueNames.Count.ToString()); | ||
| _valueNames.Add(format.Substring(openBraceIndex + 1, formatDelimiterIndex - openBraceIndex - 1)); |
There was a problem hiding this comment.
Would it be worth delaying this Substring alloc?
There was a problem hiding this comment.
Delaying it to when?
There was a problem hiding this comment.
Delaying it until the string is actually used.
I am not super familiar with this area, but if we only captured the Ranges here, and returned the _valueNames as ReadOnlyMemory<char>, maybe we could save alloc'ing these as strings all together?
There was a problem hiding this comment.
maybe we could save alloc'ing these as strings all together?
If this is likely, then yeah, we should consider that. But I have no idea how common that is (and if it's at all common, it'll end up being worse as we'll end up having stored the ranges and then needing to also store / replace that with the constructed strings).
Let's address it separately.
There was a problem hiding this comment.
I would stringy those up front.
There was a problem hiding this comment.
Meaning as-is, right? Does that mean these names are generally used?
There was a problem hiding this comment.
Yep. We have another source generator thing that will replace this.
There was a problem hiding this comment.
This was roughly what I was thinking: eerhardt@aa33457. At first I thought I could eliminate the substring allocs all together.
However, it appears that there is a hidden expectation that the scope objects implement IReadOnlyList<KeyValuePair<string, object>>, which means this would be a breaking change. Or we could make the LogValues structs implement both IReadOnlyList<KeyValuePair<string, object>> and IReadOnlyList<KeyValuePair<ReadOnlyMemory<char>, object>>, and have the Console logger (and any other loggers) check for both.
ghost
locked as resolved and limited conversation to collaborators
This is producing thousands of allocations at ASP.NET startup, due to almost 1000 call sites to LoggerMessage.Define.
Related to #44598
cc: @eerhardt, @maryamariyan