Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Improve set_position to reuse buffer.#54991

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation

@lateapexearlyspeed
Copy link
Contributor

Fix#54593.
Main purpose is to try to keep buffer valid as possible so that buffer data can still be used after set_position within range [beginBufferPosMapToUnderlyingStream, underlyingStream.Position)

@ghostghost added the area-System.IO labelJul 1, 2021
@ghost
Copy link

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

Issue Details

Fix#54593.
Main purpose is to try to keep buffer valid as possible so that buffer data can still be used after set_position within range [beginBufferPosMapToUnderlyingStream, underlyingStream.Position)

Author:lateapexearlyspeed
Assignees:-
Labels:

area-System.IO

Milestone:-

@lateapexearlyspeed
Copy link
ContributorAuthor

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.

@lateapexearlyspeedlateapexearlyspeed marked this pull request as ready for reviewJuly 14, 2021 11:09
@terrajobstterrajobst added the community-contributionIndicates that the PR has been added by a community member labelJul 19, 2021
@lateapexearlyspeedlateapexearlyspeed changed the titleInitial commit: improve set_position to reuse buffer.Improve set_position to reuse buffer.Jul 23, 2021
@jeffhandley
Copy link
Member

@carlossanlop This PR is assigned to you for follow-up/decision before the RC1 snap.

@jeffhandley
Copy link
Member

I'm going to merge from main, resolve the conflicts, and push to this PR's branch...

@lateapexearlyspeed
Copy link
ContributorAuthor

Okay and thanks for resolving conflict, this PR was pending long time :)

Copy link
Member

@jeffhandleyjeffhandley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

LGTM; I'll push in the (preexisting) typo fix. I'd like another reviewer to take a look before we merge it though.

@jeffhandley
Copy link
Member

The test failure is unrelated (as noted by@karelz). We'll just wait for an approval from@adamsitnik,@carlossanlop, or@stephentoub whenever one of them is available. The target is merging this by August 15th. Thanks for your patience,@lateapexearlyspeed.

lateapexearlyspeed reacted with heart emoji

Comment on lines 1227 to 1233
// If the seek destination is still within the data currently in the buffer, we want to keep the buffer data and continue using it.
// Otherwise we will throw away the buffer. This can only happen on read, as we flushed write data above.

// The offset of the new/updated seek pointer within _buffer:
_readPos=(int)(newPos-(oldPos-_readPos));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I was not aware of the fact thatSeek was capable of reusing the buffer whilePosition setter was not. Since we are very close to release of .NET 6 and I don't feel comfortable introducing any non trivial changes to the buffering logic (I am speaking from experience, as I've blocked preview 4 release with a bug in buffering logic myself:#51151) I would prefer to reuse the existingSeek logic by simply callingSeek fromPosition setter.

@lateapexearlyspeed please don't get me wrong. I am not saying that your refactor and logic changes have a bug, I just don't feel 100% comfortable merging it now.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I would prefer to reuse the existing Seek logic by simply calling Seek from Position setter

set_Position already calls Seek; it just first zeros out some state... it's not clear to me why it does that, but it would seem the right answer here would just be to delete code from the setter:

            set            {                if (value < 0)                    ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.value, ExceptionResource.ArgumentOutOfRange_NeedNonNegNum);-                EnsureNotClosed();-                EnsureCanSeek();--                if (_writePos > 0)-                    FlushWrite();--                _readPos = 0;-                _readLen = 0;                _stream!.Seek(value, SeekOrigin.Begin);            }

Is that sufficient to address the issue?

Copy link
Member

@adamsitnikadamsitnikAug 10, 2021•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

set_Position already calls Seek

it calls_stream!.Seek, while it should callthis.Seek(value, SeekOrigin.Begin) which contains the logic that we care about

// If the seek destination is still within the data currently in the buffer, we want to keep the buffer data and continue using it.
// Otherwise we will throw away the buffer. This can only happen on read, as we flushed write data above.
// The offset of the new/updated seek pointer within _buffer:
_readPos=(int)(newPos-(oldPos-_readPos));
// 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)
{
// Adjust the seek pointer of the underlying stream to reflect the amount of useful bytes in the read buffer:
_stream.Seek(_readLen-_readPos,SeekOrigin.Current);
}
else
{// The offset of the updated seek pointer is not a legal offset. Loose the buffer.
_readPos=_readLen=0;
}

Copy link
Member

@stephentoubstephentoubAug 10, 2021•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

it calls _stream!.Seek, while it should call this.Seek(value, SeekOrigin.Begin) which contains the logic that we care about

Ah, I missed the_stream. part. Then yeah, that last line should change as well.

            set            {                if (value < 0)                    ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.value, ExceptionResource.ArgumentOutOfRange_NeedNonNegNum);-                EnsureNotClosed();-                EnsureCanSeek();--                if (_writePos > 0)-                    FlushWrite();--                _readPos = 0;-                _readLen = 0;-                _stream!.Seek(value, SeekOrigin.Begin);+                Seek(value, SeekOrigin.Begin);            }

adamsitnik reacted with thumbs up emoji
@adamsitnikadamsitnik added this to the6.0.0 milestoneAug 10, 2021
Copy link
Member

@adamsitnikadamsitnik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I've applied the suggestion I had in order to merge it before we snap for RC1. Once again thank you@lateapexearlyspeed !

@ghost
Copy link

Hello@adamsitnik!

Because this pull request has theauto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn morehere.

@adamsitnikadamsitnik merged commitca33c4d intodotnet:mainAug 11, 2021
@ghostghost locked asresolvedand limited conversation to collaboratorsSep 10, 2021
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.

Reviewers

@stephentoubstephentoubstephentoub left review comments

@jeffhandleyjeffhandleyjeffhandley approved these changes

@adamsitnikadamsitnikadamsitnik approved these changes

@carlossanlopcarlossanlopAwaiting requested review from carlossanlop

Assignees

@carlossanlopcarlossanlop

Labels

area-System.IOcommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Milestone

6.0.0

Development

Successfully merging this pull request may close these issues.

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

6 participants

@lateapexearlyspeed@jeffhandley@stephentoub@adamsitnik@carlossanlop@terrajobst
[8]ページ先頭
©2009-2025 Movatter.jp