Skip to content

Fix space leaks of Michael-Scott queue and avoid the impossible #64

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

Merged
merged 2 commits into from
Mar 16, 2023

Conversation

polytypic
Copy link
Contributor

@polytypic polytypic commented Mar 10, 2023

This PR fixes the space leak issue of the previous implementation (i.e. issue #63). To properly fix the leaks, it is also necessary to avoid a similar leak through the tail, which could previously also retain a reference to a node containing a value. This also means that the optimization to allow the tail to fall further behind needs more care as multiple push operations could race to update the same fallen behind tail.

One approach taken in this fix is that instead of having head and tail point to a node, they actually point directly to the atomic within a node. This also conveniently avoids the impossible pattern match cases of the previous implementation.

Another approach taken in this fix is to make sure that the tail does not fall behind. After a successful update of the tail, the new implementation verifies that the tail is still the tail and, if not, tries to update the tail again. If the tail is allowed to fall behind, then it can cause a leak as then the tail could be left pointing to nodes that have already been removed from the queue.

@polytypic polytypic linked an issue Mar 10, 2023 that may be closed by this pull request
@polytypic polytypic force-pushed the ms-queue-fix-and-tweaks branch 6 times, most recently from 3ca2639 to 38056f6 Compare March 10, 2023 10:12
@polytypic polytypic changed the title Fix space leak of Michael-Scott queue and some tweaks Fix space leak of Michael-Scott queue and avoid the impossible Mar 10, 2023
@polytypic polytypic marked this pull request as ready for review March 10, 2023 10:15
@polytypic polytypic requested review from a team and removed request for a team March 10, 2023 10:15
@polytypic polytypic marked this pull request as draft March 10, 2023 11:02
@polytypic polytypic force-pushed the ms-queue-fix-and-tweaks branch from 38056f6 to 4511b93 Compare March 10, 2023 11:08
@polytypic polytypic marked this pull request as ready for review March 10, 2023 11:08
@polytypic polytypic requested review from a team and removed request for a team March 10, 2023 11:08
@polytypic polytypic marked this pull request as draft March 10, 2023 11:13
@polytypic polytypic force-pushed the ms-queue-fix-and-tweaks branch from 4511b93 to 6aa2d66 Compare March 10, 2023 11:26
@polytypic polytypic marked this pull request as ready for review March 10, 2023 11:29
@polytypic polytypic requested a review from a team March 10, 2023 11:40
@polytypic polytypic changed the title Fix space leak of Michael-Scott queue and avoid the impossible Fix space leaks of Michael-Scott queue and avoid the impossible Mar 10, 2023
@polytypic polytypic force-pushed the ms-queue-fix-and-tweaks branch from 6aa2d66 to e148857 Compare March 10, 2023 12:42
@polytypic
Copy link
Contributor Author

polytypic commented Mar 10, 2023

BTW, it seems (unsurprisingly) that avoiding false sharing with the MS queue has a very noticeable effect on performance.

The whole idea of the the MS queue is that the head and tail are two distinct shared memory locations and the push and pop operations only access one of those. Naïvely allocating the head and tail atomics leads directly to high probability of false sharing.

Here is a benchmark run where the implementation avoids false sharing:

Hand-written Lockfree.MSQueue: mean = 0.005300, sd = 0.000000 tp=56601255.915217

Without false sharing avoidance, the timings from the same benchmark vary highly and are sometimes more than twice as high and basically never as good as above.

To be clear, this PR does not include the change to avoid false sharing:

 let create () =
   let next = Atomic.make Nil in
-  { head = Atomic.make next; tail = Atomic.make next }
+  Multicore_magic.copy_as_padded
+    {
+      head = Multicore_magic.copy_as_padded @@ Atomic.make next;
+      tail = Multicore_magic.copy_as_padded @@ Atomic.make next;
+    }

@lyrm
Copy link
Collaborator

lyrm commented Mar 15, 2023

Thanks, that is a great fix ! I just finished reviewing it and everything looks good.

I will have a look at why the CI checks are failing tomorrow.

@polytypic
Copy link
Contributor Author

I will have a look at why the CI checks are failing tomorrow.

CI says:

  •  # ocamlfind: Package `threads' not found
    

Perhaps related to this?

@polytypic polytypic force-pushed the ms-queue-fix-and-tweaks branch from af4ae53 to cf89702 Compare March 16, 2023 07:10
@lyrm lyrm merged commit 52f961e into main Mar 16, 2023
@polytypic polytypic mentioned this pull request Mar 22, 2023
@polytypic polytypic deleted the ms-queue-fix-and-tweaks branch May 4, 2023 08:18
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.

Michael_scott_queue space safety
2 participants