sjvanrossum commented on code in PR #34165:
URL: https://github.com/apache/beam/pull/34165#discussion_r1989505102
##########
sdks/java/io/kafka/src/main/java/org/apache/beam/sdk/io/kafka/KafkaIOUtils.java:
##########
@@ -129,19 +131,80 @@ static Map<String, Object> getOffsetConsumerConfig(
return offsetConsumerConfig;
}
- // Maintains approximate average over last 1000 elements
- static class MovingAvg {
+ /*
+ * Attempt to prevent false sharing by padding to at least 64 bytes.
+ * object header: 4, 8, 12 or 16 bytes
+ * alignment: at least 8 bytes
+ */
+ @SuppressFBWarnings("UUF_UNUSED_FIELD")
+ private static class MovingAvgPadding {
+ byte p000, p001, p002, p003, p004, p005, p006, p007;
+ byte p010, p011, p012, p013, p014, p015, p016, p017;
+ byte p020, p021, p022, p023, p024, p025, p026, p027;
+ byte p030, p031, p032, p033, p034, p035, p036, p037;
+ byte p040, p041, p042, p043, p044, p045, p046, p047;
+ byte p050, p051, p052, p053, p054, p055, p056, p057;
+ byte p060, p061, p062, p063, p064, p065, p066, p067;
+ }
+
+ // The accumulator's fields should be padded to at least 128 bytes (at least
1 or 2
+ // cache lines).
+ private static class MovingAvgFields extends MovingAvgPadding {
Review Comment:
The compiler may rearrange fields within a class arbitrarily, but it can't
freely rearrange fields within a class hierarchy except to fill alignment gaps
in a super class since that space would be unused in an instance of the super
class and thus it's safe for any subclass to place its fields there (as happens
in JDK 15 and up).
Example:
```
// Fields may be rearranged in any order by the compiler.
class A {
long prePaddingField, ...;
volatile long importantField;
long postPaddingField, ...;
}
// For compactness use 8 byte fields instead to allow the compiler to fill
alignment gaps
class APrePadding {
long prePaddingField, ...;
}
class AFields extends APrePadding {
volatile long importantField;
}
// For compactness use 8 byte fields instead to allow the compiler to fill
alignment gaps
class APostPadding extends AFields {
long postPaddingField, ...;
}
// This class should have prePaddingField(s) placed before importantField
and postPaddingField(s) placed after importantField
class PaddedA extends APostPadding {}
```
Padding before the volatile field should ensure it is loaded onto a
different cache line than the mark word of the object header and padding after
the volatile field should ensure the same for other objects in the cache.
See
https://shipilev.net/jvm/objects-inside-out/#_field_layout_across_the_hierarchy
and
https://github.com/ben-manes/caffeine/blob/v3.2.0/caffeine/src/main/java/com/github/benmanes/caffeine/cache/BoundedBuffer.java#L172
for examples and
https://github.com/google/guava/blame/v33.4.0/android/guava/src/com/google/common/hash/Striped64.java#L103
in Guava for a comment about field reordering. Separation of fields through
the class hierarchy is considered more robust against reordering, but given
that Guava doesn't use that pattern it would seem it doesn't matter much in
practice.
In any case, after adjusting the benchmark experiments to model the
interaction between readers and writers that I expect in Beam it appears that
layout padding is irrelevant. :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscr...@beam.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
