Skip to content

Do not flush serializer buffers before first IAsyncEnumerable element is fetched - #59808

Merged
eiriktsarpalis merged 3 commits into
dotnet:mainfrom
eiriktsarpalis:stj-asyncenumerable-noflush
Oct 1, 2021
Merged

Do not flush serializer buffers before first IAsyncEnumerable element is fetched#59808
eiriktsarpalis merged 3 commits into
dotnet:mainfrom
eiriktsarpalis:stj-asyncenumerable-noflush

Conversation

@eiriktsarpalis

@eiriktsarpaliseiriktsarpalis commented Sep 30, 2021•
edited
Loading

Copy link
Copy Markdown
Member

Addresses dotnet/aspnetcore#36977

See dotnet/aspnetcore#36977 (comment) for an explanation of the issue.

I ran the benchmark suite against the changes and no performance regression was recorded.

Should consider servicing in 6.0.

@eiriktsarpaliseiriktsarpalis added this to the 7.0.0 milestone Sep 30, 2021
@eiriktsarpaliseiriktsarpalis self-assigned this Sep 30, 2021
@ghost

ghost commented Sep 30, 2021

Copy link
Copy Markdown

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

Issue Details

Addresses dotnet/aspnetcore#36977

See dotnet/aspnetcore#36977 (comment) for an explanation of the issue.

Should consider servicing in 6.0

Author:eiriktsarpalis
Assignees:eiriktsarpalis
Labels:

area-System.Text.Json

Milestone:7.0.0

@eiriktsarpalis

eiriktsarpalis commented Sep 30, 2021

Copy link
Copy Markdown
MemberAuthor

cc @pranavkm@davidfowl

@pranavkm

pranavkm commented Sep 30, 2021

Copy link
Copy Markdown
Contributor

Nice. FYI @BrennanConroy since this would resolve the IAsyncEnumerable issue.

steveharter
steveharter approved these changes Oct 1, 2021
View reviewed changes

@stevehartersteveharter left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The strategy of deferring the first flush is sound; a couple questions around Asserts

@runfoapprunfoappBot mentioned this pull request Oct 1, 2021
Open
@ericstj

ericstj commented Oct 1, 2021

Copy link
Copy Markdown
Member

Libraries Test Run release coreclr windows x64 Debug failure is #59627

@eiriktsarpalis
eiriktsarpalis merged commit 0bd91fa into dotnet:mainOct 1, 2021
@eiriktsarpalis

eiriktsarpalis commented Oct 1, 2021

Copy link
Copy Markdown
MemberAuthor

/backport to release/6.0

@eiriktsarpalis
eiriktsarpalis deleted the stj-asyncenumerable-noflush branch October 1, 2021 15:19
@github-actions

github-actionsBot commented Oct 1, 2021

Copy link
Copy Markdown
Contributor

Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/1295615263

@github-actionsgithub-actionsBot mentioned this pull request Oct 1, 2021
Merged
@ghostghost locked as resolved and limited conversation to collaborators Nov 3, 2021
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@krwqkrwqkrwq left review comments
@halter73halter73halter73 approved these changes
@ericstjericstjericstj approved these changes
@layomialayomiaAwaiting requested review from layomia
+1 more reviewer
@steveharterstevehartersteveharter approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@eiriktsarpaliseiriktsarpalis

Labels

area-System.Text.Json

Projects

None yet

Milestone

7.0.0

Development

Successfully merging this pull request may close these issues.

6 participants

@eiriktsarpalis@pranavkm@ericstj@halter73@krwq@steveharter