From e5415d9269cc9f18639a0a2fd9b5a9129eea19ea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mattias=20Finn=C3=A9?= Date: Wed, 14 Mar 2018 13:04:06 +0100 Subject: [PATCH] More retry-resilient StringSchemaKey compare As well as deduplication of some common native schema key initialization code reseting flags and entityId. --- .../schema/ConflictDetectingValueMerger.java | 2 +- .../impl/index/schema/NativeSchemaKey.java | 23 ++++++++---- .../impl/index/schema/SpatialSchemaKey.java | 3 +- .../impl/index/schema/StringSchemaKey.java | 37 ++++++++----------- 4 files changed, 33 insertions(+), 32 deletions(-) diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/ConflictDetectingValueMerger.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/ConflictDetectingValueMerger.java index 28342ac30aa21..37be19666feb6 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/ConflictDetectingValueMerger.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/ConflictDetectingValueMerger.java @@ -30,7 +30,7 @@ * {@link ValueMerger} which will merely detect conflict, not change any value if conflict, i.e. if the * key already exists. After this merge has been used in a call to {@link Writer#merge(Object, Object, ValueMerger)} * {@link #checkConflict(Value[])} can be called to check whether or not that call conflicted with - * an existing key. A call to {@link #checkConflict(Value[])} will also clear the conflict flag. + * an existing key. A call to {@link #checkConflict(Value[])} will also initialize the conflict flag. * * @param type of values being merged. */ diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/NativeSchemaKey.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/NativeSchemaKey.java index c899b6e2779aa..2689466342980 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/NativeSchemaKey.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/NativeSchemaKey.java @@ -30,7 +30,7 @@ */ abstract class NativeSchemaKey extends ValueWriter.Adapter { - static final boolean DEFAULT_COMPARE_ID = true; + private static final boolean DEFAULT_COMPARE_ID = true; private long entityId; private boolean compareId = DEFAULT_COMPARE_ID; @@ -64,12 +64,23 @@ void setEntityId( long entityId ) final void from( long entityId, Value... values ) { - compareId = DEFAULT_COMPARE_ID; - this.entityId = entityId; + initialize( entityId ); // copy value state and store in this key instance assertValidValue( values ).writeTo( this ); } + /** + * Initializes this key with entity id and resets other flags to default values. + * Doesn't touch value data. + * + * @param entityId entity id to set for this key. + */ + void initialize( long entityId ) + { + this.compareId = DEFAULT_COMPARE_ID; + this.entityId = entityId; + } + private Value assertValidValue( Value... values ) { if ( values.length > 1 ) @@ -94,8 +105,7 @@ String propertiesAsString() final void initAsLowest() { - compareId = DEFAULT_COMPARE_ID; - entityId = Long.MIN_VALUE; + initialize( Long.MIN_VALUE ); initValueAsLowest(); } @@ -103,8 +113,7 @@ final void initAsLowest() final void initAsHighest() { - compareId = DEFAULT_COMPARE_ID; - entityId = Long.MAX_VALUE; + initialize( Long.MAX_VALUE ); initValueAsHighest(); } diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/SpatialSchemaKey.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/SpatialSchemaKey.java index b26caa0fa2677..11a177abd21ad 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/SpatialSchemaKey.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/SpatialSchemaKey.java @@ -81,8 +81,7 @@ void initValueAsHighest() public void fromDerivedValue( long entityId, long derivedValue ) { rawValueBits = derivedValue; - setEntityId( entityId ); - setCompareId( DEFAULT_COMPARE_ID ); + initialize( entityId ); } /** diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/StringSchemaKey.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/StringSchemaKey.java index bee8b298f3951..60a3fe609ffb0 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/StringSchemaKey.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/StringSchemaKey.java @@ -27,7 +27,6 @@ import org.neo4j.values.storable.Values; import static java.lang.String.format; -import static org.neo4j.values.storable.UTF8StringValue.codePointByteArrayCompare; /** * Includes value and entity id (to be able to handle non-unique values). A value can be any {@link String}, @@ -39,8 +38,6 @@ class StringSchemaKey extends NativeSchemaKey private boolean ignoreLength; - // TODO something better or? - // TODO this is UTF-8 bytes for now byte[] bytes; int size() @@ -59,6 +56,13 @@ protected Value assertCorrectType( Value value ) return value; } + @Override + void initialize( long entityId ) + { + super.initialize( entityId ); + ignoreLength = false; + } + @Override public Value asValue() { @@ -80,15 +84,15 @@ void initValueAsHighest() void initAsPrefixLow( String prefix ) { writeString( prefix ); - setEntityId( Long.MIN_VALUE ); - setCompareId( DEFAULT_COMPARE_ID ); + initialize( Long.MIN_VALUE ); + // Don't set ignoreLength = true here since the "low" a.k.a. left side of the range should care about length. + // This will make the prefix lower than those that matches the prefix (their length is >= that of the prefix) } void initAsPrefixHigh( String prefix ) { writeString( prefix ); - setEntityId( Long.MAX_VALUE ); - setCompareId( DEFAULT_COMPARE_ID ); + initialize( Long.MAX_VALUE ); ignoreLength = true; } @@ -107,7 +111,6 @@ private boolean isHighest() @Override int compareValueTo( StringSchemaKey other ) { - // TODO cover all cases of bytes == null and special tie breaker and document if ( bytes != other.bytes ) { if ( bytes == null ) @@ -124,20 +127,10 @@ int compareValueTo( StringSchemaKey other ) return 0; } - try - { - // TODO change to not throw - return codePointByteArrayCompare( bytes, other.bytes, ignoreLength || other.ignoreLength ); - } - catch ( Exception e ) - { - // We can not throw here because we will visit this method inside a pageCursor.shouldRetry() block. - // Just return a comparison that at least will be commutative. - return byteArrayCompare( bytes, other.bytes ); - } + return byteArrayCompare( bytes, other.bytes, ignoreLength | other.ignoreLength ); } - private static int byteArrayCompare( byte[] a, byte[] b ) + private static int byteArrayCompare( byte[] a, byte[] b, boolean ignoreLength ) { assert a != null && b != null : "Null arrays not supported."; @@ -149,14 +142,14 @@ private static int byteArrayCompare( byte[] a, byte[] b ) int length = Math.min( a.length, b.length ); for ( int i = 0; i < length; i++ ) { - int compare = Byte.compare( a[i], b[i] ); + int compare = Short.compare( (short) (a[i] & 0xFF), (short) (b[i] & 0xFF) ); if ( compare != 0 ) { return compare; } } - return Integer.compare( a.length, b.length ); + return ignoreLength ? 0 : Integer.compare( a.length, b.length ); } @Override