From f61ca22b2ce79b99731b252289acaee0e65f2c39 Mon Sep 17 00:00:00 2001 From: Chris Vest Date: Thu, 22 Dec 2016 10:25:09 +0100 Subject: [PATCH] More work on eviction via PageList --- .../impl/muninn/OffHeapPageLock.java | 35 +++- .../io/pagecache/impl/muninn/PageList.java | 40 ++++- .../pagecache/impl/muninn/PageListTest.java | 157 +++++++++++++++++- 3 files changed, 217 insertions(+), 15 deletions(-) diff --git a/community/io/src/main/java/org/neo4j/io/pagecache/impl/muninn/OffHeapPageLock.java b/community/io/src/main/java/org/neo4j/io/pagecache/impl/muninn/OffHeapPageLock.java index 3681ff2d88194..f6db16213aaee 100644 --- a/community/io/src/main/java/org/neo4j/io/pagecache/impl/muninn/OffHeapPageLock.java +++ b/community/io/src/main/java/org/neo4j/io/pagecache/impl/muninn/OffHeapPageLock.java @@ -122,7 +122,7 @@ private static void unconditionallySetState( long address, long 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. * - * @return A stamp that must be passed to {@link #validateReadLock(long,long)} to validate the critical section. + * @return A stamp that must be passed to {@link #validateReadLock(long, long)} to validate the critical section. */ public static long tryOptimisticReadLock( long address ) { @@ -130,9 +130,9 @@ public static long tryOptimisticReadLock( long address ) } /** - * Validate a stamp from {@link #tryOptimisticReadLock(long)} or {@link #unlockExclusive(long)}, and return {@code true} - * if no write or exclusive lock overlapped with the critical section of the optimistic read lock represented by - * the stamp. + * Validate a stamp from {@link #tryOptimisticReadLock(long)} or {@link #unlockExclusive(long)}, and return + * {@code true} if no write or exclusive lock overlapped with the critical section of the optimistic read lock + * represented by the stamp. * * @param stamp The stamp of the optimistic read lock. * @return {@code true} if the optimistic read lock was valid, {@code false} otherwise. @@ -165,7 +165,7 @@ public static boolean isExclusivelyLocked( long address ) public static boolean tryWriteLock( long address ) { long s, n; - for (; ; ) + for ( ; ; ) { s = getState( address ); boolean unwritablyLocked = (s & EXL_MASK) != 0; @@ -267,7 +267,7 @@ public static long unlockExclusive( long address ) public static void unlockExclusiveAndTakeWriteLock( long address ) { long s = initiateExclusiveLockRelease( address ); - long n = nextSeq( s ) - EXL_MASK + CNT_UNIT; + long n = (nextSeq( s ) - EXL_MASK + CNT_UNIT) | MOD_MASK; unconditionallySetState( address, n ); } @@ -286,6 +286,25 @@ private static void throwUnmatchedUnlockExclusive( long s ) throw new IllegalMonitorStateException( "Unmatched unlockExclusive: " + describeState( s ) ); } + /** + * If the given lock is exclusively held, then the modified flag will be explicitly lowered (marked as + * unmodified) if the modified is currently raised. + *

+ * If the modified flag is currently not raised, then this method does nothing. + * + * @throws IllegalStateException if the lock at the given address is not in the exclusively locked state. + */ + public static void explicitlyMarkPageUnmodifiedUnderExclusiveLock( long address ) + { + long s = getState( address ); + if ( (s & EXL_MASK) != EXL_MASK ) + { + throw new IllegalStateException( "Page must be exclusively locked to explicitly lower modified bit" ); + } + s = s & (~MOD_MASK); + unconditionallySetState( address, s ); + } + /** * Grab the flush lock if it is immediately available. Flush locks prevent overlapping exclusive locks, * but do not invalidate optimistic read locks, nor do they prevent overlapping write locks. Only one flush lock @@ -293,10 +312,10 @@ private static void throwUnmatchedUnlockExclusive( long s ) * fail. *

* Successfully grabbed flush locks must always be paired with a corresponding - * {@link #unlockFlush(long,long,boolean)}. + * {@link #unlockFlush(long, long, boolean)}. * * @return If the lock is successfully grabbed, the method will return a stamp value that must be passed to the - * {@link #unlockFlush(long,long,boolean)}, and which is used for detecting any overlapping write locks. If the + * {@link #unlockFlush(long, long, boolean)}, and which is used for detecting any overlapping write locks. If the * flush lock could not be taken, {@code 0} will be returned. */ public static long tryFlushLock( long address ) diff --git a/community/io/src/main/java/org/neo4j/io/pagecache/impl/muninn/PageList.java b/community/io/src/main/java/org/neo4j/io/pagecache/impl/muninn/PageList.java index 6d3d604d17d98..f3149299611e7 100644 --- a/community/io/src/main/java/org/neo4j/io/pagecache/impl/muninn/PageList.java +++ b/community/io/src/main/java/org/neo4j/io/pagecache/impl/muninn/PageList.java @@ -197,6 +197,11 @@ public void unlockFlush( long pageRef, long stamp, boolean success ) OffHeapPageLock.unlockFlush( offLock( pageRef ), stamp, success ); } + public void explicitlyMarkPageUnmodifiedUnderExclusiveLock( long pageRef ) + { + OffHeapPageLock.explicitlyMarkPageUnmodifiedUnderExclusiveLock( offLock( pageRef ) ); + } + public int getCachePageSize() { return cachePageSize; @@ -290,7 +295,7 @@ public void fault( long pageRef, PageSwapper swapper, int swapperId, long filePa { if ( swapper == null ) { - throw new IllegalArgumentException( "swapper cannot be null" ); + throw swapperCannotBeNull(); } int currentSwapper = getSwapperId( pageRef ); long currentFilePageId = getFilePageId( pageRef ); @@ -313,7 +318,12 @@ public void fault( long pageRef, PageSwapper swapper, int swapperId, long filePa setSwapperId( pageRef, swapperId ); // Page now considered isBoundTo( swapper, filePageId ) } - private IllegalStateException cannotFaultException( long pageRef, PageSwapper swapper, int swapperId, + private static IllegalArgumentException swapperCannotBeNull() + { + return new IllegalArgumentException( "swapper cannot be null" ); + } + + private static IllegalStateException cannotFaultException( long pageRef, PageSwapper swapper, int swapperId, long filePageId, int currentSwapper, long currentFilePageId ) { String msg = format( @@ -326,6 +336,32 @@ private IllegalStateException cannotFaultException( long pageRef, PageSwapper sw public boolean tryEvict( long pageRef, PageSwapper swapper ) { + if ( swapper == null ) + { + throw swapperCannotBeNull(); + } + if ( tryExclusiveLock( pageRef ) ) + { + if ( isLoaded( pageRef ) ) + { + clearBinding( pageRef ); + if ( isModified( pageRef ) ) + { + explicitlyMarkPageUnmodifiedUnderExclusiveLock( pageRef ); + } + return true; + } + else + { + unlockExclusive( pageRef ); + } + } return false; } + + private void clearBinding( long pageRef ) + { + setFilePageId( pageRef, PageCursor.UNBOUND_PAGE_ID ); + setSwapperId( pageRef, 0 ); + } } diff --git a/community/io/src/test/java/org/neo4j/io/pagecache/impl/muninn/PageListTest.java b/community/io/src/test/java/org/neo4j/io/pagecache/impl/muninn/PageListTest.java index b95a07da36f22..99d6dd270a210 100644 --- a/community/io/src/test/java/org/neo4j/io/pagecache/impl/muninn/PageListTest.java +++ b/community/io/src/test/java/org/neo4j/io/pagecache/impl/muninn/PageListTest.java @@ -37,6 +37,8 @@ import java.util.concurrent.Executors; import java.util.concurrent.Future; import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.atomic.AtomicLong; import java.util.function.LongFunction; import org.neo4j.io.ByteUnit; @@ -70,7 +72,8 @@ public class PageListTest public static Iterable parameters() { LongFunction toArray = x -> new Object[]{x}; - return () -> Arrays.stream( pageIds ).mapToObj( toArray ).iterator(); +// return () -> Arrays.stream( pageIds ).mapToObj( toArray ).iterator(); + return () -> Arrays.stream( new long[]{0} ).mapToObj( toArray ).iterator(); } private static ExecutorService executor; @@ -711,6 +714,17 @@ public void takingWriteLockMustRaiseModifiedFlag() throws Exception pageList.unlockWrite( pageRef ); } + @Test + public void turningExclusiveLockIntoWriteLockMustRaiseModifiedFlag() throws Exception + { + assertFalse( pageList.isModified( pageRef ) ); + assertTrue( pageList.tryExclusiveLock( pageRef ) ); + assertFalse( pageList.isModified( pageRef ) ); + pageList.unlockExclusiveAndTakeWriteLock( pageRef ); + assertTrue( pageList.isModified( pageRef ) ); + pageList.unlockWrite( pageRef ); + } + @Test public void releasingFlushLockMustLowerModifiedFlagIfSuccessful() throws Exception { @@ -804,6 +818,47 @@ public void writeLockMustNotInterfereWithAdjacentModifiedFlags() throws Exceptio assertFalse( pageList.isModified( nextPageRef ) ); } + @Test( expected = IllegalStateException.class ) + public void disallowUnlockedPageToExplicitlyLowerModifiedFlag() throws Exception + { + pageList.explicitlyMarkPageUnmodifiedUnderExclusiveLock( pageRef ); + } + + @Test( expected = IllegalStateException.class ) + public void disallowReadLockedPageToExplicitlyLowerModifiedFlag() throws Exception + { + pageList.tryOptimisticReadLock( pageRef ); + pageList.explicitlyMarkPageUnmodifiedUnderExclusiveLock( pageRef ); + } + + @Test( expected = IllegalStateException.class ) + public void disallowFlushLockedPageToExplicitlyLowerModifiedFlag() throws Exception + { + assertThat( pageList.tryFlushLock( pageRef ), is( not( 0L ) ) ); + pageList.explicitlyMarkPageUnmodifiedUnderExclusiveLock( pageRef ); + } + + @Test( expected = IllegalStateException.class ) + public void disallowWriteLockedPageToExplicitlyLowerModifiedFlag() throws Exception + { + assertTrue( pageList.tryWriteLock( pageRef ) ); + pageList.explicitlyMarkPageUnmodifiedUnderExclusiveLock( pageRef ); + } + + @Test + public void allowExclusiveLockedPageToExplicitlyLowerModifiedFlag() throws Exception + { + assertFalse( pageList.isModified( pageRef ) ); + assertTrue( pageList.tryWriteLock( pageRef ) ); + pageList.unlockWrite( pageRef ); + assertTrue( pageList.isModified( pageRef ) ); + assertTrue( pageList.tryExclusiveLock( pageRef ) ); + assertTrue( pageList.isModified( pageRef ) ); + pageList.explicitlyMarkPageUnmodifiedUnderExclusiveLock( pageRef ); + assertFalse( pageList.isModified( pageRef ) ); + pageList.unlockExclusive( pageRef ); + } + // xxx ---[ Page state tests ]--- @Test @@ -1177,19 +1232,111 @@ public void tryEvictMustFailIfPageIsAlreadyExclusivelyLocked() throws Exception assertFalse( pageList.tryEvict( pageRef, DUMMY_SWAPPER ) ); } + @Test + public void tryEvictThatFailsOnExclusiveLockMustNotUndoSaidLock() throws Exception + { + doFault( 1, 42 ); // page is now loaded + // pages are delivered from the fault routine with the exclusive lock already held! + pageList.tryEvict( pageRef, DUMMY_SWAPPER ); // This attempt will fail + assertTrue( pageList.isExclusivelyLocked( pageRef ) ); // page should still have its lock + } + @Test public void tryEvictMustFailIfPageIsNotLoaded() throws Exception { assertFalse( pageList.tryEvict( pageRef, DUMMY_SWAPPER ) ); } - // todo try evict must leave page exclusively locked on success + + @Test + public void tryEvictMustWhenPageIsNotLoadedMustNotLeavePageLocked() throws Exception + { + pageList.tryEvict( pageRef, DUMMY_SWAPPER ); // This attempt fails + assertFalse( pageList.isExclusivelyLocked( pageRef ) ); // Page should not be left in locked state + } + + @Test( expected = IllegalArgumentException.class ) + public void tryEvictMustThrowIfSwapperIsNull() throws Exception + { + pageList.tryEvict( pageRef, null ); + } + + @Test + public void tryEvictMustLeavePageExclusivelyLockedOnSuccess() throws Exception + { + doFault( 1, 42 ); // page now bound & exclusively locked + pageList.unlockExclusive( pageRef ); // no longer exclusively locked; can now be evicted + assertTrue( pageList.tryEvict( pageRef, DUMMY_SWAPPER ) ); + pageList.unlockExclusive( pageRef ); // will throw if lock is not held + } + + @Test + public void pageMustNotBeLoadedAfterSuccessfulEviction() throws Exception + { + doFault( 1, 42 ); // page now bound & exclusively locked + pageList.unlockExclusive( pageRef ); // no longer exclusively locked; can now be evicted + assertTrue( pageList.isLoaded( pageRef ) ); + pageList.tryEvict( pageRef, DUMMY_SWAPPER ); + assertFalse( pageList.isLoaded( pageRef ) ); + } + + @Test + public void pageMustNotBeBoundAfterSuccessfulEviction() throws Exception + { + doFault( 1, 42 ); // page now bound & exclusively locked + pageList.unlockExclusive( pageRef ); // no longer exclusively locked; can now be evicted + assertTrue( pageList.isBoundTo( pageRef, 1, 42 ) ); + assertTrue( pageList.isLoaded( pageRef ) ); + assertThat( pageList.getSwapperId( pageRef ), is( 1 ) ); + pageList.tryEvict( pageRef, DUMMY_SWAPPER ); + assertFalse( pageList.isBoundTo( pageRef, 1, 42 ) ); + assertFalse( pageList.isLoaded( pageRef ) ); + assertThat( pageList.getSwapperId( pageRef ), is( 0 ) ); + } + + @Test + public void pageMustNotBeModifiedAfterSuccessfulEviction() throws Exception + { + doFault( 1, 42 ); + pageList.unlockExclusiveAndTakeWriteLock( pageRef ); + pageList.unlockWrite( pageRef ); // page is now modified + assertTrue( pageList.isModified( pageRef ) ); + assertTrue( pageList.tryEvict( pageRef, DUMMY_SWAPPER ) ); + assertFalse( pageList.isModified( pageRef ) ); + } + + @Test + public void tryEvictMustFlushPageIfModified() throws Exception + { + AtomicLong writtenFilePageId = new AtomicLong( -1 ); + AtomicLong writtenBufferAddress = new AtomicLong( -1 ); + AtomicInteger writtenBufferSize = new AtomicInteger( -1 ); + PageSwapper swapper = new DummyPageSwapper( "file" ) + { + @Override + public long write( long filePageId, long bufferAddress, int bufferSize ) throws IOException + { + assertTrue( writtenFilePageId.compareAndSet( -1, filePageId ) ); + assertTrue( writtenBufferAddress.compareAndSet( -1, bufferAddress ) ); + assertTrue( writtenBufferSize.compareAndSet( -1, bufferSize ) ); + return super.write( filePageId, bufferAddress, bufferSize ); + } + }; + doFault( 1, 42 ); + pageList.unlockExclusiveAndTakeWriteLock( pageRef ); + pageList.unlockWrite( pageRef ); // page is now modified + assertTrue( pageList.isModified( pageRef ) ); + assertTrue( pageList.tryEvict( pageRef, swapper ) ); + assertThat( writtenFilePageId.get(), is( 42L ) ); + assertThat( writtenBufferAddress.get(), is( pageList.address( pageRef ) ) ); +// assertThat( writtenBufferSize.get(), is( /* ... */ ) ); // todo + } // todo try evict must flush page if modified - // todo page must not be loaded after successful eviction - // todo page must not be bound after successful eviction - // todo page must not be modified after successful eviction + // todo try evict must not flush page if not modified // todo try evict must notify swapper on success // todo try evict must leave page unlocked if flush throws // todo try evict must leave page loaded if flush throws + // todo try evict must leave page bound if flush throws + // todo try evict must leave page modified if flush throws // todo try evict must report to eviction event // todo try evict that flushes must report to flush event // todo try evict that fails must not interfere with adjacent pages