Skip to content

Commit

Permalink
Make some code on SharedBlobCacheService hot path more compact
Browse files Browse the repository at this point in the history
Make some code here a little friendlier to inline on the very hot path
by removing assertions and safety checks (that are redundant anyway).
This doesn't matter massively now but the closer we move to directly
accessing the mmap in the SharedBytes.IO the more we are hurt by any
extra code/checks.
Also, replace the evicted AtomicBoolean with a hand-crafted volatile
+ var handle solution to save some memory and indirection as we look
into potentially making the cache more fine-grained.
  • Loading branch information
original-brownbear committed Jul 28, 2023
1 parent 46c8193 commit ee7aff5
Showing 1 changed file with 39 additions and 20 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,8 @@

import java.io.IOException;
import java.io.UncheckedIOException;
import java.lang.invoke.MethodHandles;
import java.lang.invoke.VarHandle;
import java.nio.ByteBuffer;
import java.util.ArrayList;
import java.util.Collection;
Expand All @@ -51,7 +53,6 @@
import java.util.concurrent.ConcurrentLinkedQueue;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.Executor;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.concurrent.atomic.AtomicInteger;
import java.util.concurrent.atomic.AtomicReferenceArray;
import java.util.concurrent.atomic.LongAdder;
Expand Down Expand Up @@ -336,7 +337,7 @@ public int getRecoveryRangeSize() {
}

private int getRegion(long position) {
return Math.toIntExact(position / regionSize);
return (int) (position / regionSize);
}

private long getRegionRelativePosition(long position) {
Expand All @@ -352,11 +353,7 @@ private long getRegionEnd(int region) {
}

private int getEndingRegion(long position) {
assert position > 0L;
if (position % regionSize == 0L) {
return getRegion(position - 1);
}
return getRegion(position);
return getRegion(position - (position % regionSize == 0 ? 1 : 0));
}

private ByteRange mapSubRangeToRegion(ByteRange range, int region) {
Expand Down Expand Up @@ -714,7 +711,36 @@ static class Entry<T> {
}
}

class CacheFileRegion extends AbstractRefCounted {
// only inherited by CacheFileRegion to enable the use of a static var handle in on a non-static inner class
private abstract static class EvictableRefCounted extends AbstractRefCounted {
protected static final VarHandle VH_EVICTED_FIELD;

static {
try {
VH_EVICTED_FIELD = MethodHandles.lookup()
.in(EvictableRefCounted.class)
.findVarHandle(EvictableRefCounted.class, "evicted", int.class);
} catch (NoSuchFieldException | IllegalAccessException e) {
throw new RuntimeException(e);
}
}

// If != 0 this file region has been evicted from the cache and should not be used anymore
// implemented using a var handle instead of an atomic boolean to save space and indirection
@SuppressWarnings("FieldMayBeFinal") // updated via VH_EVICTED_FIELD (and _only_ via VH_EVICTED_FIELD)
private volatile int evicted = 0;

protected final boolean evict() {
return VH_EVICTED_FIELD.compareAndSet(this, 0, 1);
}

public final boolean isEvicted() {
return evicted != 0;
}
}

class CacheFileRegion extends EvictableRefCounted {

final RegionKey<KeyType> regionKey;
final SparseFileTracker tracker;
volatile int sharedBytesPos = -1;
Expand All @@ -733,14 +759,11 @@ public long physicalEndOffset() {
return sharedBytes.getPhysicalOffset(sharedBytesPos + 1);
}

// If true this file region has been evicted from the cache and should not be used any more
private final AtomicBoolean evicted = new AtomicBoolean(false);

// tries to evict this chunk if noone is holding onto its resources anymore
// visible for tests.
boolean tryEvict() {
assert Thread.holdsLock(SharedBlobCacheService.this) : "must hold lock when evicting";
if (refCount() <= 1 && evicted.compareAndSet(false, true)) {
if (refCount() <= 1 && evict()) {
logger.trace("evicted {} with channel offset {}", regionKey, physicalStartOffset());
evictCount.increment();
decRef();
Expand All @@ -751,7 +774,7 @@ boolean tryEvict() {

public boolean forceEvict() {
assert Thread.holdsLock(SharedBlobCacheService.this) : "must hold lock when evicting";
if (evicted.compareAndSet(false, true)) {
if (evict()) {
logger.trace("force evicted {} with channel offset {}", regionKey, physicalStartOffset());
evictCount.increment();
decRef();
Expand All @@ -760,10 +783,6 @@ public boolean forceEvict() {
return false;
}

public boolean isEvicted() {
return evicted.get();
}

@Override
protected void closeInternal() {
// now actually free the region associated with this chunk
Expand All @@ -772,7 +791,7 @@ protected void closeInternal() {
}

private void ensureOpen() {
if (evicted.get()) {
if (isEvicted()) {
throwAlreadyEvicted();
}
}
Expand Down Expand Up @@ -944,7 +963,7 @@ public int populateAndRead(
final RangeAvailableHandler reader,
final RangeMissingHandler writer
) throws Exception {
if (rangeToRead.length() == 0L) {
if (rangeToRead.isEmpty()) {
// nothing to read, skip
return 0;
}
Expand Down Expand Up @@ -989,7 +1008,7 @@ private int readMultiRegions(
try (var listeners = new RefCountingListener(1, readsComplete)) {
for (int region = startRegion; region <= endRegion; region++) {
final ByteRange subRangeToRead = mapSubRangeToRegion(rangeToRead, region);
if (subRangeToRead.length() == 0L) {
if (subRangeToRead.isEmpty()) {
// nothing to read, skip
continue;
}
Expand Down

0 comments on commit ee7aff5

Please sign in to comment.