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

Lock free Read/Write Identical Request Implementation #124

Merged

Conversation

deanmarcussen
Copy link
Collaborator

Prerequisites

  • I have written a descriptive pull-request title
  • I have verified that there are no overlapping pull-requests open
  • I have verified that I am following matches the existing coding patterns and practice as demonstrated in the repository. These follow strict Stylecop rules 👮.
  • I have provided test coverage for my change (where applicable)

Description

Proposes replacing the AsyncKeyLock with an implementation based around ConcurrentDictionary.

Note: I have not removed the AsyncKeyLock code or related test code.

Test coverage from the Integration Tests.

Copy link
Member

@JimBobSquarePants JimBobSquarePants left a comment

Choose a reason for hiding this comment

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

Awesome, thanks!

@JimBobSquarePants JimBobSquarePants merged commit 434b149 into SixLabors:js/faster-cache Sep 17, 2020
@cristipufu
Copy link

@deanmarcussen @JimBobSquarePants can you guys please provide more details about this change? How is it better (pros/cons) vs the previous implementation with the AsyncLock?

@JimBobSquarePants
Copy link
Member

It’s about 30x faster as I recall from the load test benchmarks

@sebastienros
Copy link
Contributor

To be precise, under stress it went from 2K RPS to 60K RPS, which was the static file middleware raw perf.

@deanmarcussen
Copy link
Collaborator Author

deanmarcussen commented Dec 13, 2020

Worth having a read of the full pr from @JimBobSquarePants #121 where the bottlenecks in the existing lock where identified and we tested the lock free approach

@cristipufu
Copy link

Thanks a lot guys!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants