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

[master <] Introduce a reader writer spin lock #1187

Merged
merged 2 commits into from Sep 1, 2023
Merged

Conversation

Ignition
Copy link
Contributor

@Ignition Ignition commented Aug 22, 2023

There are situations where multiple readonly queries in parallel are contended on the same vertex lock. Take for example a highly connected supernode which is repetitively used in result values and it needs encode it to a bolt value for sending to the client.

Since SpinLock can only be used exclusively, it causes unnecessary blocking. RWSpinLock would allow shared reading.

Before: you can see heavy use of __pthread_spin_lock
before

After: in the same part of the profile the locks don't even appear, this is because of the RWSpinLock
after

[master < Task] PR

  • Check, and update documentation if necessary
  • Provide the full content or a guide for the final git message

To keep docs changelog up to date, one more thing to do:

  • Write a release note here, including added/changed clauses --> "Performance improvement for concurrent operations which are contending on the same vertex"
  • Tag someone from docs team in the comments

closes #1189

@Ignition Ignition self-assigned this Aug 22, 2023
@Ignition Ignition marked this pull request as ready for review August 23, 2023 09:41
Copy link
Contributor

@as51340 as51340 left a comment

Choose a reason for hiding this comment

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

Great job, I like it very much! Only thing which I don't like is the usage of [[likely]]. I think it is impossible to predict the workload and hence we could actually lose performance by using it.

src/utils/rw_spin_lock.hpp Outdated Show resolved Hide resolved
src/utils/rw_spin_lock.hpp Outdated Show resolved Hide resolved
src/utils/rw_spin_lock.hpp Show resolved Hide resolved
src/utils/rw_spin_lock.hpp Outdated Show resolved Hide resolved
src/utils/rw_spin_lock.hpp Outdated Show resolved Hide resolved
src/utils/rw_spin_lock.hpp Outdated Show resolved Hide resolved
src/storage/v2/disk/storage.cpp Outdated Show resolved Hide resolved
@as51340
Copy link
Contributor

as51340 commented Aug 23, 2023

@Josipmrden I used this to play a bit with it to understand exactly what is going on. Maybe you will find it useful, if not ignore

src/utils/rw_spin_lock.hpp Show resolved Hide resolved
src/utils/rw_spin_lock.hpp Show resolved Hide resolved
@Ignition Ignition force-pushed the MG_rw_spin_lock branch 2 times, most recently from 70c64aa to 3bd76ff Compare August 31, 2023 08:53
@gitbuda gitbuda added this to the mg-v2.11.0 milestone Aug 31, 2023
@gitbuda gitbuda added benchmarking mgBench and benchmarking Memgraph feature feature labels Aug 31, 2023
It is possible for multiple read only queries to be accessing the same
squence of vertices/edges. The reader mode of the spin lock will ensure
multiple threads can make progress at the same time.
@Ignition Ignition dismissed as51340’s stale review September 1, 2023 13:20

AFAICT this is resolved

@Ignition Ignition merged commit 9661c52 into master Sep 1, 2023
6 checks passed
@Ignition Ignition deleted the MG_rw_spin_lock branch September 1, 2023 13:21
@Ignition Ignition added the Docs - changelog only Docs - changelog only label Sep 12, 2023
as51340 pushed a commit that referenced this pull request Nov 10, 2023
It is possible for multiple read only queries to be accessing the same
sequence of vertices/edges. The reader mode of the spin lock will ensure
multiple threads can make progress at the same time.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
benchmarking mgBench and benchmarking Memgraph Docs - changelog only Docs - changelog only feature feature
Projects
None yet
4 participants