Reduce boxing in logging #88560
Reduce boxing in logging #88560
Conversation
- Logging got overhauled to use CompositeFormat in .NET 8, this allows for non boxing string formatting via string.Format. There's one piece of code that boxing the generic argument before handing off to string.Format so remove it for primitive types and enums.
|
Tagging subscribers to this area: @dotnet/area-extensions-logging Issue Details
Fixes #50768
|
davidfowl
changed the title
|
Tks! |
| // Avoiding boxing of known types | ||
| if (typeof(T).IsPrimitive || typeof(T).IsEnum) | ||
| { | ||
| stringValue = null; | ||
| return false; | ||
| } |
There was a problem hiding this comment.
What benefit does this block of code provide? I'm not seeing where boxing would be coming from without it, at least not in optimized code. For a primitive or an enum, the JIT should effectively get rid of everything else in the method already, since it'll be generically specialized for that value type T and the JIT can tell the value is non-null and not an IEnumerable.
There was a problem hiding this comment.
at least not in optimized code.
This is what I was worried about. Maybe I was looking at tier 0 code? I can revert, but while profiling this I was seeing boxing and I assumed it was coming from the interface check (if T is IEnumerable).
There was a problem hiding this comment.
Maybe I was looking at tier 0 code?
That's my guess.
I assumed it was coming from the interface check (if T is IEnumerable).
That shouldn't box in optimized code (unless the T actually implements IEnumerable, in which case it's not a primitive or an enum, anyway). If it does, we should fix the JIT. @EgorBo?
There was a problem hiding this comment.
It looks like both of you're correct - we optimize this in Tier1 but still allocate in Tier0 (although, should not be a real issue for this case because BCL libs are prejitted, hence, the boxing will be always optimized unless user explicitly defines DOTNET_ReadyToRun=0). Filed #88987 with a minimal repro
There was a problem hiding this comment.
Ah! IIRC allocation profiler sets DOTNET_ReadyToRun=0.
There was a problem hiding this comment.
so I can probably remove this workaround in that PR
I think we should remove it, regardless. We generally don't add code to optimize tier 0.
There was a problem hiding this comment.
Sure I'll send a PR, I just wanted to understand why I was seeing boxing in the tool 😄.
There was a problem hiding this comment.
So this doesn't explain why you were seeing boxing from here...
Certainly the boxing via the object argument would result in boxing, and this PR should have fixed that, but I don't know why you would have seen boxing in the tool after your object=>T changes, unless you unchecked that Disable Tiered Compilation setting.
ghost
locked as resolved and limited conversation to collaborators
Fixes #50768
(while this doesn't fully fix the problem, it's as improved as we would make it for now for < 4 arguments).
PS: Between 4 and 10 arguments, I think we can consider using the new inline array to avoid the object array allocation (probably not worth it, but fun nonetheless).