From 1754caaa65f1b819492628df87ee5d737a6eef08 Mon Sep 17 00:00:00 2001 From: MishaDemianenko Date: Tue, 23 Jan 2018 19:25:36 +0100 Subject: [PATCH] Update new fields access to use unsafe only, tests for newly introduced helper methods --- .../io/pagecache/impl/muninn/MuninnPage.java | 28 +++---- .../impl/muninn/MuninnPageCursor.java | 20 ++++- .../muninn/MuninnPageEvictionCallback.java | 2 +- .../impl/muninn/MuninnPagedFile.java | 14 +--- .../impl/internal/dragons/UnsafeUtil.java | 26 ++++++ .../impl/internal/dragons/UnsafeUtilTest.java | 81 ++++++++++++++++++- 6 files changed, 135 insertions(+), 36 deletions(-) diff --git a/community/io/src/main/java/org/neo4j/io/pagecache/impl/muninn/MuninnPage.java b/community/io/src/main/java/org/neo4j/io/pagecache/impl/muninn/MuninnPage.java index d2fe0eae6ba3a..8815131b1628f 100644 --- a/community/io/src/main/java/org/neo4j/io/pagecache/impl/muninn/MuninnPage.java +++ b/community/io/src/main/java/org/neo4j/io/pagecache/impl/muninn/MuninnPage.java @@ -51,7 +51,7 @@ final class MuninnPage extends SequenceLock implements Page private long pointer; // max transaction id that updated this page - @SuppressWarnings( "unused" ) + @SuppressWarnings( "unused" ) // accessed using unsafe private volatile long lastModifiedTxId = UNBOUND_LAST_MODIFIED_TX_ID; // Optimistically incremented; occasionally truncated to a max of 4. @@ -91,30 +91,22 @@ public long address() return pointer; } - public long getLastModifiedTxId() + long getLastModifiedTxId() { - return lastModifiedTxId; + return UnsafeUtil.getLongVolatile( this, lastModifiedTxIdOffset ); } - public long resetLastModifiedTxId() + /** + * @return return current value of {@link #lastModifiedTxId} and resets it to {@link #UNBOUND_LAST_MODIFIED_TX_ID} + */ + long getAndResetLastModifiedTransactionId() { - long value = lastModifiedTxId; - lastModifiedTxId = UNBOUND_LAST_MODIFIED_TX_ID; - return value; + return UnsafeUtil.getAndSetLong( this, lastModifiedTxIdOffset, UNBOUND_LAST_MODIFIED_TX_ID ); } - public void setLastModifiedTxId( long modifierTxId ) + void setLastModifiedTxId( long modifierTxId ) { - long pageLatestModifier; - do - { - pageLatestModifier = this.lastModifiedTxId; - if ( pageLatestModifier >= modifierTxId ) - { - return; - } - } - while ( !UnsafeUtil.compareAndSwapLong( this, lastModifiedTxIdOffset, pageLatestModifier, modifierTxId ) ); + UnsafeUtil.compareAndSetMaxLong( this, lastModifiedTxIdOffset, modifierTxId ); } /** diff --git a/community/io/src/main/java/org/neo4j/io/pagecache/impl/muninn/MuninnPageCursor.java b/community/io/src/main/java/org/neo4j/io/pagecache/impl/muninn/MuninnPageCursor.java index e2e1841906816..09118e6e18ecc 100644 --- a/community/io/src/main/java/org/neo4j/io/pagecache/impl/muninn/MuninnPageCursor.java +++ b/community/io/src/main/java/org/neo4j/io/pagecache/impl/muninn/MuninnPageCursor.java @@ -131,17 +131,31 @@ public final boolean next( long pageId ) throws IOException void verifyContext() { VersionContext versionContext = versionContextSupplier.getVersionContext(); - if ( versionContext.lastClosedTransactionId() == Long.MAX_VALUE ) + long lastClosedTransactionId = versionContext.lastClosedTransactionId(); + if ( lastClosedTransactionId == Long.MAX_VALUE ) { return; } - if ( page.getLastModifiedTxId() > versionContext.lastClosedTransactionId() || - pagedFile.getHighestEvictedTransactionId() > versionContext.lastClosedTransactionId() ) + if ( isPotentiallyReadingDirtyData( lastClosedTransactionId ) ) { versionContext.markAsDirty(); } } + /** + * We reading potentially dirty data in case if our page last modification version is higher then + * requested lastClosedTransactionId; or for this page file we already evict some page with version that is higher + * then requested lastClosedTransactionId. In this case we can't be sure that data of current page satisfying + * visibility requirements and we pessimistically will assume that we reading dirty data. + * @param lastClosedTransactionId last closed transaction id + * @return true in case if we reading potentially dirty data for requested lastClosedTransactionId. + */ + private boolean isPotentiallyReadingDirtyData( long lastClosedTransactionId ) + { + return page.getLastModifiedTxId() > lastClosedTransactionId || + pagedFile.getHighestEvictedTransactionId() > lastClosedTransactionId; + } + @Override public final void close() { diff --git a/community/io/src/main/java/org/neo4j/io/pagecache/impl/muninn/MuninnPageEvictionCallback.java b/community/io/src/main/java/org/neo4j/io/pagecache/impl/muninn/MuninnPageEvictionCallback.java index 56b17075bb18d..e55a8362bd27d 100644 --- a/community/io/src/main/java/org/neo4j/io/pagecache/impl/muninn/MuninnPageEvictionCallback.java +++ b/community/io/src/main/java/org/neo4j/io/pagecache/impl/muninn/MuninnPageEvictionCallback.java @@ -38,6 +38,6 @@ public void onEvict( long filePageId, Page page ) assert removed == page : "Removed unexpected page when cleaning up translation table for filePageId " + filePageId + ". " + "Evicted " + page + " but removed " + removed + " from the translation table."; - file.setHighestEvictedTransactionId( removed.resetLastModifiedTxId() ); + file.setHighestEvictedTransactionId( removed.getAndResetLastModifiedTransactionId() ); } } diff --git a/community/io/src/main/java/org/neo4j/io/pagecache/impl/muninn/MuninnPagedFile.java b/community/io/src/main/java/org/neo4j/io/pagecache/impl/muninn/MuninnPagedFile.java index 0af926b77a97a..6e0f553be486c 100644 --- a/community/io/src/main/java/org/neo4j/io/pagecache/impl/muninn/MuninnPagedFile.java +++ b/community/io/src/main/java/org/neo4j/io/pagecache/impl/muninn/MuninnPagedFile.java @@ -77,6 +77,7 @@ final class MuninnPagedFile implements PagedFile, Flushable // max modifier transaction id among evicted pages for this file private static final long evictedTransactionIdOffset = UnsafeUtil.getFieldOffset( MuninnPagedFile.class, "highestEvictedTransactionId" ); + @SuppressWarnings( "unused" ) // accessed using unsafe private volatile long highestEvictedTransactionId; /** @@ -612,21 +613,12 @@ MuninnPage evictPage( long filePageId ) void setHighestEvictedTransactionId( long modifiedTransactionId ) { - long highestModifier; - do - { - highestModifier = this.highestEvictedTransactionId; - if ( highestModifier >= modifiedTransactionId ) - { - return; - } - } - while ( !UnsafeUtil.compareAndSwapLong( this, evictedTransactionIdOffset, highestModifier, modifiedTransactionId ) ); + UnsafeUtil.compareAndSetMaxLong( this, evictedTransactionIdOffset, modifiedTransactionId ); } long getHighestEvictedTransactionId() { - return highestEvictedTransactionId; + return UnsafeUtil.getLongVolatile( this, evictedTransactionIdOffset ); } /** diff --git a/community/unsafe/src/main/java/org/neo4j/unsafe/impl/internal/dragons/UnsafeUtil.java b/community/unsafe/src/main/java/org/neo4j/unsafe/impl/internal/dragons/UnsafeUtil.java index 7df8799fac2b7..0eaf8b8db0acb 100644 --- a/community/unsafe/src/main/java/org/neo4j/unsafe/impl/internal/dragons/UnsafeUtil.java +++ b/community/unsafe/src/main/java/org/neo4j/unsafe/impl/internal/dragons/UnsafeUtil.java @@ -307,6 +307,32 @@ public static Object getAndSetObject( Object obj, long offset, Object newValue ) return unsafe.getAndSetObject( obj, offset, newValue ); } + /** + * Atomically exchanges provided newValue with the current value of field or array element, with + * provided offset. + */ + public static long getAndSetLong( Object object, long offset, long newValue ) + { + return unsafe.getAndSetLong( object, offset, newValue ); + } + + /** + * Atomically set field or array element to a maximum between current value and provided newValue + */ + public static void compareAndSetMaxLong( Object object, long fieldOffset, long newValue ) + { + long currentValue; + do + { + currentValue = UnsafeUtil.getLong( object, fieldOffset ); + if ( currentValue >= newValue ) + { + return; + } + } + while ( !UnsafeUtil.compareAndSwapLong( object, fieldOffset, currentValue, newValue ) ); + } + /** * Create a string with a char[] that you know is not going to be modified, so avoid the copy constructor. * diff --git a/community/unsafe/src/test/java/org/neo4j/unsafe/impl/internal/dragons/UnsafeUtilTest.java b/community/unsafe/src/test/java/org/neo4j/unsafe/impl/internal/dragons/UnsafeUtilTest.java index 60476c31f6d71..fb55a92a5a04c 100644 --- a/community/unsafe/src/test/java/org/neo4j/unsafe/impl/internal/dragons/UnsafeUtilTest.java +++ b/community/unsafe/src/test/java/org/neo4j/unsafe/impl/internal/dragons/UnsafeUtilTest.java @@ -24,6 +24,8 @@ import java.nio.ByteBuffer; import java.util.Objects; +import static java.lang.System.currentTimeMillis; +import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.isOneOf; import static org.hamcrest.Matchers.not; @@ -33,9 +35,59 @@ import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertThat; import static org.junit.Assert.assertTrue; -import static java.lang.System.currentTimeMillis; - -import static org.neo4j.unsafe.impl.internal.dragons.UnsafeUtil.*; +import static org.neo4j.unsafe.impl.internal.dragons.UnsafeUtil.allocateMemory; +import static org.neo4j.unsafe.impl.internal.dragons.UnsafeUtil.arrayBaseOffset; +import static org.neo4j.unsafe.impl.internal.dragons.UnsafeUtil.arrayIndexScale; +import static org.neo4j.unsafe.impl.internal.dragons.UnsafeUtil.arrayOffset; +import static org.neo4j.unsafe.impl.internal.dragons.UnsafeUtil.assertHasUnsafe; +import static org.neo4j.unsafe.impl.internal.dragons.UnsafeUtil.compareAndSetMaxLong; +import static org.neo4j.unsafe.impl.internal.dragons.UnsafeUtil.compareAndSwapLong; +import static org.neo4j.unsafe.impl.internal.dragons.UnsafeUtil.compareAndSwapObject; +import static org.neo4j.unsafe.impl.internal.dragons.UnsafeUtil.free; +import static org.neo4j.unsafe.impl.internal.dragons.UnsafeUtil.getAndAddInt; +import static org.neo4j.unsafe.impl.internal.dragons.UnsafeUtil.getAndSetLong; +import static org.neo4j.unsafe.impl.internal.dragons.UnsafeUtil.getAndSetObject; +import static org.neo4j.unsafe.impl.internal.dragons.UnsafeUtil.getBoolean; +import static org.neo4j.unsafe.impl.internal.dragons.UnsafeUtil.getBooleanVolatile; +import static org.neo4j.unsafe.impl.internal.dragons.UnsafeUtil.getByte; +import static org.neo4j.unsafe.impl.internal.dragons.UnsafeUtil.getByteVolatile; +import static org.neo4j.unsafe.impl.internal.dragons.UnsafeUtil.getChar; +import static org.neo4j.unsafe.impl.internal.dragons.UnsafeUtil.getCharVolatile; +import static org.neo4j.unsafe.impl.internal.dragons.UnsafeUtil.getDouble; +import static org.neo4j.unsafe.impl.internal.dragons.UnsafeUtil.getDoubleVolatile; +import static org.neo4j.unsafe.impl.internal.dragons.UnsafeUtil.getFieldOffset; +import static org.neo4j.unsafe.impl.internal.dragons.UnsafeUtil.getFloat; +import static org.neo4j.unsafe.impl.internal.dragons.UnsafeUtil.getFloatVolatile; +import static org.neo4j.unsafe.impl.internal.dragons.UnsafeUtil.getInt; +import static org.neo4j.unsafe.impl.internal.dragons.UnsafeUtil.getIntVolatile; +import static org.neo4j.unsafe.impl.internal.dragons.UnsafeUtil.getLong; +import static org.neo4j.unsafe.impl.internal.dragons.UnsafeUtil.getLongVolatile; +import static org.neo4j.unsafe.impl.internal.dragons.UnsafeUtil.getObject; +import static org.neo4j.unsafe.impl.internal.dragons.UnsafeUtil.getObjectVolatile; +import static org.neo4j.unsafe.impl.internal.dragons.UnsafeUtil.getShort; +import static org.neo4j.unsafe.impl.internal.dragons.UnsafeUtil.getShortVolatile; +import static org.neo4j.unsafe.impl.internal.dragons.UnsafeUtil.initDirectByteBuffer; +import static org.neo4j.unsafe.impl.internal.dragons.UnsafeUtil.newDirectByteBuffer; +import static org.neo4j.unsafe.impl.internal.dragons.UnsafeUtil.pageSize; +import static org.neo4j.unsafe.impl.internal.dragons.UnsafeUtil.putBoolean; +import static org.neo4j.unsafe.impl.internal.dragons.UnsafeUtil.putBooleanVolatile; +import static org.neo4j.unsafe.impl.internal.dragons.UnsafeUtil.putByte; +import static org.neo4j.unsafe.impl.internal.dragons.UnsafeUtil.putByteVolatile; +import static org.neo4j.unsafe.impl.internal.dragons.UnsafeUtil.putChar; +import static org.neo4j.unsafe.impl.internal.dragons.UnsafeUtil.putCharVolatile; +import static org.neo4j.unsafe.impl.internal.dragons.UnsafeUtil.putDouble; +import static org.neo4j.unsafe.impl.internal.dragons.UnsafeUtil.putDoubleVolatile; +import static org.neo4j.unsafe.impl.internal.dragons.UnsafeUtil.putFloat; +import static org.neo4j.unsafe.impl.internal.dragons.UnsafeUtil.putFloatVolatile; +import static org.neo4j.unsafe.impl.internal.dragons.UnsafeUtil.putInt; +import static org.neo4j.unsafe.impl.internal.dragons.UnsafeUtil.putIntVolatile; +import static org.neo4j.unsafe.impl.internal.dragons.UnsafeUtil.putLong; +import static org.neo4j.unsafe.impl.internal.dragons.UnsafeUtil.putLongVolatile; +import static org.neo4j.unsafe.impl.internal.dragons.UnsafeUtil.putObject; +import static org.neo4j.unsafe.impl.internal.dragons.UnsafeUtil.putObjectVolatile; +import static org.neo4j.unsafe.impl.internal.dragons.UnsafeUtil.putShort; +import static org.neo4j.unsafe.impl.internal.dragons.UnsafeUtil.putShortVolatile; +import static org.neo4j.unsafe.impl.internal.dragons.UnsafeUtil.setMemory; public class UnsafeUtilTest { @@ -347,6 +399,29 @@ public void getAndSetObjectField() throws Exception assertThat( obj, is( new Obj() ) ); } + @Test + public void getAndSetLongField() + { + Obj obj = new Obj(); + long offset = getFieldOffset( Obj.class, "aLong" ); + assertThat( getAndSetLong( obj, offset, 42L ), equalTo( 0L ) ); + assertThat( getAndSetLong( obj, offset, -1 ), equalTo( 42L ) ); + } + + @Test + public void compareAndSetMaxLongField() + { + Obj obj = new Obj(); + long offset = getFieldOffset( Obj.class, "aLong" ); + assertThat( getAndSetLong( obj, offset, 42L ), equalTo( 0L ) ); + + compareAndSetMaxLong( obj, offset, 5 ); + assertEquals( 42, getLong( obj, offset ) ); + + compareAndSetMaxLong( obj, offset, 105 ); + assertEquals( 105, getLong( obj, offset ) ); + } + @Test public void unsafeArrayElementAccess() throws Exception {