Binary writer fix - #107369
#107369Conversation
|
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 |
adamsitnik
left a comment
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.
There was a problem hiding this comment.
Pull request overview
This PR fixes a breaking change in BinaryWriter.Write(string) introduced in .NET 6.0, where an optimization for short UTF-8 strings bypassed the Write7BitEncodedInt method, preventing derived classes from tracking writes through their overridden Write(byte) method. The fix ensures that derived BinaryWriter classes maintain consistent virtual method dispatch by adding a type check before applying the optimization.
Changes:
- Added
GetType() == typeof(BinaryWriter)check to ensure the fast-path string optimization is only used for non-derived BinaryWriter instances - Added comprehensive tests with derived BinaryWriter/BinaryReader classes that track internal positions to verify virtual method calls are symmetric
- Tests cover both UTF-8 and UTF-16 encodings with both base and derived classes
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/libraries/System.Private.CoreLib/src/System/IO/BinaryWriter.cs | Added type check to preserve virtual method dispatch for derived BinaryWriter classes when writing strings |
| src/libraries/System.Runtime/tests/System.IO.Tests/BinaryWriter/BinaryWriter.WriteTests.cs | Added TestWriter and TestReader classes that track position, expanded WriteTest to run with 4 scenarios (base/derived × UTF-8/UTF-16), added position synchronization validation |
|
@koszeggy / @adamsitnik I tasked Copilot with Add BinaryWriter benchmarks for _useFastUtf8 true and false code paths (dotnet/performance#5121) to unblock this PR. @huoyaoyuan's comment long ago reassured us the pattern taken here should be OK but for this change I do want to make sure we have benchmarks that would catch any salient perf changes. @adamsitnik Can you re-review though too please? |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
/ba-g BadExit on an unrelated test |
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.