-
Notifications
You must be signed in to change notification settings - Fork 4.5k
Optimize locking in several critical-path methods #22162
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
Conversation
|
Run Java PreCommit |
3 similar comments
|
Run Java PreCommit |
|
Run Java PreCommit |
|
Run Java PreCommit |
|
The Pulsar tests are consistently failing again, seems unrelated to this change: |
lukecwik
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be great to add a concurrency test to ensure that things like toString/populateDisplayData/serialize all work when as() is modifying computed properties.
Not sure how easy it will be since you'll need to generate classes at runtime to invoke .as() to.
| // exception in writeObject() | ||
| @SuppressFBWarnings("SE_BAD_FIELD") | ||
| private final Map<String, BoundValue> options; | ||
| private final ConcurrentHashMap<String, BoundValue> options; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| private final ConcurrentHashMap<String, BoundValue> options; | |
| /** | |
| * Enumerating {@code options} must always be done on a copy made before accessing or deriving properties from {@code computedProperties} since concurrent hash maps are <a href="https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/package-summary.html#Weakly>weakly consistent</a>. This will allow us to ensure that the keys in {code options} will always be a subset of properties stored in {code computedProperties}. | |
| */ | |
| private final ConcurrentHashMap<String, BoundValue> options; |
sdks/java/core/src/main/java/org/apache/beam/sdk/options/ProxyInvocationHandler.java
Show resolved
Hide resolved
sdks/java/core/src/main/java/org/apache/beam/sdk/options/ProxyInvocationHandler.java
Outdated
Show resolved
Hide resolved
sdks/java/core/src/main/java/org/apache/beam/sdk/options/ProxyInvocationHandler.java
Show resolved
Hide resolved
...a/core/src/main/java/org/apache/beam/sdk/transforms/reflect/ByteBuddyDoFnInvokerFactory.java
Show resolved
Hide resolved
sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/reflect/DoFnSignatures.java
Show resolved
Hide resolved
sdks/java/core/src/main/java/org/apache/beam/sdk/options/ProxyInvocationHandler.java
Show resolved
Hide resolved
Co-authored-by: Lukasz Cwik <lcwik@google.com>
Hm, yeah what did you have in mind here? I was struggling to think of anything useful to test. |
generate new classes in a loop that add a new getter/setter and call .as() and then set the value to a new property. At the same time have another thread in a loop continuously calling toString/populateDisplayData/serialize ensuring that we don't throw an exception. |
Cool, done |
sdks/java/core/src/test/java/org/apache/beam/sdk/options/ProxyInvocationHandlerTest.java
Outdated
Show resolved
Hide resolved
sdks/java/core/src/test/java/org/apache/beam/sdk/options/ProxyInvocationHandlerTest.java
Outdated
Show resolved
Hide resolved
sdks/java/core/src/test/java/org/apache/beam/sdk/options/ProxyInvocationHandlerTest.java
Outdated
Show resolved
Hide resolved
|
Run Java PreCommit |
3 similar comments
|
Run Java PreCommit |
|
Run Java PreCommit |
|
Run Java PreCommit |
* Optimize locking in several critical-path methods * spotbugs * Apply suggestions from code review Co-authored-by: Lukasz Cwik <lcwik@google.com> * review comments * unit test for concurrency * the buddy of bytes Co-authored-by: Lukasz Cwik <lcwik@google.com>
This fixes
ProxyInvocationHandler.as,DoFnSignatures.getSignature, andByteBuddyDoFnInvokerFactory.getByteBuddyInvokerConstructorfrom #22161The fixes to DoFnSignatures and ByteBuddyDoFnInvokerFactory are trivially changing a Map to ConcurrentHashMap.
ProxyInvocationHandler is more involved, since multiple data structures must be updated atomically on a cache miss. To do this we extract the relevant structures into an immutable class. On a cache miss we construct a new instance of it with new (also immutable) copies of the updated structures. The trade-off here is slightly more garbage creation on a cache miss (since it needs to copy the structures rather than mutate them), however since in steady-state misses ~never occur, and the structures here in question are generally small (~100s of elements).
The impact of these changes is very significant, without these changes my test job spends ~30% of its time in the "start bundle" state, after these changes (as well as ExecutionStateSampler), the % time goes down to ~1-2%.
R: @lukecwik
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
R: @username).addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, commentfixes #<ISSUE NUMBER>instead.CHANGES.mdwith noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI.