From 368e3606392fde5185bb01e8ca892de42ba3bd8c Mon Sep 17 00:00:00 2001 From: Sunjeet Singh Date: Sat, 7 Oct 2023 12:00:19 -0700 Subject: [PATCH] Read state concurrent access bugfix- atomicity in accessing fields of shard holder --- .../object/HollowObjectTypeReadState.java | 115 ++++++++++++++---- .../HollowObjectTypeReadStateShard.java | 81 ++++-------- 2 files changed, 120 insertions(+), 76 deletions(-) diff --git a/hollow/src/main/java/com/netflix/hollow/core/read/engine/object/HollowObjectTypeReadState.java b/hollow/src/main/java/com/netflix/hollow/core/read/engine/object/HollowObjectTypeReadState.java index 9baa6b8501..f8cf6ceb82 100644 --- a/hollow/src/main/java/com/netflix/hollow/core/read/engine/object/HollowObjectTypeReadState.java +++ b/hollow/src/main/java/com/netflix/hollow/core/read/engine/object/HollowObjectTypeReadState.java @@ -20,6 +20,7 @@ import com.netflix.hollow.api.sampling.HollowObjectSampler; import com.netflix.hollow.api.sampling.HollowSampler; import com.netflix.hollow.api.sampling.HollowSamplingDirector; +import com.netflix.hollow.core.memory.HollowUnsafeHandle; import com.netflix.hollow.core.memory.MemoryMode; import com.netflix.hollow.core.memory.encoding.GapEncodedVariableLengthIntegerReader; import com.netflix.hollow.core.memory.encoding.VarInt; @@ -60,7 +61,7 @@ public ShardsHolder(HollowSchema schema, HollowObjectTypeDataElements[] dataElem this.shards = new HollowObjectTypeReadStateShard[numShards]; for (int i=0; i> shard.shardOrdinalShift, fieldIndex); + HollowObjectTypeReadState.ShardsHolder shardsHolder; + int result; + + do { + shardsHolder = this.shardsVolatile; // this read can return a stale shardHolder with greater or lesser than current + // num shards but maxOrdinal remains same across stale vs current. So given this + // atomic assignment below operations on a stale shards holder will be legal + HollowObjectTypeReadStateShard shard = shardsHolder.shards[ordinal & shardsHolder.shardNumberMask]; + result = shard.readInt(ordinal >> shard.shardOrdinalShift, fieldIndex); + } while(readWasUnsafe(shardsHolder)); + return result; } @Override public float readFloat(int ordinal, int fieldIndex) { sampler.recordFieldAccess(fieldIndex); - HollowObjectTypeReadStateShard shard = shardsVolatile.shards[ordinal & shardsVolatile.shardNumberMask]; - return shard.readFloat(ordinal >> shard.shardOrdinalShift, fieldIndex); + HollowObjectTypeReadState.ShardsHolder shardsHolder; + float result; + + do { + shardsHolder = this.shardsVolatile; + HollowObjectTypeReadStateShard shard = shardsHolder.shards[ordinal & shardsHolder.shardNumberMask]; + result = shard.readFloat(ordinal >> shard.shardOrdinalShift, fieldIndex); + } while(readWasUnsafe(shardsHolder)); + return result; } @Override public double readDouble(int ordinal, int fieldIndex) { sampler.recordFieldAccess(fieldIndex); - HollowObjectTypeReadStateShard shard = shardsVolatile.shards[ordinal & shardsVolatile.shardNumberMask]; - return shard.readDouble(ordinal >> shard.shardOrdinalShift, fieldIndex); + HollowObjectTypeReadState.ShardsHolder shardsHolder; + double result; + + do { + shardsHolder = this.shardsVolatile; + HollowObjectTypeReadStateShard shard = shardsHolder.shards[ordinal & shardsHolder.shardNumberMask]; + result = shard.readDouble(ordinal >> shard.shardOrdinalShift, fieldIndex); + } while(readWasUnsafe(shardsHolder)); + return result; } @Override public long readLong(int ordinal, int fieldIndex) { sampler.recordFieldAccess(fieldIndex); - HollowObjectTypeReadStateShard shard = shardsVolatile.shards[ordinal & shardsVolatile.shardNumberMask]; - return shard.readLong(ordinal >> shard.shardOrdinalShift, fieldIndex); + HollowObjectTypeReadState.ShardsHolder shardsHolder; + long result; + + do { + shardsHolder = this.shardsVolatile; + HollowObjectTypeReadStateShard shard = shardsHolder.shards[ordinal & shardsHolder.shardNumberMask]; + result = shard.readLong(ordinal >> shard.shardOrdinalShift, fieldIndex); + } while(readWasUnsafe(shardsHolder)); + return result; } @Override public Boolean readBoolean(int ordinal, int fieldIndex) { sampler.recordFieldAccess(fieldIndex); - HollowObjectTypeReadStateShard shard = shardsVolatile.shards[ordinal & shardsVolatile.shardNumberMask]; - return shard.readBoolean(ordinal >> shard.shardOrdinalShift, fieldIndex); + HollowObjectTypeReadState.ShardsHolder shardsHolder; + Boolean result; + + do { + shardsHolder = this.shardsVolatile; + HollowObjectTypeReadStateShard shard = shardsHolder.shards[ordinal & shardsHolder.shardNumberMask]; + result = shard.readBoolean(ordinal >> shard.shardOrdinalShift, fieldIndex); + } while(readWasUnsafe(shardsHolder)); + return result; } @Override public byte[] readBytes(int ordinal, int fieldIndex) { sampler.recordFieldAccess(fieldIndex); - HollowObjectTypeReadStateShard shard = shardsVolatile.shards[ordinal & shardsVolatile.shardNumberMask]; - return shard.readBytes(ordinal >> shard.shardOrdinalShift, fieldIndex); + HollowObjectTypeReadState.ShardsHolder shardsHolder; + byte[] result; + + do { + shardsHolder = this.shardsVolatile; + HollowObjectTypeReadStateShard shard = shardsHolder.shards[ordinal & shardsHolder.shardNumberMask]; + result = shard.readBytes(ordinal >> shard.shardOrdinalShift, fieldIndex); + } while(readWasUnsafe(shardsHolder)); + return result; } @Override public String readString(int ordinal, int fieldIndex) { sampler.recordFieldAccess(fieldIndex); - HollowObjectTypeReadStateShard shard = shardsVolatile.shards[ordinal & shardsVolatile.shardNumberMask]; - return shard.readString(ordinal >> shard.shardOrdinalShift, fieldIndex); + HollowObjectTypeReadState.ShardsHolder shardsHolder; + String result; + + do { + shardsHolder = this.shardsVolatile; + HollowObjectTypeReadStateShard shard = shardsHolder.shards[ordinal & shardsHolder.shardNumberMask]; + result = shard.readString(ordinal >> shard.shardOrdinalShift, fieldIndex); + } while(readWasUnsafe(shardsHolder)); + return result; } @Override public boolean isStringFieldEqual(int ordinal, int fieldIndex, String testValue) { sampler.recordFieldAccess(fieldIndex); - HollowObjectTypeReadStateShard shard = shardsVolatile.shards[ordinal & shardsVolatile.shardNumberMask]; - return shard.isStringFieldEqual(ordinal >> shard.shardOrdinalShift, fieldIndex, testValue); + HollowObjectTypeReadState.ShardsHolder shardsHolder; + boolean result; + + do { + shardsHolder = this.shardsVolatile; + HollowObjectTypeReadStateShard shard = shardsHolder.shards[ordinal & shardsHolder.shardNumberMask]; + result = shard.isStringFieldEqual(ordinal >> shard.shardOrdinalShift, fieldIndex, testValue); + } while(readWasUnsafe(shardsHolder)); + return result; } @Override public int findVarLengthFieldHashCode(int ordinal, int fieldIndex) { sampler.recordFieldAccess(fieldIndex); - HollowObjectTypeReadStateShard shard = shardsVolatile.shards[ordinal & shardsVolatile.shardNumberMask]; - return shard.findVarLengthFieldHashCode(ordinal >> shard.shardOrdinalShift, fieldIndex); + HollowObjectTypeReadState.ShardsHolder shardsHolder; + int hashCode; + + do { + shardsHolder = this.shardsVolatile; + HollowObjectTypeReadStateShard shard = shardsHolder.shards[ordinal & shardsHolder.shardNumberMask]; + hashCode = shard.findVarLengthFieldHashCode(ordinal >> shard.shardOrdinalShift, fieldIndex); + } while(readWasUnsafe(shardsHolder)); + return hashCode; + } + + private boolean readWasUnsafe(ShardsHolder shardsHolder) { + HollowUnsafeHandle.getUnsafe().loadFence(); // for why, see explanation in HollowObjectTypeReadStateShard::readWasUnsafe + return shardsHolder != shardsVolatile; } /** @@ -522,7 +593,7 @@ void setCurrentData(HollowObjectTypeDataElements data) { HollowObjectTypeReadStateShard[] shards = this.shardsVolatile.shards; if(shards.length > 1) throw new UnsupportedOperationException("Cannot directly set data on sharded type state"); - shards[0].setCurrentData(this.shardsVolatile, data); + shards[0].setCurrentData(data); maxOrdinal = data.maxOrdinal; } diff --git a/hollow/src/main/java/com/netflix/hollow/core/read/engine/object/HollowObjectTypeReadStateShard.java b/hollow/src/main/java/com/netflix/hollow/core/read/engine/object/HollowObjectTypeReadStateShard.java index efdc487efa..142b46ac3e 100644 --- a/hollow/src/main/java/com/netflix/hollow/core/read/engine/object/HollowObjectTypeReadStateShard.java +++ b/hollow/src/main/java/com/netflix/hollow/core/read/engine/object/HollowObjectTypeReadStateShard.java @@ -33,27 +33,23 @@ import java.util.Collections; import java.util.List; -public class HollowObjectTypeReadStateShard { // TODO: package private +class HollowObjectTypeReadStateShard { private volatile HollowObjectTypeDataElements currentDataVolatile; - private volatile HollowObjectTypeReadState.ShardsHolder currentShardsVolatile; // SNAP: TODO: test that back reference doesn't need to be cleaned up - - final int shardOrdinalShift; // (ordinal>>shardOrdinalShift) yields a translated in-shard ordinal; useful for skipping ordinals in underlying data elements when splitting shards + final int shardOrdinalShift; private final HollowObjectSchema schema; - public HollowObjectTypeReadStateShard(HollowObjectSchema schema, int shardOrdinalShift) { // TODO: package private + HollowObjectTypeReadStateShard(HollowObjectSchema schema, int shardOrdinalShift) { this.schema = schema; this.shardOrdinalShift = shardOrdinalShift; } public boolean isNull(int ordinal, int fieldIndex) { - HollowObjectTypeReadState.ShardsHolder currentShards; HollowObjectTypeDataElements currentData; long fixedLengthValue; do { - currentShards = this.currentShardsVolatile; currentData = this.currentDataVolatile; long bitOffset = fieldOffset(currentData, ordinal, fieldIndex); @@ -62,7 +58,7 @@ public boolean isNull(int ordinal, int fieldIndex) { fixedLengthValue = numBitsForField <= 56 ? currentData.fixedLengthData.getElementValue(bitOffset, numBitsForField) : currentData.fixedLengthData.getLargeElementValue(bitOffset, numBitsForField); - } while(readWasUnsafe(currentShards, currentData)); + } while(readWasUnsafe(currentData)); switch(schema.getFieldType(fieldIndex)) { case BYTES: @@ -79,15 +75,13 @@ public boolean isNull(int ordinal, int fieldIndex) { } public int readOrdinal(int ordinal, int fieldIndex) { - HollowObjectTypeReadState.ShardsHolder currentShards; HollowObjectTypeDataElements currentData; long refOrdinal; do { - currentShards = this.currentShardsVolatile; currentData = this.currentDataVolatile; refOrdinal = readFixedLengthFieldValue(currentData, ordinal, fieldIndex); - } while(readWasUnsafe(currentShards, currentData)); + } while(readWasUnsafe(currentData)); if(refOrdinal == currentData.nullValueForField[fieldIndex]) return ORDINAL_NONE; @@ -95,15 +89,13 @@ public int readOrdinal(int ordinal, int fieldIndex) { } public int readInt(int ordinal, int fieldIndex) { - HollowObjectTypeReadState.ShardsHolder currentShards; HollowObjectTypeDataElements currentData; long value; do { - currentShards = this.currentShardsVolatile; currentData = this.currentDataVolatile; value = readFixedLengthFieldValue(currentData, ordinal, fieldIndex); - } while(readWasUnsafe(currentShards, currentData)); + } while(readWasUnsafe(currentData)); if(value == currentData.nullValueForField[fieldIndex]) return Integer.MIN_VALUE; @@ -111,15 +103,13 @@ public int readInt(int ordinal, int fieldIndex) { } public float readFloat(int ordinal, int fieldIndex) { - HollowObjectTypeReadState.ShardsHolder currentShards; HollowObjectTypeDataElements currentData; int value; do { - currentShards = this.currentShardsVolatile; currentData = this.currentDataVolatile; value = (int)readFixedLengthFieldValue(currentData, ordinal, fieldIndex); - } while(readWasUnsafe(currentShards, currentData)); + } while(readWasUnsafe(currentData)); if(value == HollowObjectWriteRecord.NULL_FLOAT_BITS) return Float.NaN; @@ -127,16 +117,14 @@ public float readFloat(int ordinal, int fieldIndex) { } public double readDouble(int ordinal, int fieldIndex) { - HollowObjectTypeReadState.ShardsHolder currentShards; HollowObjectTypeDataElements currentData; long value; do { - currentShards = this.currentShardsVolatile; currentData = this.currentDataVolatile; long bitOffset = fieldOffset(currentData, ordinal, fieldIndex); value = currentData.fixedLengthData.getLargeElementValue(bitOffset, 64, -1L); - } while(readWasUnsafe(currentShards, currentData)); + } while(readWasUnsafe(currentData)); if(value == HollowObjectWriteRecord.NULL_DOUBLE_BITS) return Double.NaN; @@ -144,17 +132,15 @@ public double readDouble(int ordinal, int fieldIndex) { } public long readLong(int ordinal, int fieldIndex) { - HollowObjectTypeReadState.ShardsHolder currentShards; HollowObjectTypeDataElements currentData; long value; do { - currentShards = this.currentShardsVolatile; currentData = this.currentDataVolatile; long bitOffset = fieldOffset(currentData, ordinal, fieldIndex); int numBitsForField = currentData.bitsPerField[fieldIndex]; value = currentData.fixedLengthData.getLargeElementValue(bitOffset, numBitsForField); - } while(readWasUnsafe(currentShards, currentData)); + } while(readWasUnsafe(currentData)); if(value == currentData.nullValueForField[fieldIndex]) return Long.MIN_VALUE; @@ -162,15 +148,13 @@ public long readLong(int ordinal, int fieldIndex) { } public Boolean readBoolean(int ordinal, int fieldIndex) { - HollowObjectTypeReadState.ShardsHolder currentShards; HollowObjectTypeDataElements currentData; long value; do { - currentShards = this.currentShardsVolatile; currentData = this.currentDataVolatile; value = readFixedLengthFieldValue(currentData, ordinal, fieldIndex); - } while(readWasUnsafe(currentShards, currentData)); + } while(readWasUnsafe(currentData)); if(value == currentData.nullValueForField[fieldIndex]) return null; @@ -187,7 +171,6 @@ private long readFixedLengthFieldValue(HollowObjectTypeDataElements currentData, } public byte[] readBytes(int ordinal, int fieldIndex) { - HollowObjectTypeReadState.ShardsHolder currentShards; HollowObjectTypeDataElements currentData; byte[] result; @@ -197,14 +180,13 @@ public byte[] readBytes(int ordinal, int fieldIndex) { long startByte; do { - currentShards = this.currentShardsVolatile; currentData = this.currentDataVolatile; numBitsForField = currentData.bitsPerField[fieldIndex]; long currentBitOffset = fieldOffset(currentData, ordinal, fieldIndex); endByte = currentData.fixedLengthData.getElementValue(currentBitOffset, numBitsForField); startByte = ordinal != 0 ? currentData.fixedLengthData.getElementValue(currentBitOffset - currentData.bitsPerRecord, numBitsForField) : 0; - } while(readWasUnsafe(currentShards, currentData)); + } while(readWasUnsafe(currentData)); if((endByte & (1L << numBitsForField - 1)) != 0) return null; @@ -216,13 +198,12 @@ public byte[] readBytes(int ordinal, int fieldIndex) { for(int i=0;i