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

Consider using native std::sync primitives instead of parking_lot #126

Closed
Tpt opened this issue Sep 22, 2022 · 4 comments
Closed

Consider using native std::sync primitives instead of parking_lot #126

Tpt opened this issue Sep 22, 2022 · 4 comments
Labels
enhancement New feature or request

Comments

@Tpt
Copy link
Contributor

Tpt commented Sep 22, 2022

Now that Mutex is implemented in std without boxing when futex is available, it might make sense to use them instead of parking_lot. There are some benchmarks hinting about better std performances.

It might also make testing using e.g. loom easier.

Is it worth a trial and a benchmark?

@arkpar
Copy link
Member

arkpar commented Sep 22, 2022

Sure. We can use the stress tool to benchmark this.

@Tpt Tpt added the enhancement New feature or request label Sep 22, 2022
@Tpt
Copy link
Contributor Author

Tpt commented Sep 26, 2022

I have tried to migrate to std::sync naively. Mixed read/write workloads see a huge performance drop because of the absence of "upgradable locks" in std::sync. So it's might still be doable but will require quite a bit of work.

@Tpt
Copy link
Contributor Author

Tpt commented Sep 28, 2022

I have made some improvements by replacing upgreadable lock by a read lock that is then released into a write lock. I have not checked yet if this translation is always valid. The code is here: f72ff20

Sadly there is still some performance regression. With time cargo run --release -- stress --writers 10 --seed 1 --readers 10 I get with parking_lot:

Completed 100000 commits in 170.020986395 seconds. 588.1626857973623 cps. 75265591 hits, 334927146 misses, 0 iterations, 2412600.618884911 qps
Completed 10000000 queries in 9.287924863 seconds. 1076666.7632978675 qps

And with std::sync::

Completed 100000 commits in 181.039704137 seconds. 552.36502112446 cps. 72209028 hits, 330636690 misses, 0 iterations, 2225178.8353296826 
Completed 10000000 queries in 10.081445958 seconds. 991921.2027382471 qps

@arkpar
Copy link
Member

arkpar commented Sep 28, 2022

I have made some improvements by replacing upgreadable lock by a read lock that is then released into a write lock. I have not checked yet if this translation is always valid.

In general it is not. As soon as the lock is released other waiting thread may gain access and do modifications. Upgradable lock guarantees that the upgrade is done with no intermediate modification. Upgradable locks are also exclusive wrt other upgradable locks, but not read locks. Even though with the way thing setup in paritydb this could be fine (e.g. we only expect enact_plan to be called from a single thread), I'd still prefer not to introduce these subtle breaking points.

It seems we're stuck with parking_lot for now.

@arkpar arkpar closed this as completed Sep 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants