Reduce allocations in `BinaryReader`. #80331

teo-tsirpanis · 2023-01-07T16:59:26Z

This PR optimizes the BinaryReader class to allocate its fields only if we call its text-related methods. Additionally _buffer and _charBytes fields were removed and replaced with stack allocations or array pool rents.

ghost · 2023-01-07T16:59:38Z

Tagging subscribers to this area: @dotnet/area-system-io
See info in area-owners.md if you want to be subscribed.

Issue Details

When a BinaryReader is created, it creates a small byte array and uses it as a temporary buffer. This buffer is used in the Read*** methods of various numeric types, and in the legacy FillBuffer method.

In the former case this PR uses stack-allocated memory instead, and in the latter case the byte array is allocated lazily. I also improved stuff like a byte array slicing to use spans.

Author:	teo-tsirpanis
Assignees:	-
Labels:	`area-System.IO`, `community-contribution`
Milestone:	-

src/libraries/System.Private.CoreLib/src/System/IO/BinaryReader.cs

teo-tsirpanis · 2023-01-07T21:52:39Z

src/libraries/System.Private.CoreLib/src/System/IO/BinaryReader.cs

                if (n == 0)
                {
                    ThrowHelper.ThrowEndOfFileException();
                }

-                charsRead = _decoder.GetChars(_charBytes, 0, n, _charBuffer, 0);
+                charsRead = _decoder.GetChars(charBytes[..n], _charBuffer, flush: false);


The use of a stateful class like Decoder in this method does not seem right to me. Isn't each invocation of ReadString standalone? Shouldn't we Reset it on each non-text operation?

From the docs:

Reads a string from the current stream. The string is prefixed with the length, encoded as an integer seven bits at a time.

My understanding is that in this particular case the string must be prefixed with the length, so the job of the method is to read the bytes from the stream (and possibly modify it’s state like Position) and covert them into string.

The method does not read all bytes at a time (readLength can be different than stringLength) so the encoder should maintain it's state between the loop iterations:

runtime/src/libraries/System.Private.CoreLib/src/System/IO/BinaryReader.cs

Line 272 in be84df7

stringLength = Read7BitEncodedInt();

runtime/src/libraries/System.Private.CoreLib/src/System/IO/BinaryReader.cs

Lines 289 to 291 in be84df7

readLength = ((stringLength - currPos) > MaxCharBytesSize) ? MaxCharBytesSize : (stringLength - currPos);

n = _stream.Read(_charBytes, 0, readLength);

Now should it reset the state in the final iteration?

Checking for the leftovers and throwing would be considered a breaking change. Just resetting the state would be considered a breaking change as well, because someone in theory could rely on keeping this state.

Shouldn't we Reset it on each non-text operation?

I mean, if we call Read(), it will return the next character, and if we call ReadUInt32(); Read() after that, the decoder's state will be out-of-sync from the stream.

I mean, if we call Read(), it will return the next character, and if we call ReadUInt32(); Read() after that, the decoder's state will be out-of-sync from the stream.

Could you please add a unit test that exhibits the described behavior?

I can't find an easy way to test it. Can we skip it for this PR?

adamsitnik · 2023-01-09T14:44:33Z

@teo-tsirpanis thank you for your contribution! Could you please provide benchmark results that show the gains from the proposed changes? Thanks!

teo-tsirpanis · 2023-01-10T17:53:20Z

I will, let's address #80331 (comment) first.

jeffhandley · 2023-02-06T19:28:46Z

@teo-tsirpanis - It looks like you're blocked and awaiting guidance from @stephentoub or @adamsitnik on the question you posted here; is that the accurate status of this PR?

teo-tsirpanis · 2023-02-06T19:31:11Z

Yes. And I can't work on it for the next couple of days.

adamsitnik

thank you for your contribution @teo-tsirpanis ! I would be fine with the proposed changed if FillBuffer was not part of the public API.

src/libraries/System.Private.CoreLib/src/System/IO/BinaryReader.cs

It now gets allocated only when calling the obscure `FillBuffer` method.

We checked it on the constructor and already had asserts right before the casts.

… is small.

src/libraries/System.Private.CoreLib/src/System/IO/BinaryReader.cs

adamsitnik · 2023-07-31T12:56:52Z

@teo-tsirpanis could you please share the benchmark results?

teo-tsirpanis · 2023-08-02T22:22:10Z

I will do it in the weekend. When will the snap for .NET 9 happen?

adamsitnik · 2023-08-03T08:13:48Z

I will do it in the weekend. When will the snap for .NET 9 happen?

The snap where we could merge all changes already happened on July 17th. Until August 14th we can merge bug fixes, important features and important perf fixes, but we need a direct manager approval. After Aug 14th only impactful issues, with more complex approval process.

teo-tsirpanis · 2023-08-03T23:14:51Z

Here they are:

runtime-old is 41b8bc4 (the commit from main we merged into this branch)
runtime-new is 6273201

BenchmarkDotNet=v0.13.2.2052-nightly, OS=Windows 10 (10.0.19045.3271)
Intel Core i7-7700HQ CPU 2.80GHz (Kaby Lake), 1 CPU, 8 logical and 4 physical cores
.NET SDK=8.0.100-preview.6.23330.14
  [Host]     : .NET 8.0.0 (8.0.23.32907), X64 RyuJIT AVX2
  Job-WTHNYV : .NET 8.0.0 (42.42.42.42424), X64 RyuJIT AVX2
  Job-BHRFRA : .NET 8.0.0 (42.42.42.42424), X64 RyuJIT AVX2

PowerPlanMode=00000000-0000-0000-0000-000000000000  Arguments=/p:EnableUnsafeBinaryFormatterSerialization=true  IterationTime=250.0000 ms  
MaxIterationCount=20  MinIterationCount=15  WarmupCount=1

Method	Job	Toolchain	Mean	Error	StdDev	Median	Min	Max	Ratio	RatioSD	Gen0	Allocated	Alloc Ratio
DefaultCtor	Job-WTHNYV	\runtime-new\shared\Microsoft.NETCore.App\8.0.0\corerun.exe	17.602 ns	1.1102 ns	1.2785 ns	17.796 ns	15.588 ns	19.620 ns	0.54	0.05	0.0178	56 B	0.35
DefaultCtor	Job-BHRFRA	\runtime-old\shared\Microsoft.NETCore.App\8.0.0\corerun.exe	32.702 ns	1.3043 ns	1.5020 ns	32.652 ns	30.358 ns	35.462 ns	1.00	0.00	0.0510	160 B	1.00

ReadBool	Job-WTHNYV	\runtime-new\shared\Microsoft.NETCore.App\8.0.0\corerun.exe	1.346 ns	0.1610 ns	0.1790 ns	1.350 ns	1.133 ns	1.777 ns	0.88	0.15	-	-	NA
ReadBool	Job-BHRFRA	\runtime-old\shared\Microsoft.NETCore.App\8.0.0\corerun.exe	1.572 ns	0.1748 ns	0.1795 ns	1.528 ns	1.340 ns	2.028 ns	1.00	0.00	-	-	NA

ReadAsciiChar	Job-WTHNYV	\runtime-new\shared\Microsoft.NETCore.App\8.0.0\corerun.exe	17.657 ns	0.6939 ns	0.7425 ns	17.350 ns	16.933 ns	19.590 ns	0.86	0.06	-	-	NA
ReadAsciiChar	Job-BHRFRA	\runtime-old\shared\Microsoft.NETCore.App\8.0.0\corerun.exe	20.638 ns	0.8774 ns	0.9010 ns	20.428 ns	19.445 ns	22.915 ns	1.00	0.00	-	-	NA

ReadNonAsciiChar	Job-WTHNYV	\runtime-new\shared\Microsoft.NETCore.App\8.0.0\corerun.exe	102.504 ns	3.9581 ns	4.5582 ns	102.609 ns	93.493 ns	109.382 ns	1.03	0.04	-	-	NA
ReadNonAsciiChar	Job-BHRFRA	\runtime-old\shared\Microsoft.NETCore.App\8.0.0\corerun.exe	99.448 ns	2.4639 ns	2.5302 ns	99.793 ns	94.699 ns	104.341 ns	1.00	0.00	-	-	NA

ReadUInt16	Job-WTHNYV	\runtime-new\shared\Microsoft.NETCore.App\8.0.0\corerun.exe	4.289 ns	0.2605 ns	0.3000 ns	4.200 ns	3.760 ns	4.873 ns	0.75	0.06	-	-	NA
ReadUInt16	Job-BHRFRA	\runtime-old\shared\Microsoft.NETCore.App\8.0.0\corerun.exe	5.701 ns	0.1344 ns	0.1191 ns	5.707 ns	5.545 ns	5.977 ns	1.00	0.00	-	-	NA

ReadUInt32	Job-WTHNYV	\runtime-new\shared\Microsoft.NETCore.App\8.0.0\corerun.exe	4.004 ns	0.2161 ns	0.2489 ns	3.982 ns	3.619 ns	4.481 ns	0.73	0.05	-	-	NA
ReadUInt32	Job-BHRFRA	\runtime-old\shared\Microsoft.NETCore.App\8.0.0\corerun.exe	5.475 ns	0.1395 ns	0.1493 ns	5.469 ns	5.302 ns	5.792 ns	1.00	0.00	-	-	NA

ReadUInt64	Job-WTHNYV	\runtime-new\shared\Microsoft.NETCore.App\8.0.0\corerun.exe	3.978 ns	0.2170 ns	0.2499 ns	3.960 ns	3.577 ns	4.521 ns	0.73	0.05	-	-	NA
ReadUInt64	Job-BHRFRA	\runtime-old\shared\Microsoft.NETCore.App\8.0.0\corerun.exe	5.455 ns	0.1771 ns	0.1969 ns	5.505 ns	5.137 ns	5.744 ns	1.00	0.00	-	-	NA

ReadHalf	Job-WTHNYV	\runtime-new\shared\Microsoft.NETCore.App\8.0.0\corerun.exe	5.168 ns	0.5830 ns	0.6480 ns	4.943 ns	4.483 ns	6.770 ns	0.83	0.11	-	-	NA
ReadHalf	Job-BHRFRA	\runtime-old\shared\Microsoft.NETCore.App\8.0.0\corerun.exe	6.138 ns	0.1526 ns	0.1428 ns	6.159 ns	5.866 ns	6.360 ns	1.00	0.00	-	-	NA

ReadSingle	Job-WTHNYV	\runtime-new\shared\Microsoft.NETCore.App\8.0.0\corerun.exe	4.972 ns	0.1933 ns	0.2226 ns	4.944 ns	4.573 ns	5.332 ns	0.81	0.08	-	-	NA
ReadSingle	Job-BHRFRA	\runtime-old\shared\Microsoft.NETCore.App\8.0.0\corerun.exe	6.138 ns	0.3766 ns	0.4337 ns	5.993 ns	5.514 ns	7.003 ns	1.00	0.00	-	-	NA

ReadDouble	Job-WTHNYV	\runtime-new\shared\Microsoft.NETCore.App\8.0.0\corerun.exe	4.903 ns	0.1319 ns	0.1466 ns	4.886 ns	4.689 ns	5.207 ns	0.77	0.07	-	-	NA
ReadDouble	Job-BHRFRA	\runtime-old\shared\Microsoft.NETCore.App\8.0.0\corerun.exe	6.379 ns	0.5360 ns	0.5958 ns	6.387 ns	5.456 ns	7.763 ns	1.00	0.00	-	-	NA

ReadSmallString	Job-WTHNYV	\runtime-new\shared\Microsoft.NETCore.App\8.0.0\corerun.exe	42.309 ns	1.2379 ns	1.3759 ns	42.268 ns	40.002 ns	44.566 ns	0.88	0.07	0.0153	48 B	1.00
ReadSmallString	Job-BHRFRA	\runtime-old\shared\Microsoft.NETCore.App\8.0.0\corerun.exe	47.916 ns	3.2521 ns	3.7452 ns	47.150 ns	43.816 ns	56.249 ns	1.00	0.00	0.0151	48 B	1.00

ReadLargeString	Job-WTHNYV	\runtime-new\shared\Microsoft.NETCore.App\8.0.0\corerun.exe	346.669 ns	16.3936 ns	18.8789 ns	344.723 ns	315.138 ns	391.731 ns	0.90	0.09	0.8388	2632 B	1.00
ReadLargeString	Job-BHRFRA	\runtime-old\shared\Microsoft.NETCore.App\8.0.0\corerun.exe	385.614 ns	22.4324 ns	24.9335 ns	377.237 ns	354.246 ns	432.387 ns	1.00	0.00	0.8385	2632 B	1.00

adamsitnik

Overall it looks good, I found only one minor thing (unused resource can be removed).

Thank you for your contribution @teo-tsirpanis !

src/libraries/System.Private.CoreLib/src/System/IO/BinaryReader.cs

teo-tsirpanis · 2023-08-04T09:56:25Z

Feedback addressed.

adamsitnik

LGTM! Now I'll have to discuss with @jeffhandley whether we want to include this in .NET 8 or 9, will keep you updated.

Thank you again @teo-tsirpanis !

jeffhandley · 2023-08-07T17:08:49Z

I'd prefer to let this wait until .NET 9. It's a change in low-level IO dealing with serialization, with a small bit of unsafe added in, and it wasn't something we had planned for .NET 8.

teo-tsirpanis · 2023-09-23T11:09:45Z

@adamsitnik can we merge it?

EgorBo · 2023-10-19T16:47:47Z

Improvements on x64 dotnet/perf-autofiling-issues#23341

ghost added the community-contribution Indicates that the PR has been added by a community member label Jan 7, 2023

dotnet-issue-labeler bot added the area-System.IO label Jan 7, 2023

reflectronic reviewed Jan 7, 2023

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/IO/BinaryReader.cs Outdated Show resolved Hide resolved

stephentoub reviewed Jan 7, 2023

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/IO/BinaryReader.cs Outdated Show resolved Hide resolved

teo-tsirpanis force-pushed the binaryreader-opt branch from dbe91a9 to cde6822 Compare January 7, 2023 21:05

teo-tsirpanis commented Jan 7, 2023

View reviewed changes

teo-tsirpanis changed the title ~~Avoid allocating a byte array in BinaryReader in most cases.~~ Reduce allocations in BinaryReader. Jan 7, 2023

build-analysis bot mentioned this pull request Jan 8, 2023

Tracking issue for CI build timeouts #76454

Closed

adamsitnik added the tenet-performance Performance related issue label Jan 9, 2023

jeffhandley assigned adamsitnik Feb 6, 2023

adamsitnik requested changes Feb 13, 2023

View reviewed changes

ghost added the needs-author-action An issue or pull request that requires more info or actions from the author. label Feb 13, 2023

teo-tsirpanis added 10 commits February 13, 2023 21:37

Use stack-allocated memory in BinaryReader's Read*** methods.

22cbc62

Avoid allocating _buffer.

1203af6

It now gets allocated only when calling the obscure `FillBuffer` method.

Use array slicing syntax.

65979b8

Lazily allocate the Decoder.

a705ce0

Entirely remove the _buffer field in favor of array pool allocations.

92b0113

Use array slicing syntax once more.

6912f4d

Remove the _charBytes field in favor of stack allocations.

d450473

Use Unsafe.As to cast to memory streams.

48e27cc

We checked it on the constructor and already had asserts right before the casts.

Optimize GetString to directly decode the bytes into a string if it…

d6d3bc8

… is small.

Use sizeof.

f7a2043

ghost removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Feb 13, 2023

teo-tsirpanis force-pushed the binaryreader-opt branch from 6aa529d to f7a2043 Compare February 13, 2023 19:40

danmoseley reviewed Feb 13, 2023

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/IO/BinaryReader.cs Outdated Show resolved Hide resolved

adamsitnik added the needs-author-action An issue or pull request that requires more info or actions from the author. label Jul 31, 2023

ghost removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Aug 2, 2023

adamsitnik added the needs-author-action An issue or pull request that requires more info or actions from the author. label Aug 3, 2023

Merge branch 'main' into binaryreader-opt

6273201

teo-tsirpanis force-pushed the binaryreader-opt branch from 805474b to 6273201 Compare August 3, 2023 22:26

ghost removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Aug 3, 2023

adamsitnik reviewed Aug 4, 2023

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/IO/BinaryReader.cs Show resolved Hide resolved

src/libraries/System.Private.CoreLib/src/System/IO/BinaryReader.cs Outdated Show resolved Hide resolved

Address review feedback.

e212ed6

adamsitnik approved these changes Aug 4, 2023

View reviewed changes

adamsitnik added this to the 9.0.0 milestone Aug 7, 2023

jozkee closed this Oct 13, 2023

jozkee reopened this Oct 13, 2023

build-analysis bot mentioned this pull request Oct 13, 2023

[mono][tvos] OOM in System.IO.Tests.MemoryStreamTests #92467

Closed

jozkee merged commit 92907e2 into dotnet:main Oct 13, 2023
176 of 178 checks passed

teo-tsirpanis deleted the binaryreader-opt branch October 13, 2023 22:51

jozkee mentioned this pull request Oct 14, 2023

Fix regression or document breaking change in BinaryReader #93500

Closed

kotlarmilos mentioned this pull request Oct 18, 2023

[Perf] Linux/x64: 7 Improvements on 10/13/2023 10:50:45 PM dotnet/perf-autofiling-issues#23226

Closed

This was referenced Oct 19, 2023

[Perf] Windows/arm64: 1 Regression on 10/16/2023 3:26:10 AM dotnet/perf-autofiling-issues#23338

Closed

Regressions in System.IO.Tests.BinaryReaderTests #93743

Closed

This was referenced Oct 19, 2023

[Perf] Windows/arm64: 2 Improvements on 10/14/2023 8:59:45 PM dotnet/perf-autofiling-issues#23346

Closed

[Perf] Windows/arm64: 1 Improvement on 10/14/2023 8:59:45 PM dotnet/perf-autofiling-issues#23387

Closed

ghost locked as resolved and limited conversation to collaborators Nov 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduce allocations in `BinaryReader`. #80331

Reduce allocations in `BinaryReader`. #80331

teo-tsirpanis commented Jan 7, 2023 •

edited

Loading

ghost commented Jan 7, 2023

teo-tsirpanis Jan 7, 2023 •

edited

Loading

adamsitnik Feb 13, 2023

teo-tsirpanis Feb 13, 2023

adamsitnik May 19, 2023

teo-tsirpanis Aug 3, 2023

adamsitnik commented Jan 9, 2023

teo-tsirpanis commented Jan 10, 2023

jeffhandley commented Feb 6, 2023

teo-tsirpanis commented Feb 6, 2023

adamsitnik left a comment

adamsitnik commented Jul 31, 2023

teo-tsirpanis commented Aug 2, 2023

adamsitnik commented Aug 3, 2023

teo-tsirpanis commented Aug 3, 2023 •

edited

Loading

adamsitnik left a comment

teo-tsirpanis commented Aug 4, 2023

adamsitnik left a comment

jeffhandley commented Aug 7, 2023

teo-tsirpanis commented Sep 23, 2023

EgorBo commented Oct 19, 2023

	readLength = ((stringLength - currPos) > MaxCharBytesSize) ? MaxCharBytesSize : (stringLength - currPos);

	n = _stream.Read(_charBytes, 0, readLength);

Reduce allocations in BinaryReader. #80331

Reduce allocations in BinaryReader. #80331

Conversation

teo-tsirpanis commented Jan 7, 2023 • edited Loading

ghost commented Jan 7, 2023

teo-tsirpanis Jan 7, 2023 • edited Loading

Choose a reason for hiding this comment

adamsitnik Feb 13, 2023

Choose a reason for hiding this comment

teo-tsirpanis Feb 13, 2023

Choose a reason for hiding this comment

adamsitnik May 19, 2023

Choose a reason for hiding this comment

teo-tsirpanis Aug 3, 2023

Choose a reason for hiding this comment

adamsitnik commented Jan 9, 2023

teo-tsirpanis commented Jan 10, 2023

jeffhandley commented Feb 6, 2023

teo-tsirpanis commented Feb 6, 2023

adamsitnik left a comment

Choose a reason for hiding this comment

adamsitnik commented Jul 31, 2023

teo-tsirpanis commented Aug 2, 2023

adamsitnik commented Aug 3, 2023

teo-tsirpanis commented Aug 3, 2023 • edited Loading

adamsitnik left a comment

Choose a reason for hiding this comment

teo-tsirpanis commented Aug 4, 2023

adamsitnik left a comment

Choose a reason for hiding this comment

jeffhandley commented Aug 7, 2023

teo-tsirpanis commented Sep 23, 2023

EgorBo commented Oct 19, 2023

Reduce allocations in `BinaryReader`. #80331

Reduce allocations in `BinaryReader`. #80331

teo-tsirpanis commented Jan 7, 2023 •

edited

Loading

teo-tsirpanis Jan 7, 2023 •

edited

Loading

teo-tsirpanis commented Aug 3, 2023 •

edited

Loading