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

Bug fix for Multithreaded DSU #132

Merged
merged 3 commits into from
Jul 21, 2023
Merged

Bug fix for Multithreaded DSU #132

merged 3 commits into from
Jul 21, 2023

Conversation

etwest
Copy link
Collaborator

@etwest etwest commented Jul 2, 2023

This pull request fixes an edge case bug in the multithreaded DSU. Specifically the error occurs under the following conditions.

  1. Two threads find the same edge and attempt to perform a DSU merge
  2. The endpoints of the edge (x, y) have the same component size, i.e. size[x] == size[y]
  3. By chance, thread 1 makes y the parent of x and simultaneously thread 2 makes x the parent of y.

This will not cause the CAS to fail because thread 1 is operating on parent[x] while thread 2 is operating upon parent[y]. To solve this issue we make the ordering of edges deterministic and fixed. Thus, both threads orient the edge the same way if the sizes are identical. To "randomize" the roots, we switch whether the smaller or larger of x/y is first based upon if their sum is even/odd.

@etwest etwest requested a review from DanielDeLayo July 2, 2023 23:57
@Victor-C-Zhang
Copy link
Collaborator

why is this an issue? aren’t we always merging into the lexicographically lower parent in cases of ties?

@etwest
Copy link
Collaborator Author

etwest commented Jul 17, 2023

So the way that the code has worked so far is that if the sizes are the same then whichever endpoint is first (the src) then that is made the parent. We found it was better to "randomize" which endpoint was first so that the roots aren't clustered. This helps performance but also causes the issue I'm talking about in this pull request. Feels like this should just be solved on the DSU side rather than the caller having to worry about it.

@etwest etwest requested a review from Victor-C-Zhang July 19, 2023 15:56
@etwest etwest merged commit 45a878f into main Jul 21, 2023
@etwest etwest deleted the fix_dsu branch July 21, 2023 20:42
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.

3 participants