Use nonfair locks #938
Open
Use nonfair locks #938
+4
−4
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fair locks are fair, which mean,s that they are locked in the order threads called lock() and not in pseudo random order, but they are slow. Like very slow. Invoking The Random Guy On The Internet Who Did Some Benchmarks, we can see that fair locks are like a hundred time slower [blog here](https://alidg.me/blog/2020/3/7/scalable-fair-lock#barging--fairness-cost). We had a production issue with a lot of threads waiting to lock these locks. To go a bit deeper, the semantic of what is done under those locks looks a lot like a Map.computeIfAbsent(), and the code could gain clarity and probably performance by using a ConcurrentHashMap. As the attribute names are a char[] with offset and length and not a String, there would need to be a smart wrapper for the key. That means an light object allocation for each access, I don't think that's a big deal. Another alternative would be to keep the pair of Lists, but instead of a big lock around both, we could store them in a AtomicReference (to be precise: one AtomicRef storing some ligh object containing the two Lists). On access, the methods would load the two lists without the lock and when update is needed, they could update the Lists optimistically, try to swap the AtomicRef's content, and retry the whole update cycle on failure. WDYT?
|
any feedback? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fair locks are fair, which mean,s that they are locked in the order threads called lock() and not in pseudo random order, but they are slow. Like very slow.
Invoking The Random Guy On The Internet Who Did Some Benchmarks, we can see that fair locks are like a hundred time slower blog here.
We had a production issue with a lot of threads waiting to lock these locks.
To go a bit deeper, the semantic of what is done under those locks looks a lot like a Map.computeIfAbsent(), and the code could gain clarity and probably performance by using a ConcurrentHashMap. As the attribute names are a char[] with offset and length and not a String, there would need to be a smart wrapper for the key. That means an light object allocation for each access, I don't think that's a big deal.
Another alternative would be to keep the pair of Lists, but instead of a big lock around both, we could store them in a AtomicReference (to be precise: one AtomicRef storing some ligh object containing the two Lists). On access, the methods would load the two lists without the lock and when update is needed, they could update the Lists optimistically, try to swap the AtomicRef's content, and retry the whole update cycle on failure.
WDYT?