Skip to content

Commit

Permalink
Strengthen memory barriers used by benign reads and writes
Browse files Browse the repository at this point in the history 
`drainStatus`
The status is used to trigger the maintenance and guard against some
unnecessary runs. As the maintenance work is fast, atomic, and expected
to run frequently there is no harm even if a race causes an extra
execution. This might happen due to a buffered write to transition into
the processing state being stomped on by a cache write CAS'ing to flag
that a drain is required ASAP. This case now uses a stronger release and
opaque is used where no conflicting writes are possible.

The loop in `scheduleAfterWrite()` only assumes a read barrier if
retrying due to a failed CAS. This is guaranteed on weakly ordered
memory models like Arm's. We specifically avoided assumptions like [1]
that tried to piggyback other writes onto a CAS.

`timestamps`
The expiration timestamps are upgraded from plain loads and stores to
opaque ones. This is primarily for code clarity when matching operations
against [2]. A benefit is that it does ensure 64-bit loads on 32-bit
systems to avoid word tearing (as allowed by the Java spec, but is no
longer applicable).

[1] https://youtu.be/EFkpmFt61Jo?t=1789
[2] http://gee.cs.oswego.edu/dl/html/j9mm.html
  • Loading branch information
@ben-manes
ben-manes committed Nov 28, 2021
1 parent 0fd820f commit 4fb19e8
Show file tree
Hide file tree
Showing 6 changed files with 50 additions and 43 deletions.
4 changes: 2 additions & 2 deletions caffeine/src/javaPoet/java/com/github/benmanes/caffeine/cache/node/AddDeques.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ protected void execute() {
/** Adds a simple field, accessor, and mutator for the variable. */
private void addFieldAndGetter(String varName) {
context.nodeSubtype.addField(NODE, varName)
.addMethod(newGetter(Strength.STRONG, NODE, varName, Visibility.IMMEDIATE))
.addMethod(newSetter(NODE, varName, Visibility.IMMEDIATE));
.addMethod(newGetter(Strength.STRONG, NODE, varName, Visibility.VOLATILE))
.addMethod(newSetter(NODE, varName, Visibility.VOLATILE));
}
}
10 changes: 5 additions & 5 deletions caffeine/src/javaPoet/java/com/github/benmanes/caffeine/cache/node/AddExpiration.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -83,13 +83,13 @@ private void addLink(String method, String varName) {
private void addVariableTime(String varName) {
var getter = MethodSpec.methodBuilder("getVariableTime")
.addModifiers(Modifier.PUBLIC)
.addStatement("return (long) $L.get(this)", varHandleName(varName))
.addStatement("return (long) $L.getOpaque(this)", varHandleName(varName))
.returns(long.class)
.build();
var setter = MethodSpec.methodBuilder("setVariableTime")
.addModifiers(Modifier.PUBLIC)
.addParameter(long.class, varName)
.addStatement("$L.set(this, $N)", varHandleName(varName), varName)
.addStatement("$L.setOpaque(this, $N)", varHandleName(varName), varName)
.build();
var cas = MethodSpec.methodBuilder("casVariableTime")
.addModifiers(Modifier.PUBLIC)
Expand All @@ -112,8 +112,8 @@ private void addAccessExpiration() {

context.nodeSubtype
.addField(long.class, "accessTime", Modifier.VOLATILE)
.addMethod(newGetter(Strength.STRONG, TypeName.LONG, "accessTime", Visibility.PLAIN))
.addMethod(newSetter(TypeName.LONG, "accessTime", Visibility.PLAIN));
.addMethod(newGetter(Strength.STRONG, TypeName.LONG, "accessTime", Visibility.OPAQUE))
.addMethod(newSetter(TypeName.LONG, "accessTime", Visibility.OPAQUE));
addVarHandle("accessTime", TypeName.get(long.class));
addTimeConstructorAssignment(context.constructorByKey, "accessTime", "now");
addTimeConstructorAssignment(context.constructorByKeyRef, "accessTime", "now");
Expand All @@ -124,7 +124,7 @@ private void addWriteExpiration() {
&& Feature.useWriteTime(context.generateFeatures)) {
context.nodeSubtype
.addField(long.class, "writeTime", Modifier.VOLATILE)
.addMethod(newGetter(Strength.STRONG, TypeName.LONG, "writeTime", Visibility.PLAIN))
.addMethod(newGetter(Strength.STRONG, TypeName.LONG, "writeTime", Visibility.OPAQUE))
.addMethod(newSetter(TypeName.LONG, "writeTime", Visibility.PLAIN));
addVarHandle("writeTime", TypeName.get(long.class));
addTimeConstructorAssignment(context.constructorByKey, "writeTime", "now & ~1L");
Expand Down
8 changes: 4 additions & 4 deletions caffeine/src/javaPoet/java/com/github/benmanes/caffeine/cache/node/AddMaximum.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,13 +57,13 @@ private void addWeight() {
return;
}
context.nodeSubtype.addField(int.class, "weight")
.addMethod(newGetter(Strength.STRONG, TypeName.INT, "weight", Visibility.IMMEDIATE))
.addMethod(newSetter(TypeName.INT, "weight", Visibility.IMMEDIATE));
.addMethod(newGetter(Strength.STRONG, TypeName.INT, "weight", Visibility.VOLATILE))
.addMethod(newSetter(TypeName.INT, "weight", Visibility.VOLATILE));
context.constructorByKey.addStatement("this.$N = $N", "weight", "weight");
context.constructorByKeyRef.addStatement("this.$N = $N", "weight", "weight");

context.nodeSubtype.addField(int.class, "policyWeight")
.addMethod(newGetter(Strength.STRONG, TypeName.INT, "policyWeight", Visibility.IMMEDIATE))
.addMethod(newSetter(TypeName.INT, "policyWeight", Visibility.IMMEDIATE));
.addMethod(newGetter(Strength.STRONG, TypeName.INT, "policyWeight", Visibility.VOLATILE))
.addMethod(newSetter(TypeName.INT, "policyWeight", Visibility.VOLATILE));
}
}
41 changes: 22 additions & 19 deletions caffeine/src/javaPoet/java/com/github/benmanes/caffeine/cache/node/NodeRule.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -132,22 +132,27 @@ protected final MethodSpec newGetter(Strength strength, TypeName varType,
.addModifiers(context.publicFinalModifiers())
.returns(varType);
if (strength == Strength.STRONG) {
if (visibility.isPlain) {
if (varType.isPrimitive()) {
getter.addStatement("return ($L) $L.get(this)",
varType.toString(), varHandleName(varName));
} else {
getter.addStatement("return ($T) $L.get(this)", varType, varHandleName(varName));
}
} else {
if ((visibility == Visibility.PLAIN) || (visibility == Visibility.OPAQUE)) {
var template = String.format("return (%s) $L.get(this)",
varType.isPrimitive() ? "$L" : "$T");
getter.addStatement(template, varType, varHandleName(varName));
} else if (visibility == Visibility.OPAQUE) {
var template = String.format("return (%s) $L.getOpaque(this)",
varType.isPrimitive() ? "$L" : "$T");
getter.addStatement(template, varType, varHandleName(varName));
} else if (visibility == Visibility.VOLATILE) {
getter.addStatement("return $N", varName);
} else {
throw new IllegalArgumentException();
}
} else {
if (visibility.isPlain) {
if (visibility == Visibility.PLAIN) {
getter.addStatement("return (($T<$T>) $L.get(this)).get()",
Reference.class, varType, varHandleName(varName));
} else {
} else if (visibility == Visibility.VOLATILE) {
getter.addStatement("return $N.get()", varName);
} else {
throw new IllegalArgumentException();
}
}
return getter.build();
Expand All @@ -159,10 +164,14 @@ protected final MethodSpec newSetter(TypeName varType, String varName, Visibilit
var setter = MethodSpec.methodBuilder(methodName)
.addModifiers(context.publicFinalModifiers())
.addParameter(varType, varName);
if (visibility.isPlain) {
if (visibility == Visibility.PLAIN) {
setter.addStatement("$L.set(this, $N)", varHandleName(varName), varName);
} else {
} else if (visibility == Visibility.OPAQUE) {
setter.addStatement("$L.setOpaque(this, $N)", varHandleName(varName), varName);
} else if (visibility == Visibility.VOLATILE) {
setter.addStatement("this.$N = $N", varName, varName);
} else {
throw new IllegalArgumentException();
}
return setter.build();
}
Expand All @@ -181,12 +190,6 @@ protected enum Strength {
}

protected enum Visibility {
IMMEDIATE(false), PLAIN(true);

final boolean isPlain;

Visibility(boolean mode) {
this.isPlain = mode;
}
PLAIN, OPAQUE, VOLATILE
}
}
16 changes: 10 additions & 6 deletions caffeine/src/main/java/com/github/benmanes/caffeine/cache/BoundedLocalCache.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1510,7 +1510,7 @@ void scheduleDrainBuffers() {
if (drainStatus >= PROCESSING_TO_IDLE) {
return;
}
lazySetDrainStatus(PROCESSING_TO_IDLE);
setDrainStatusRelease(PROCESSING_TO_IDLE);
executor.execute(drainBuffersTask);
} catch (Throwable t) {
logger.log(Level.WARNING, "Exception thrown when submitting maintenance task", t);
Expand Down Expand Up @@ -1556,7 +1556,7 @@ void performCleanUp(@Nullable Runnable task) {
*/
@GuardedBy("evictionLock")
void maintenance(@Nullable Runnable task) {
lazySetDrainStatus(PROCESSING_TO_IDLE);
setDrainStatusRelease(PROCESSING_TO_IDLE);

try {
drainReadBuffer();
Expand All @@ -1575,7 +1575,7 @@ void maintenance(@Nullable Runnable task) {
climb();
} finally {
if ((drainStatus() != PROCESSING_TO_IDLE) || !casDrainStatus(PROCESSING_TO_IDLE, IDLE)) {
lazySetDrainStatus(REQUIRED);
setDrainStatusOpaque(REQUIRED);
}
}
}
Expand Down Expand Up @@ -1684,7 +1684,7 @@ void drainWriteBuffer() {
}
task.run();
}
lazySetDrainStatus(PROCESSING_TO_REQUIRED);
setDrainStatusOpaque(PROCESSING_TO_REQUIRED);
}

/**
Expand Down Expand Up @@ -4131,13 +4131,17 @@ boolean shouldDrainBuffers(boolean delayable) {
}

int drainStatus() {
return (int) DRAIN_STATUS.get(this);
return (int) DRAIN_STATUS.getOpaque(this);
}

void lazySetDrainStatus(int drainStatus) {
void setDrainStatusOpaque(int drainStatus) {
DRAIN_STATUS.setOpaque(this, drainStatus);
}

void setDrainStatusRelease(int drainStatus) {
DRAIN_STATUS.setRelease(this, drainStatus);
}

boolean casDrainStatus(int expect, int update) {
return DRAIN_STATUS.compareAndSet(this, expect, update);
}
Expand Down
14 changes: 7 additions & 7 deletions caffeine/src/test/java/com/github/benmanes/caffeine/cache/BoundedLocalCacheTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -755,10 +755,10 @@ public void updateRecency_onReplaceConditionally(
}

private static Node<Int, Int> firstBeforeAccess(
BoundedLocalCache<Int, Int> localCache, CacheContext context) {
BoundedLocalCache<Int, Int> cache, CacheContext context) {
return context.isZeroWeighted()
? localCache.accessOrderWindowDeque().peek()
: localCache.accessOrderProbationDeque().peek();
? cache.accessOrderWindowDeque().peek()
: cache.accessOrderProbationDeque().peek();
}

private static void updateRecency(BoundedLocalCache<Int, Int> cache,
Expand Down Expand Up @@ -874,7 +874,7 @@ public void drain_onWrite(BoundedLocalCache<Int, Int> cache, CacheContext contex
public void drain_nonblocking(BoundedLocalCache<Int, Int> cache, CacheContext context) {
var done = new AtomicBoolean();
Runnable task = () -> {
cache.lazySetDrainStatus(REQUIRED);
cache.setDrainStatusRelease(REQUIRED);
cache.scheduleDrainBuffers();
done.set(true);
};
Expand Down Expand Up @@ -907,13 +907,13 @@ public void drain_blocksCapacity(BoundedLocalCache<Int, Int> cache,
checkDrainBlocks(cache, () -> eviction.setMaximum(0));
}

void checkDrainBlocks(BoundedLocalCache<Int, Int> localCache, Runnable task) {
void checkDrainBlocks(BoundedLocalCache<Int, Int> cache, Runnable task) {
var done = new AtomicBoolean();
var lock = localCache.evictionLock;
var lock = cache.evictionLock;
lock.lock();
try {
ConcurrentTestHarness.execute(() -> {
localCache.lazySetDrainStatus(REQUIRED);
cache.setDrainStatusRelease(REQUIRED);
task.run();
done.set(true);
});
Expand Down

0 comments on commit 4fb19e8

Please sign in to comment.