Fix Log Formatter mutating its arguments. #38734
Conversation
Log Formatter now creates a new object array and inserts the formatted parameters there instead of the original array. Also added a test to verify if the types of the parameters remain unchanged. Fix #36165
|
I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label. |
|
|
|
@davidfowl expressed this concern in the linked issue: Have you validated that this doesn't measurably regress important usage? |
| { | ||
| object[] formattedValues = null; | ||
|
|
||
| if (values != null) |
There was a problem hiding this comment.
Might values be Length == 0? There's no benefit allocating in that case.
There was a problem hiding this comment.
No, when the values array is either null or of length 0, the Log formatter is set to null and in that case, array formatting is skipped.
This does makes the check for a null array redundant, no?
| for (int i = 0; i < values.Length; i++) | ||
| { | ||
| values[i] = FormatArgument(values[i]); | ||
| formattedValues[i] = FormatArgument(values[i]); |
There was a problem hiding this comment.
It looks like FormatArgument will often just return the input, and if that's the case for all inputs, there's no benefit allocating an array. Seems like this would be better off lazily creating the new array the first time a formatted argument is a different reference than the input. (That will also implicitly take care of the empty case I mentioned.)
There was a problem hiding this comment.
On top of that, it looks like this method has four call sites, three of which always pass in a brand new array, where it doesn't matter if the method modifies it. Seems like there should be a separate path for that, if this really is hot.
@stephentoub I'm running the dotnet/performance microbenchmarks now, on both the proposed solution and on a preliminary lazy-loaded version. What is the best way to share the benchmark results? |
I doubt there's any coverage of this there. These libraries were only just moved to dotnet/runtime recently, and nothing else in dotnet/runtime will depend on these. I expect the perf impact would show up more in an ASP.NET application of some kind. But if nothing else, you could write a microbenchmark on the relevant types. |
|
I would write a new micro benchmark for this. I think doing the work to avoid the allocation in the very common case where the args aren’t modified is what I was originally thinking |
I wrote some benchmarks, should I try to do a PR on the benchmark repo or would you like me to add them here before that? Also, I'm afraid that having to check every time if the returned object matches the reference of the original value to only then allocate the new array will not be effective, specially when an enumerable is the last item in the param array... |
Why? |
|
To be clear, I'm suggesting it essentially be this: public string Format(object[] values)
{
object[] formattedValues = values;
if (values != null)
{
for (int i = 0; i < values.Length; i++)
{
object formattedValue = FormatArgument(values[i]);
if (!ReferenceEquals(values[i], formattedValue)
{
formattedValues = new object[values.Length];
Array.Copy(values, 0, formattedValues, 0, i);
formattedValues[i++] = formattedValue;
for (; i < values.Length; i++)
{
formattedValues[i] = FormatArgument(values[i]);
}
break;
}
}
}
return string.Format(CultureInfo.InvariantCulture, _format, formattedValues ?? Array.Empty<object>());
} |
|
Tagging subscribers to this area: @maryamariyan |
|
@vaz-rodrigo, are you still working on this? |
|
Closing this PR. @vaz-rodrigo feel free to create a new PR with the recommended changes. |
ghost
locked as resolved and limited conversation to collaborators
6 participants
Log Formatter now creates a new object array and inserts the formatted
parameters there instead of the original array.
Also added a test to verify if the types of the original objects remain
unchanged.
Fix #36165