ICU-23061: LowercaseTransliterator has poor scalability due to synchronized state #3417
+90
−7
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
stevenschlansker
force-pushed
the
translit-lower-scalability-jmh
branch
from
95a0533 to
2af99d6
Compare
|
Notice: the branch changed across the force-push!
~ Your Friendly Jira-GitHub PR Checker Bot |
stevenschlansker
force-pushed
the
translit-lower-scalability-jmh
branch
from
2af99d6 to
2197adb
Compare
|
Notice: the branch changed across the force-push!
~ Your Friendly Jira-GitHub PR Checker Bot |
stevenschlansker
changed the title
…nitor contention Current implementation synchronizes the whole lowercase operation, so it is always single-threaded Removing the shared state lets us utilize multiple CPU cores
JMH is the OpenJDK microbenchmark harness. It is a standardized tool with many integrations and provides a lot of out-of-the-box functionality. No existing perf-tests are changed https://github.com/openjdk/jmh
stevenschlansker
force-pushed
the
translit-lower-scalability-jmh
branch
from
2197adb to
f332c3c
Compare
|
Hooray! The files in the branch are the same across the force-push. 😃 ~ Your Friendly Jira-GitHub PR Checker Bot |
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.
LowercaseTransliterator tries to reuse buffers to avoid allocation. Historically, this may have been an important optimization.
However on modern machines you are much more likely to use multiple CPUs, and modern JVMs are very efficient at GC, especially for local temporary data. Contending on a monitor is a big deal nowadays, especially for something so low level and hidden.
I've prepared a benchmark using JMH to show the change in scalability:
The current implementation exhibits negative scalability: adding more threads hurts throughput (~5% loss going from 1T to 8T)
By removing the
synchronizedstate, the implementation now scales perfectly (~8x increase going from 1T to 8T for the longer test case)There is a very minor (~1%) penalty for pure single-threaded throughput since we no longer reuse state.
I believe this is acceptable in the context of allowing multiple threads to work without coordination.
Checklist