BinaryReader.PeekChar causes excessive reads in underlying Stream if used with BufferedStream

[lekrus]

Description

When BinaryReader.PeekChar is called multiple times for BufferedStream it will cause read of full buffer with each call.
Sample reproduce:

public class CustomStream : Stream {
	private readonly FileStream _fileStream;
	public CustomStream(FileStream fileStream) {
		_fileStream = fileStream;
	}
	public override int Read(byte[] buffer, int offset, int count) {
		Console.WriteLine("Read: count: {0}, pos: {1}, len: {2}, off: {3}", count, Position, Length, offset);
		return _fileStream.Read(buffer, offset, count);
	}
// other methods are skipped for brevity
}

static void Main(string[] args) {
	using var input = new BinaryReader(new BufferedStream(new CustomStream(File.Open("test.rtf", FileMode.Open))));
	
	for (int i = 0; i < 3; i++) {
		input.PeekChar();
	}
}

Running that code will output:

Read: count: 4096, pos: 0, len: 55355, off: 0
Read: count: 4096, pos: 0, len: 55355, off: 0
Read: count: 4096, pos: 0, len: 55355, off: 0

It shows that underlying stream is called to read full buffer (in our case 4096 is a default buffer, but with bigger buffer size it becomes even more prominent)

Analysis

From what I see, the issue occurs when PeekChar restores it's position:

long origPos = _stream.Position;
int ch = Read();
_stream.Position = origPos;

and setting Position in BufferedStream resets data in read buffer:

if (_writePos > 0)
FlushWrite();

_readPos = 0;
_readLen = 0; // <-- resetting read buffer
_stream!.Seek(value, SeekOrigin.Begin);

[ghost]

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

Issue Details

---

Description

When BinaryReader.PeekChar is called multiple times for BufferedStream it will cause read of full buffer with each call.
Sample reproduce:

public class CustomStream : Stream {
	private readonly FileStream _fileStream;
	public CustomStream(FileStream fileStream) {
		_fileStream = fileStream;
	}
	public override int Read(byte[] buffer, int offset, int count) {
		Console.WriteLine("Read: count: {0}, pos: {1}, len: {2}, off: {3}", count, Position, Length, offset);
		return _fileStream.Read(buffer, offset, count);
	}
// other methods are skipped for brevity
}

static void Main(string[] args) {
	using var input = new BinaryReader(new BufferedStream(new CustomStream(File.Open("test.rtf", FileMode.Open))));
	
	for (int i = 0; i < 3; i++) {
		input.PeekChar();
	}
}

Running that code will output:

Read: count: 4096, pos: 0, len: 55355, off: 0
Read: count: 4096, pos: 0, len: 55355, off: 0
Read: count: 4096, pos: 0, len: 55355, off: 0

It shows that underlying stream is called to read full buffer (in our case 4096 is a default buffer, but with bigger buffer size it becomes even more prominent)

Analysis

From what I see, the issue occurs when PeekChar restores it's position:

long origPos = _stream.Position;
int ch = Read();
_stream.Position = origPos;

and setting Position in BufferedStream resets data in read buffer:

if (_writePos > 0)
FlushWrite();

_readPos = 0;
_readLen = 0; // <-- resetting read buffer
_stream!.Seek(value, SeekOrigin.Begin);

Author:	lekrus
Assignees:	-
Labels:	area-System.IO, tenet-performance, untriaged
Milestone:	-

[lateapexearlyspeed]

Seems the point is to try to keep shared buffer data valid as possible even after set_position within some reasonable range. I would like to learn and try it with PR, thanks !

[lateapexearlyspeed]

Hi, this PR is ready for review now, please review, thanks.

Description:

1. Improve set_posittion: Make buffer data valid if set_posittion inside range of current buffer
2. Refactor existing implementation of Seek() so that Seek() and set_position can use new common logic (I feel new logic can cover existing one and simplify code. If not suitable, we can revert this part2)
3. Add test case which covers both set_position and Seek to verify Reading after changing position will not trigger underlying stream's Read while keep other operations correct.

[lateapexearlyspeed]

Hi @adamsitnik, could you please help review ?