diff --git a/community/io/src/main/java/org/neo4j/io/pagecache/impl/muninn/MuninnPageCache.java b/community/io/src/main/java/org/neo4j/io/pagecache/impl/muninn/MuninnPageCache.java index 2d983a0bf7be3..fc05f2c8a93ef 100644 --- a/community/io/src/main/java/org/neo4j/io/pagecache/impl/muninn/MuninnPageCache.java +++ b/community/io/src/main/java/org/neo4j/io/pagecache/impl/muninn/MuninnPageCache.java @@ -864,6 +864,7 @@ int evictPages( int pageCountToEvict, int clockArm, EvictionRunEvent evictionRun private boolean evictPage( MuninnPage page, EvictionEvent evictionEvent ) { + //noinspection TryWithIdenticalCatches - this warning is a false positive; bug in Intellij inspection try { page.evict( evictionEvent ); @@ -972,14 +973,14 @@ private long flushAtIORatio( double ratio ) // counters fast enough to reach zero. // Skip the page if it is already write locked, or not dirty, or too popular. - boolean thisPageIsDirty = false; + boolean thisPageIsDirty; if ( !(thisPageIsDirty = page.isDirty()) || !page.decrementUsage() ) { seenDirtyPages |= thisPageIsDirty; continue; // Continue looping to the next page. } - if ( page.tryExclusiveLock() ) // TODO somehow avoid taking these exclusive locks, that we currently need to avoid racing with other flushes + if ( page.tryFreezeLock() ) { try { @@ -1005,7 +1006,7 @@ private long flushAtIORatio( double ratio ) } finally { - page.unlockExclusive(); + page.unlockFreeze(); } } 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 3ea310afe58cc..67a46398306cd 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 @@ -69,7 +69,7 @@ final class MuninnPagedFile implements PagedFile * The layout looks like this: * * ┏━ Empty file marker bit. When 1, the file is empty. - * ┃ ┏━ Reference count, 15 bites. + * ┃ ┏━ Reference count, 15 bits. * ┃ ┃ ┏━ 48 bits for the last page id. * ┃┏━━━┻━━━━━━━━━━┓ ┏━━━┻━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┓ * MRRRRRRR RRRRRRRR IIIIIIII IIIIIIII IIIIIIII IIIIIIII IIIIIIII IIIIIIII @@ -225,7 +225,7 @@ void flushAndForceInternal( FlushEventOpportunity flushOpportunity, boolean forC if ( element instanceof MuninnPage ) { MuninnPage page = (MuninnPage) element; - if ( !(forClosing? page.tryExclusiveLock() : page.tryWriteLock()) ) + if ( !(forClosing? page.tryExclusiveLock() : page.tryFreezeLock()) ) { continue; } @@ -244,7 +244,7 @@ else if ( forClosing ) } else { - page.unlockWrite(); + page.unlockFreeze(); } } break; @@ -322,7 +322,7 @@ private int vectoredFlush( } else { - pages[j].unlockWrite(); + pages[j].unlockFreeze(); } } } diff --git a/community/io/src/main/java/org/neo4j/io/pagecache/impl/muninn/SequenceLock.java b/community/io/src/main/java/org/neo4j/io/pagecache/impl/muninn/SequenceLock.java index b2bc5e4474903..ef2fb37b3af6e 100644 --- a/community/io/src/main/java/org/neo4j/io/pagecache/impl/muninn/SequenceLock.java +++ b/community/io/src/main/java/org/neo4j/io/pagecache/impl/muninn/SequenceLock.java @@ -42,19 +42,38 @@ */ public class SequenceLock { - // Bits for counting concurrent write-locks. We use 17 bits because our pages are most likely 8192 bytes, and - // 2^17 = 131.072, which is far more than our page size, so makes it highly unlikely that we are going to overflow - // our concurrent write lock counter. Meanwhile, it's also small enough that we have a very large (2^46) number - // space for our sequence. + /* + * Bits for counting concurrent write-locks. We use 17 bits because our pages are most likely 8192 bytes, and + * 2^17 = 131.072, which is far more than our page size, so makes it highly unlikely that we are going to overflow + * our concurrent write lock counter. Meanwhile, it's also small enough that we have a very large (2^45) number + * space for our sequence. This one value controls the layout of the lock bit-state. The rest of the layout is + * derived from this. + * + * With 17 writer count bits, the layout looks like this: + * + * ┏━ Freeze lock bit + * ┃┏━ Exclusive lock bit + * ┃┃ ┏━ Count of currently concurrently held write locks, 17 bits. + * ┃┃ ┃ ┏━ 45 bits for the read lock sequence, incremented on write & exclusive unlock. + * ┃┃┏━━━┻━━━━━━━━━━━━━┓┏━━━┻━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┓ + * FEWWWWWW WWWWWWWW WWWSSSSS SSSSSSSS SSSSSSSS SSSSSSSS SSSSSSSS SSSSSSSS + * 1 2 3 4 5 6 7 8 byte + */ private static final long CNT_BITS = 17; private static final long BITS_IN_LONG = 64; - private static final long SEQ_BITS = BITS_IN_LONG - 1 - CNT_BITS; + private static final long EXL_LOCK_BITS = 1; + private static final long FRZ_LOCK_BITS = 1; + private static final long SEQ_BITS = BITS_IN_LONG - FRZ_LOCK_BITS - EXL_LOCK_BITS - CNT_BITS; private static final long CNT_UNIT = 1L << SEQ_BITS; private static final long SEQ_MASK = CNT_UNIT - 1L; private static final long SEQ_IMSK = ~SEQ_MASK; private static final long CNT_MASK = ((1L << CNT_BITS) - 1L) << SEQ_BITS; - private static final long EXCL_MASK = (1L << CNT_BITS + SEQ_BITS); + private static final long EXL_MASK = (1L << CNT_BITS + SEQ_BITS); + private static final long FRZ_MASK = (1L << CNT_BITS + SEQ_BITS + 1L); + private static final long FRZ_IMSK = ~FRZ_MASK; + private static final long FAE_MASK = FRZ_MASK + EXL_MASK; // "freeze and/or exclusive" mask + private static final long UNL_MASK = FAE_MASK + CNT_MASK; // unlocked mask private static final long STATE = UnsafeUtil.getFieldOffset( SequenceLock.class, "state" ); @@ -71,6 +90,11 @@ private boolean compareAndSetState( long expect, long update ) return UnsafeUtil.compareAndSwapLong( this, STATE, expect, update ); } + private void unconditionallySetState( long update ) + { + state = update; + } + /** * Start an optimistic critical section, and return a stamp that can be used to validate if the read-lock was * consistent. That is, if no write or exclusive lock was overlapping with the optimistic read-lock. @@ -93,7 +117,7 @@ public long tryOptimisticReadLock() public boolean validateReadLock( long stamp ) { UnsafeUtil.loadFence(); - return getState() == stamp; + return (getState() & FRZ_IMSK) == stamp; } /** @@ -111,14 +135,15 @@ public boolean tryWriteLock() for (; ; ) { s = getState(); - if ( (s & EXCL_MASK) == EXCL_MASK ) - { - return false; - } - if ( (s & CNT_MASK) == CNT_MASK ) + boolean unwritablyLocked = (s & FAE_MASK) != 0; + boolean writeCountOverflow = (s & CNT_MASK) == CNT_MASK; + + // bitwise-OR to reduce branching and allow more ILP + if ( unwritablyLocked | writeCountOverflow ) { - throwWriteLockOverflow( s ); + return failWriteLock( s, writeCountOverflow ); } + n = s + CNT_UNIT; if ( compareAndSetState( s, n ) ) { @@ -127,6 +152,16 @@ public boolean tryWriteLock() } } + private boolean failWriteLock( long s, boolean writeCountOverflow ) + { + if ( writeCountOverflow ) + { + throwWriteLockOverflow( s ); + } + // Otherwise it was either exclusively or freeze locked + return false; + } + private long throwWriteLockOverflow( long s ) { throw new IllegalMonitorStateException( "Write lock counter overflow: " + describeState( s ) ); @@ -172,7 +207,7 @@ private long nextSeq( long s ) public boolean tryExclusiveLock() { long s = getState(); - return ((s & CNT_MASK) == 0) & ((s & EXCL_MASK) == 0) && compareAndSetState( s, s + EXCL_MASK ); + return ((s & UNL_MASK) == 0) && compareAndSetState( s, s + EXL_MASK ); } /** @@ -184,8 +219,9 @@ public boolean tryExclusiveLock() public long unlockExclusive() { long s = initiateExclusiveLockRelease(); - long n = nextSeq( s ) - EXCL_MASK; - compareAndSetState( s, n ); + long n = nextSeq( s ) - EXL_MASK; + // Exclusive locks prevent any state modifications from write locks + unconditionallySetState( n ); return n; } @@ -195,14 +231,14 @@ public long unlockExclusive() public void unlockExclusiveAndTakeWriteLock() { long s = initiateExclusiveLockRelease(); - long n = nextSeq( s ) - EXCL_MASK + CNT_UNIT; - compareAndSetState( s, n ); + long n = nextSeq( s ) - EXL_MASK + CNT_UNIT; + unconditionallySetState( n ); } private long initiateExclusiveLockRelease() { long s = getState(); - if ( (s & EXCL_MASK) != EXCL_MASK ) + if ( (s & EXL_MASK) != EXL_MASK ) { throwUnmatchedUnlockExclusive( s ); } @@ -214,6 +250,30 @@ private void throwUnmatchedUnlockExclusive( long s ) throw new IllegalMonitorStateException( "Unmatched unlockExclusive: " + describeState( s ) ); } + public boolean tryFreezeLock() + { + long s = getState(); + return ((s & UNL_MASK) == 0) && compareAndSetState( s, s + FRZ_MASK ); + } + + public void unlockFreeze() + { + long s = getState(); + if ( (s & FRZ_MASK) != FRZ_MASK ) + { + throwUnmatchedUnlockFreeze( s ); + } + // We don't increment the sequence with nextSeq here, because freeze locks don't invalidate readers + long n = s - FRZ_MASK; + // Freeze locks prevent any state modifications from write and exclusive locks + unconditionallySetState( n ); + } + + private void throwUnmatchedUnlockFreeze( long s ) + { + throw new IllegalMonitorStateException( "Unmatched unlockFreeze: " + describeState( s ) ); + } + @Override public String toString() { diff --git a/community/io/src/test/java/org/neo4j/io/pagecache/impl/muninn/SequenceLockTest.java b/community/io/src/test/java/org/neo4j/io/pagecache/impl/muninn/SequenceLockTest.java index f02b04e5b2521..3dffe43151ac6 100644 --- a/community/io/src/test/java/org/neo4j/io/pagecache/impl/muninn/SequenceLockTest.java +++ b/community/io/src/test/java/org/neo4j/io/pagecache/impl/muninn/SequenceLockTest.java @@ -38,7 +38,7 @@ public class SequenceLockTest { private static final long TIMEOUT = 5000; - private static final ExecutorService executor = Executors.newCachedThreadPool(new DaemonThreadFactory()); + private static final ExecutorService executor = Executors.newCachedThreadPool( new DaemonThreadFactory() ); @AfterClass public static void shutDownExecutor() @@ -62,7 +62,7 @@ public void mustNotValidateRandomStamp() throws Exception } @Test - public void writeLockMustInvalidateOptimisticLock() throws Exception + public void writeLockMustInvalidateOptimisticReadLock() throws Exception { long r = lock.tryOptimisticReadLock(); lock.tryWriteLock(); @@ -71,7 +71,7 @@ public void writeLockMustInvalidateOptimisticLock() throws Exception } @Test - public void takingWriteLockMustInvalidateOptimisticLock() throws Exception + public void takingWriteLockMustInvalidateOptimisticReadLock() throws Exception { long r = lock.tryOptimisticReadLock(); lock.tryWriteLock(); @@ -147,8 +147,8 @@ public void unmatchedUnlockWriteLockMustThrow() throws Exception @Test( expected = IllegalMonitorStateException.class, timeout = TIMEOUT ) public void writeLockCountOverflowMustThrow() throws Exception { - // TODO its possible we might want to spin-yield or fail instead of throwing, hoping someone will give us a lock - for ( ;; ) + //noinspection InfiniteLoopStatement + for (; ; ) { assertTrue( lock.tryWriteLock() ); } @@ -338,6 +338,153 @@ public void stampFromUnlockExclusiveMustNotBeValidIfThereAreWriteLocks() throws assertFalse( lock.validateReadLock( r ) ); } + @Test + public void uncontendedFreezeLockMustBeAvailable() throws Exception + { + assertTrue( lock.tryFreezeLock() ); + } + + @Test + public void freezeLockMustNotInvalidateOptimisticReadLock() throws Exception + { + long r = lock.tryOptimisticReadLock(); + lock.tryFreezeLock(); + lock.unlockFreeze(); + assertTrue( lock.validateReadLock( r ) ); + } + + @Test + public void freezeLockMustFailWriteLock() throws Exception + { + lock.tryFreezeLock(); + assertFalse( lock.tryWriteLock() ); + } + + @Test + public void freezeLockMustFailExclusiveLock() throws Exception + { + lock.tryFreezeLock(); + assertFalse( lock.tryExclusiveLock() ); + } + + @Test + public void cannotTakeFreezeLockIfAlreadyTaken() throws Exception + { + assertTrue( lock.tryFreezeLock() ); + assertFalse( lock.tryFreezeLock() ); + } + + @Test + public void writeLockMustFailFreezeLock() throws Exception + { + lock.tryWriteLock(); + assertFalse( lock.tryFreezeLock() ); + } + + @Test + public void exclusiveLockMustFailFreezeLock() throws Exception + { + lock.tryExclusiveLock(); + assertFalse( lock.tryFreezeLock() ); + } + + @Test + public void unlockExclusiveAndTakeWriteLockMustFailFreezeLock() throws Exception + { + lock.tryExclusiveLock(); + lock.unlockExclusiveAndTakeWriteLock(); + assertFalse( lock.tryFreezeLock() ); + } + + @Test + public void freezeUnlockMustNotInvalidateOptimisticReadLock() throws Exception + { + long r = lock.tryOptimisticReadLock(); + assertTrue( lock.tryFreezeLock() ); + assertTrue( lock.validateReadLock( r ) ); + } + + @Test + public void optimisticReadLockMustValidateUnderFreezeLock() throws Exception + { + lock.tryFreezeLock(); + long r = lock.tryOptimisticReadLock(); + assertTrue( lock.validateReadLock( r ) ); + } + + @Test + public void freezeLockReleaseMustNotInvalidateOptimisticReadLock() throws Exception + { + lock.tryFreezeLock(); + long r = lock.tryOptimisticReadLock(); + lock.unlockFreeze(); + assertTrue( lock.validateReadLock( r ) ); + } + + @Test( expected = IllegalMonitorStateException.class ) + public void unmatchedUnlockFreezeMustThrow() throws Exception + { + lock.unlockFreeze(); + } + + @Test + public void uncontendedOptimisticReadLockMustBeAvailableAfterFreezeLock() throws Exception + { + lock.tryFreezeLock(); + lock.unlockFreeze(); + long r = lock.tryOptimisticReadLock(); + assertTrue( lock.validateReadLock( r ) ); + } + + @Test + public void uncontendedWriteLockMustBeAvailableAfterFreezeLock() throws Exception + { + lock.tryFreezeLock(); + lock.unlockFreeze(); + assertTrue( lock.tryWriteLock() ); + } + + @Test + public void uncontendedExclusiveLockMustBeAvailableAfterFreezeLock() throws Exception + { + lock.tryFreezeLock(); + lock.unlockFreeze(); + assertTrue( lock.tryExclusiveLock() ); + } + + @Test + public void uncontendedFreezeLockMustBeAvailableAfterWriteLock() throws Exception + { + lock.tryWriteLock(); + lock.unlockWrite(); + assertTrue( lock.tryFreezeLock() ); + } + + @Test + public void uncontendedFreezeLockMustBeAvailableAfterExclusiveLock() throws Exception + { + lock.tryExclusiveLock(); + lock.unlockExclusive(); + assertTrue( lock.tryFreezeLock() ); + } + + @Test + public void uncontendedFreezeLockMustBeAvailableAfterFreezeLock() throws Exception + { + lock.tryFreezeLock(); + lock.unlockFreeze(); + assertTrue( lock.tryFreezeLock() ); + } + + @Test + public void stampFromUnlockExclusiveMustBeValidUnderFreezeLock() throws Exception + { + lock.tryExclusiveLock(); + long r = lock.unlockExclusive(); + lock.tryFreezeLock(); + assertTrue( lock.validateReadLock( r ) ); + } + @Test public void toStringMustDescribeState() throws Exception {