Use byte padding to avoid fields re-ordering on JDK 15 and above#3168
Use byte padding to avoid fields re-ordering on JDK 15 and above#3168simonbasle merged 12 commits intoreactor:3.4.xfrom lantalex:enhancement/improve_spsc_queue_padding_3_4_x
Conversation
|
fixes #2057. |
|
@lantalex I tried to come up with a simple JOL-based test to validate the layout but it didn't fail even with original long padding in Java 17: @Test
@Tag("slow")
void objectPadding() {
ClassLayout layout = ClassLayout.parseClass(SpscArrayQueue.class);
AtomicLong currentPaddingSize = new AtomicLong();
List<String> interestingFields = new ArrayList<>();
layout.fields().forEach(f -> {
if (f.name().startsWith("pad")) {
currentPaddingSize.addAndGet(f.size());
}
else {
if (currentPaddingSize.get() > 0) {
interestingFields.add("padding of " + currentPaddingSize.getAndSet(0));
}
interestingFields.add(f.name());
}
});
if (currentPaddingSize.get() > 0) {
interestingFields.add("padding of " + currentPaddingSize.getAndSet(0));
}
assertThat(interestingFields).containsExactly(
"array",
"mask",
"padding of 120",
"producerIndex",
"padding of 120",
"consumerIndex",
"padding of 120"
);
}Compare to a similar test in another class that has padding (and could be also modified in your PR btw), @Test
@Tag("slow")
void objectPadding2() {
ClassLayout layout = ClassLayout.parseClass(QueueDrainSubscriber.class);
AtomicLong currentPaddingSize = new AtomicLong();
List<String> interestingFields = new ArrayList<>();
layout.fields().forEach(f -> {
if (f.name().startsWith("pad")) {
currentPaddingSize.addAndGet(f.size());
}
else {
if (currentPaddingSize.get() > 0) {
interestingFields.add("padding of " + currentPaddingSize.getAndSet(0));
}
interestingFields.add(f.name());
}
});
if (currentPaddingSize.get() > 0) {
interestingFields.add("padding of " + currentPaddingSize.getAndSet(0));
}
assertThat(interestingFields).containsExactly(
"padding of 120",
"wip",
"padding of 120",
"requested",
"padding of 120",
"cancelled",
"done",
"actual",
"queue",
"error"
);
}which fails on Java 17, having this layout detected instead: Note that in both cases I've renamed the padding fields for better identification to all use |
simonbasle
left a comment
There was a problem hiding this comment.
looking like a good start, but see my comment in PR discussion.
in addition, I would:
- use
padprefix instead ofb - add the
objectLayout()test (see PR comment, needs edition for 128 padding instead of 120) - make a similar change in
QueueDrainSubscriber(usingpadprefix too) - replace the
QueueDrainSubscriber#objectLayout()test with the simplified one (seeobjectLayout2()in my PR comment)
|
@simonbasle |
|
@lantalex nah, I'd say let's keep it that way, just in case it becomes the inspiration for a future padded class 😜 |
simonbasle
left a comment
There was a problem hiding this comment.
nice ! I wouldn't filter on the paddingSizes assertions but otherwise LGTM
reactor-core/src/test/java/reactor/core/publisher/QueueDrainSubscriberTest.java
Outdated
Show resolved
Hide resolved
reactor-core/src/test/java/reactor/util/concurrent/SpscArrayQueueTest.java
Outdated
Show resolved
Hide resolved
|
Hello @simonbasle ! |
reactor-core/src/test/java/reactor/util/concurrent/SpscArrayQueueTest.java
Outdated
Show resolved
Hide resolved
reactor-core/src/test/java/reactor/util/concurrent/SpscArrayQueueTest.java
Show resolved
Hide resolved
|
@simonbasle this PR seems to have been merged on a maintenance branch, please ensure the change is merge-forwarded to intermediate maintenance branches and up to |
|
thanks for the PR @lantalex 🎁 🙇 |
Thank you for review and fast feedback! 😀 |
Same thing as previous PR but for
3.4.xOn modern JDK (15+) good old trick using long fields as padding is no longer reliable. To prevent false-sharing we could use byte padding