Allocate MsQuicSafeHandle._traceId lazily #72360
Conversation
ghost
assigned 
|
Tagging subscribers to this area: @dotnet/ncl Issue DetailsCloses #71935. I tried running it in our benchmarks via Crank but I have not observed any significant perf difference. I also put a breakpoint in the ToString() of the
|
| $"GetParam({handle}, {parameter}) failed"); | ||
| (byte*)&value); | ||
|
|
||
| if (StatusFailed(status)) |
There was a problem hiding this comment.
What's the difference between this and what ThrowHelper.ThrowIfMsQuicError does?
There was a problem hiding this comment.
It does not force creating the interpolated string (and avoids call to SafeMsQuicHandle.ToString()) on success path
There was a problem hiding this comment.
If we feel strongly about not creating _traceId we might add condition in SafeMsQuicHandle.ToString. If we don't, and we're ok allocating it in case of an error, we could add overload for ThrowIfMsQuicError that takes FormattableString.
I got the feeling that this will creep back in if we don't take precautions. What do you think?
There was a problem hiding this comment.
Can we use FormattableString in a way that does not cause excess allocations? I have never used it and the few tutorials I found look like the args are embedded in the instance (which would imply allocating one instance per call)
There was a problem hiding this comment.
FormattableString will incur its own costs: you won't get a string formatted, but you will get two allocations, for the FormattableString itself and for the object[] args array.
I think what you have here is fine.
There was a problem hiding this comment.
(If you really wanted to, you could implement a custom interpolated string handler, but I don't think it's worth it in this case: https://devblogs.microsoft.com/dotnet/string-interpolation-in-c-10-and-net-6/)
There was a problem hiding this comment.
Yeah, I should have realize that it incurs its own allocations. Custom implementation is certainly not worth it.
Closes #71935.
I tried running it in our benchmarks via Crank but I have not observed any significant perf difference.
I also put a breakpoint in the ToString() of the
MsQuicSafeHandleto really check that we don't force the allocation anywhere.