-
Notifications
You must be signed in to change notification settings - Fork 48
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
inheritance based padding #65
Conversation
I still didn't work out all the details, but after reading more into chapter 10 of Aleksey Shipilёvs article, there is much more in this topic. |
Thank you for the detailed write up and explanation. At a minimum for this change to be accepted you will need to replace bytes in both locations that exhibit this pattern. I would recommend using 128 bytes also rather than 64 to avoid cache prefetch. Finally be sure both locations include the 'sumToAvoidOptimization' trick so that the bytes do not get optimized out. I note that in my version of AbstractWaitingCondition this is omitted. |
Done. I noticed this pattern in 4 locations in total. I assume this change woule be beneficial for all of them?
Done.
Done. I omitted Thank you for your comment! I will follow up with a brief explanation of my understanding of this inheritance based field padding. Still no performance testing done from my side. |
Here is my current understanding of the topic. Basically all of my information is from this article by Aleksey Shipilёv which I recommend reading. It turns out that in general the actual field layout is not the same as the declaration order see §10.1, even before JDK15. This means the "c-style" padding strategy for preventing false sharing is in general not reliable in Java see §10.2. I tried out jol-cli for printing out the field layout of the relevant classes in conversant disruptor. currently workingAbstractWaitingCondition (master, JDK22)temurin-22.0.2/bin/java -jar jol-cli-latest.jar internals -cp disruptor-1.2.22-SNAPSHOT.jar com.conversantmedia.util.concurrent.AbstractWaitingCondition
Here is the current layout of currently brokenMPMCConcurrentQueue$Cell (master, JDK22)temurin-22.0.2/bin/java -jar jol-cli-latest.jar internals -cp disruptor-1.2.22-SNAPSHOT.jar com.conversantmedia.util.concurrent.MPMCConcurrentQueue\$Cell
Looking at broken exampleAs an experiment, I hacked AbstractWaitingCondition (hacked int waitCache, JDK22)temurin-22.0.2/bin/java -jar jol-cli-latest.jar internals -cp disruptor-1.2.22-SNAPSHOT.jar com.conversantmedia.util.concurrent.AbstractWaitingCondition
Here we can see that the layout was reordered and broken fixThe same happened in my first more or less blind "fix" in 453b8eb which evidently actually broke the layout of AbstractWaitingCondition (453b8eb, JDK22)temurin-22.0.2/bin/java -jar jol-cli-latest.jar internals -cp disruptor-1.2.22-SNAPSHOT.jar com.conversantmedia.util.concurrent.AbstractWaitingCondition
current draft PR, workingAs a workaround we can use inheritance to construct the desired field layout see §11.3. Here we abuse an implementation detail where fields are ordered by the inheritance of the classes. However, now we reach the issue since JDK15 where fields can be reordered "across classes" see first half of §11.5. To workaround this new issue padding should always be done with Explanation in the linked JMH Sample:
Using inheritance for padding and using bytes has the following results: MPMCConcurrentQueue$Cell (current PR, JDK22)temurin-22.0.2/bin/java -jar jol-cli-latest.jar internals -cp disruptor-1.2.22-SNAPSHOT.jar com.conversantmedia.util.concurrent.MPMCConcurrentQueue\$Cell
current PR working-ishAbstractWaitingCondition (current PR, JDK22)temurin-22.0.2/bin/java -jar jol-cli-latest.jar internals -cp disruptor-1.2.22-SNAPSHOT.jar com.conversantmedia.util.concurrent.AbstractWaitingCondition
The new
The only remaining quirk is that I assume this is because of the following mentioned issue in the JMH comment, which I still have to investigate.
``PushPullConcurrentQueue |
@lokok8 thank you for the fantastic and detailed write up this is super helpful.
I'd say those changes are good enough to change this to a regular pr and we can do a final round of benchmarking and certification and it would be good to merge. |
src/main/java/com/conversantmedia/util/concurrent/AbstractWaitingCondition.java
Outdated
Show resolved
Hide resolved
src/main/java/com/conversantmedia/util/concurrent/MPMCConcurrentQueue.java
Outdated
Show resolved
Hide resolved
Thank you for your feedback, I'll get to it sometime in the early part of next week. |
e225c1d
to
5e4c9d3
Compare
@jac18281828, I hope you don't mind me force pushing the changes. I removed the field padding in MPMCConcurrentQueue$Cell and also in AbstractWaitingCondition. If I understood your second bullet point correctly, you are talking about AbstractWaitingCondition specifically? Please let me know if that is not the case. sumToAvoidOptimization() is now formatted to be single column - I agree that it works better. Unfortunately all of these padding variables are quite unwieldy 😄 . |
- using bytes instead of longs - 128 bytes padding
- using bytes instead of longs - 128 bytes padding
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.
OK, this looks good. Thank you for all your help. I will test and merge today if it passes.
🎉🔥
I did not perform a super scientific benchmark but I did sanity check. I'd say the results are neutral to positive. In particular I notice a nice performance improvement in the highly contended multi thread case. I believe the changes in the single thread performance are within the error bounds and therefore of no significance. I also note an improvement in ARM performance although this project was never intended for ARM and LMAX seems like a much better solution on that platform architecture. Nice work! ⛏️🚀
|
Hi,
I want to start with noting that I am no expert in this domain, but while curiously browsing JDK23 release notes I stumbled onto the deprecation of
-XX:+UseEmptySlotsInSupers
in JDK-8330607 which then lead me into finding about field layout changes in JDK15+ which can affect padding for avoiding false-sharing.Long story short, it appears that padding now should be done with bytes instead of longs.
Unfortunately I did not yet get into doing any actual performance testing. Any feedback is highly appreciated.
Some relevant reading material: