Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unify BufferedFileStreamStrategy seeking #78457

Closed
adamsitnik opened this issue Nov 16, 2022 · 1 comment · Fixed by #78984
Closed

Unify BufferedFileStreamStrategy seeking #78457

adamsitnik opened this issue Nov 16, 2022 · 1 comment · Fixed by #78984
Labels
area-System.IO good first issue Issue should be easy to implement, good for first-time contributors help wanted [up-for-grabs] Good issue for external contributors tenet-performance Performance related issue
Milestone

Comments

@adamsitnik
Copy link
Member

Today while trying to find an answer to #78390 I've realized that BufferedFileStreamStrategy has two different approaches to seeking.

Seek provides a smart way of seeking that tries to avoid losing the buffered data if possible:

// If the offset of the updated seek pointer in the buffer is still legal, then we can keep using the buffer:
if (0 <= readPos && readPos < _readLen)
{
_readPos = (int)readPos;
// Adjust the seek pointer of the underlying stream to reflect the amount of useful bytes in the read buffer:
_strategy.Seek(_readLen - _readPos, SeekOrigin.Current);
}
else
{ // The offset of the updated seek pointer is not a legal offset. Loose the buffer.
_readPos = _readLen = 0;
}

While Position setter is plain simple and always loses the buffered data:

set
{
if (_writePos > 0)
{
FlushWrite();
}
_readPos = 0;
_readLen = 0;
_strategy.Position = value;
}

Position should be "smart" too and most likely just delegate the work to Seek

@adamsitnik adamsitnik added area-System.IO good first issue Issue should be easy to implement, good for first-time contributors help wanted [up-for-grabs] Good issue for external contributors labels Nov 16, 2022
@adamsitnik adamsitnik added this to the 8.0.0 milestone Nov 16, 2022
@ghost
Copy link

ghost commented Nov 16, 2022

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

Issue Details

Today while trying to find an answer to #78390 I've realized that BufferedFileStreamStrategy has two different approaches to seeking.

Seek provides a smart way of seeking that tries to avoid losing the buffered data if possible:

// If the offset of the updated seek pointer in the buffer is still legal, then we can keep using the buffer:
if (0 <= readPos && readPos < _readLen)
{
_readPos = (int)readPos;
// Adjust the seek pointer of the underlying stream to reflect the amount of useful bytes in the read buffer:
_strategy.Seek(_readLen - _readPos, SeekOrigin.Current);
}
else
{ // The offset of the updated seek pointer is not a legal offset. Loose the buffer.
_readPos = _readLen = 0;
}

While Position setter is plain simple and always loses the buffered data:

set
{
if (_writePos > 0)
{
FlushWrite();
}
_readPos = 0;
_readLen = 0;
_strategy.Position = value;
}

Position should be "smart" too and most likely just delegate the work to Seek

Author: adamsitnik
Assignees: -
Labels:

area-System.IO, good first issue, help wanted

Milestone: 8.0.0

@adamsitnik adamsitnik added the tenet-performance Performance related issue label Nov 16, 2022
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Nov 29, 2022
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Dec 23, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Jan 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.IO good first issue Issue should be easy to implement, good for first-time contributors help wanted [up-for-grabs] Good issue for external contributors tenet-performance Performance related issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant