Skip to content

Pipelines remove lock from uncontended path #36956

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.

Sign up for GitHub

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

Jump to bottom
Closed
benaadams wants to merge 1 commit into from
Closed

Pipelines remove lock from uncontended path #36956

benaadams wants to merge 1 commit into from

Conversation

@benaadams
Copy link
Member

@benaadams benaadams commented May 24, 2020 •
edited
Loading

Simpler version of #36955

Drops the lock for common path through the highlighted code paths for Json TE:

image

Before

image

After

image

Should follow up with more state out to the PipeOperationState to avoid cache thrashing, but should be easier to review.

/cc @halter73 @adamsitnik @stephentoub @davidfowl

@benaadams benaadams force-pushed the pipelines-locks-partial branch from b81a2e6 to 85e0b95 Compare May 24, 2020 18:09
@benaadams benaadams changed the title Pipelines remove lock from uncontended path (partial) Pipelines remove lock from uncontended path May 24, 2020
@benaadams benaadams marked this pull request as ready for review May 24, 2020 19:43
@benaadams benaadams force-pushed the pipelines-locks-partial branch 2 times, most recently from 04b1ee2 to b28a42a Compare May 24, 2020 21:41
@benaadams
Copy link
Member Author

benaadams commented May 24, 2020

System.IO.Pipelines.zip

@benaadams benaadams force-pushed the pipelines-locks-partial branch 2 times, most recently from 7f1d835 to 9296864 Compare May 25, 2020 03:32
@adamsitnik
Copy link
Member

adamsitnik commented May 25, 2020

obraz

@benaadams
Copy link
Member Author

benaadams commented May 25, 2020

Interesting, did you managed to get a trace? Json would be interesting to see how things shifted around

@benaadams
Copy link
Member Author

benaadams commented May 25, 2020

Ah, let me add cache seperation

@jaredpar jaredpar mentioned this pull request May 25, 2020
Closed
@adamsitnik
Copy link
Member

adamsitnik commented May 25, 2020

Interesting, did you managed to get a trace?

@benaadams I've just uploaded PlatformJson trace to traces/36956_pipes_locks

@benaadams
Copy link
Member Author

benaadams commented May 25, 2020

Does look to behave as expected

Before

image

After (still lock taken in GetMemory, so some contention there)

image

@benaadams benaadams mentioned this pull request May 25, 2020
Closed
@benaadams
Copy link
Member Author

benaadams commented May 25, 2020

The GetMemory() showing up as still taking the lock (although much less) means there is contention on pipe between reader and writer which shouldn't happen here for Json.

It looks like _waitForData == false so the FlushAsync() happens triggering the reader and then the writer immediately calls GetMemory() causing contention; the fix is to call GetMemory() prior to calling FlushAsync() in this scenario dotnet/aspnetcore#22201

@davidfowl
Copy link
Member

davidfowl commented May 25, 2020

@adamsitnik can you collect counters before and after the next run. I want to make sure we’re looking at the right metrics

@benaadams benaadams force-pushed the pipelines-locks-partial branch from cbf8fa0 to ae6983e Compare May 25, 2020 21:54
@benaadams
Copy link
Member Author

benaadams commented May 25, 2020

Merged the commits so can try something more easily

@benaadams benaadams mentioned this pull request May 25, 2020
Closed
@adamsitnik
Copy link
Member

adamsitnik commented May 26, 2020

can you collect counters before and after the next run

@davidfowl I've re-run the benchmarks against latest changes and included the Lock Contention (#/s) metric.

Machine Benchmark RPS before RPS after locks/s before locks/s after
Citrine 28 cores PlaintextPlatform 9,199,114 9,379,908 401 505
  JsonPlatform 1,202,437 1,185,605 20 32
  FortunesPlatform 313,169 315,329 81 65
  Fortunes Batching 425,197 414,649 651 631
           
           
Perf 12 cores PlaintextPlatform 5,997,033 5,940,848 114 98
  JsonPlatform 605,564 607,849 13 10
  FortunesPlatform 130,403 129,567 65 68
           
           
ARM 32 cores PlaintextPlatform 6,132,864 6,082,789 330 682
  JsonPlatform 566,019 578,091 310 152
  FortunesPlatform 96,260 91,606 19 29
           
           
Mono 56 cores PlaintextPlatform 6,945,170 6,947,873 5,891 5,883
  JsonPlatform 1,113,376 1,149,895 995 666
           
           
AMD 46 cores JsonPlatform 7,607,998 7,627,253 531 498
  JsonPlatform 719,018 675,684 68 61
  FortunesPlatform 260,262 258,619 18 20

the lock contention on Mono machine could most probably lead us to answer why it's not faster than Citrine (cc @stephentoub)

@davidfowl
Copy link
Member

davidfowl commented May 26, 2020

@adamsitnik are we comparing a fixed number of requests using bombardier? Or are we using wrk?

@adamsitnik
Copy link
Member

adamsitnik commented May 26, 2020

are we comparing a fixed number of requests using bombardier? Or are we using wrk?

@davidfowl I am not sure whether bombardier or wrk are used, I am using the following arguments for BenchmarksDriver

--connections 512 --self-contained --sdk 5.0.100-preview.6.20272.2 --runtime  5.0.0-preview.6.20272.2 --aspnetcoreversion 5.0.0-preview.5.20255.6

@benaadams benaadams mentioned this pull request May 26, 2020
Closed
@benaadams
Copy link
Member Author

benaadams commented May 26, 2020

the lock contention on Mono machine could most probably lead us to answer why it's not faster than Citrine

Lock contention is reported after any spinning is been done and when it's going to sleep; rather than just raw contention that is resolved after spinning: #37023 (comment)

@halter73
Copy link
Member

halter73 commented May 26, 2020

If you're using version 1 of the benchmark driver and you haven't specified a --job or --client-name, you're using wrk.

@benaadams benaadams marked this pull request as draft August 1, 2020 21:24
@benaadams
Copy link
Member Author

benaadams commented Aug 2, 2020

Revist post 5.0

@benaadams benaadams closed this Aug 2, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

Copilot code review Copilot Awaiting requested review from Copilot Copilot will automatically review once the pull request is marked ready for review

Assignees

No one assigned

Labels

area-System.IO.Pipelines

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@benaadams @adamsitnik @davidfowl @halter73 @Dotnet-GitSync-Bot