Binary writer fix #107369
Binary writer fix #107369
Conversation
ghost
added
the
dotnet-policy-service
bot
added
the
community-contribution
| OutStream = Stream.Null; | ||
| _encoding = Encoding.UTF8; | ||
| _useFastUtf8 = true; | ||
| _isDerivedWriter = GetType() != typeof(BinaryWriter); |
There was a problem hiding this comment.
Note: You do not need to save the result into a field. GetType() != typeof(X) compiles into a single instruction.
There was a problem hiding this comment.
Ah, alright. As I mentioned, feel free to adjust it. Or let me know if you want me to change it.
|
The original issue was marked as "needs further triage" and we haven't decided what's the expected behavior. Since it's not a recent change happened from 9.0, it's not likely to change for 9.0. |
Understood. Actually I can live with the (far from optimized) workaround I applied as it's just a unit test helper class: public override void Write(string value)
{
#if !NET6_0_OR_GREATER // https://github.com/dotnet/runtime/issues/107265
base.Write(value);
#else
var bytes = Encoding.UTF8.GetBytes(value);
Write7BitEncodedInt(bytes.Length);
OutStream.Write(bytes, 0, bytes.Length);
#endif
// [...] actual logging, stack trace dump
} |
|
add a unit test in |
In the end I did not add a new test case but modified the Now the already existing |
There was a problem hiding this comment.
@koszeggy First of all please excuse me for such a delay in the code review process. For some reason I've missed the GH notification.
The fix LGTM, but I need to ensure that it's not going to regress the performance before merging it.
| // If this is a non-derived BinaryWriter, then we can bypass the Write7BitEncodedInt call. | ||
| // But when this is a derived instance, call must not bypass it for compatibility reasons | ||
| // as it calls the virtual Write(int) overload. | ||
| if (GetType() == typeof(BinaryWriter) && value.Length <= 127 / 3) |
There was a problem hiding this comment.
@EgorBo How is runtime optimizing the logic that uses GetType() == typeof(BinaryWriter) pattern as of today? Will this be always replaced with a constant for each derived type? If not, should we cache the result of the check? Or at least change the order (perform the cheap length check first)
There was a problem hiding this comment.
It's a single pointer check (*object == TypeTableOf(BinaryWriter)) and equivalent to accessing a field. This pattern is used a lot currently.
Fixes #107265
I see that you already set the milestone to v10 but hoping that some contribution may help to add this to .NET 9 I prepared a possible solution. Of course, I understand if it's too late.
Feel free to adjust it.