From 7705a2bda019fb8f3eadeec92c6f77faeca58bdd Mon Sep 17 00:00:00 2001 From: Anton Persson Date: Fri, 19 May 2017 14:53:15 +0200 Subject: [PATCH] Replace multiple locks in GBPTree ... with single simple lock. --- .../neo4j/index/internal/gbptree/GBPTree.java | 43 ++--- .../internal/gbptree/GBPTreeCleanupJob.java | 14 +- .../index/internal/gbptree/GBPTreeLock.java | 101 +++++++++++ .../internal/gbptree/GBPTreeLockTest.java | 163 ++++++++++++++++++ 4 files changed, 287 insertions(+), 34 deletions(-) create mode 100644 community/index/src/main/java/org/neo4j/index/internal/gbptree/GBPTreeLock.java create mode 100644 community/index/src/test/java/org/neo4j/index/internal/gbptree/GBPTreeLockTest.java diff --git a/community/index/src/main/java/org/neo4j/index/internal/gbptree/GBPTree.java b/community/index/src/main/java/org/neo4j/index/internal/gbptree/GBPTree.java index 1d345245884d2..8d7d7affd9bfc 100644 --- a/community/index/src/main/java/org/neo4j/index/internal/gbptree/GBPTree.java +++ b/community/index/src/main/java/org/neo4j/index/internal/gbptree/GBPTree.java @@ -27,9 +27,6 @@ import java.nio.file.NoSuchFileException; import java.nio.file.StandardOpenOption; import java.util.concurrent.atomic.AtomicBoolean; -import java.util.concurrent.locks.Lock; -import java.util.concurrent.locks.ReentrantLock; -import java.util.concurrent.locks.StampedLock; import java.util.function.Consumer; import java.util.function.LongSupplier; import java.util.function.Supplier; @@ -249,24 +246,20 @@ public void startupState( boolean clean ) private volatile boolean changesSinceLastCheckpoint; /** + * Lock with two individual parts. Writer lock and cleaner lock. + *

* There are a few different scenarios that involve writing or flushing that can not be happen concurrently: *

- * This lock is acquired as part of all of those tasks. - */ - private final Lock writerLock = new ReentrantLock(); - - /** - * If cleaning of crash pointers is needed the tree can not be allowed to perform a checkpoint until that job - * has finished. Therefore this lock is acquired by checkpoint and when cleaning is needed. + * For those scenarios, writer lock is taken. *

- * The cleaner job is responsible for releasing this lock after it has finished. This job will be executed by - * some other thread, that's why this lock is a {@link StampedLock}. + * If cleaning of crash pointers is needed the tree can not be allowed to perform a checkpoint until that job + * has finished. For this scenario, cleaner lock is taken. */ - private final StampedLock cleanerLock = new StampedLock(); + private final GBPTreeLock lock = new GBPTreeLock(); /** * Page size, i.e. tree node size, of the tree nodes in this tree. The page size is determined on @@ -771,8 +764,8 @@ private void checkpoint( IOLimiter ioLimiter, Header.Writer headerWriter ) throw // Block writers, or if there's a current writer then wait for it to complete and then block // From this point and till the lock is released we know that the tree won't change. - long stamp = cleanerLock.writeLock(); - writerLock.lock(); + lock.cleanerLock(); + lock.writerLock(); try { // Flush dirty pages since that last flush above. This should be a very small set of pages @@ -799,8 +792,8 @@ private void checkpoint( IOLimiter ioLimiter, Header.Writer headerWriter ) throw { // Unblock writers, any writes after this point and up until the next checkpoint will have // the new unstable generation. - writerLock.unlock(); - cleanerLock.unlockWrite( stamp ); + lock.writerUnlock(); + lock.cleanerUnlock(); } } @@ -822,7 +815,7 @@ private void assertRecoveryCleanSuccessful() throws IOException @Override public void close() throws IOException { - writerLock.lock(); + lock.writerLock(); try { if ( closed ) @@ -847,7 +840,7 @@ public void close() throws IOException } finally { - writerLock.unlock(); + lock.writerUnlock(); } } @@ -923,7 +916,7 @@ private CleanupJob createCleanupJob( boolean needsCleaning ) throws IOException } else { - long stamp = cleanerLock.writeLock(); + lock.cleanerLock(); long generation = this.generation; long stableGeneration = stableGeneration( generation ); @@ -933,7 +926,7 @@ private CleanupJob createCleanupJob( boolean needsCleaning ) throws IOException CrashGenerationCleaner crashGenerationCleaner = new CrashGenerationCleaner( pagedFile, bTreeNode, IdSpace.MIN_TREE_NODE_ID, highTreeNodeId, stableGeneration, unstableGeneration, monitor ); - return new GBPTreeCleanupJob( crashGenerationCleaner, cleanerLock, stamp ); + return new GBPTreeCleanupJob( crashGenerationCleaner, lock ); } } @@ -1016,13 +1009,13 @@ private class SingleWriter implements Writer * Either fully initialized: *

* Of fully closed: * * @@ -1040,7 +1033,7 @@ void initialize() throws IOException boolean success = false; try { - writerLock.lock(); + lock.writerLock(); cursor = openRootCursor( PagedFile.PF_SHARED_WRITE_LOCK ); stableGeneration = stableGeneration( generation ); unstableGeneration = unstableGeneration( generation ); @@ -1144,7 +1137,7 @@ public void close() throws IOException ", but writer is already closed." ); } closeCursor(); - writerLock.unlock(); + lock.writerUnlock(); } private void closeCursor() diff --git a/community/index/src/main/java/org/neo4j/index/internal/gbptree/GBPTreeCleanupJob.java b/community/index/src/main/java/org/neo4j/index/internal/gbptree/GBPTreeCleanupJob.java index 3840bb67e387a..fc0befbb2540b 100644 --- a/community/index/src/main/java/org/neo4j/index/internal/gbptree/GBPTreeCleanupJob.java +++ b/community/index/src/main/java/org/neo4j/index/internal/gbptree/GBPTreeCleanupJob.java @@ -20,26 +20,22 @@ package org.neo4j.index.internal.gbptree; import java.io.IOException; -import java.util.concurrent.locks.Lock; -import java.util.concurrent.locks.StampedLock; class GBPTreeCleanupJob implements CleanupJob { private final CrashGenerationCleaner crashGenerationCleaner; - private final StampedLock stampedLock; - private final long stamp; + private final GBPTreeLock gbpTreeLock; private volatile boolean needed; private volatile Exception failure; /** * @param crashGenerationCleaner {@link CrashGenerationCleaner} to use for cleaning. - * @param lock {@link Lock} to be released when job has either successfully finished or failed. + * @param gbpTreeLock {@link GBPTreeLock} to be released when job has either successfully finished or failed. */ - GBPTreeCleanupJob( CrashGenerationCleaner crashGenerationCleaner, StampedLock lock, long stamp ) + GBPTreeCleanupJob( CrashGenerationCleaner crashGenerationCleaner, GBPTreeLock gbpTreeLock ) { this.crashGenerationCleaner = crashGenerationCleaner; - this.stampedLock = lock; - this.stamp = stamp; + this.gbpTreeLock = gbpTreeLock; this.needed = true; } @@ -76,7 +72,7 @@ public void run() } finally { - stampedLock.unlockWrite( stamp ); + gbpTreeLock.cleanerUnlock(); } } } diff --git a/community/index/src/main/java/org/neo4j/index/internal/gbptree/GBPTreeLock.java b/community/index/src/main/java/org/neo4j/index/internal/gbptree/GBPTreeLock.java new file mode 100644 index 0000000000000..0df0560765007 --- /dev/null +++ b/community/index/src/main/java/org/neo4j/index/internal/gbptree/GBPTreeLock.java @@ -0,0 +1,101 @@ +/* + * Copyright (c) 2002-2017 "Neo Technology," + * Network Engine for Objects in Lund AB [http://neotechnology.com] + * + * This file is part of Neo4j. + * + * Neo4j is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + */ +package org.neo4j.index.internal.gbptree; + +import org.neo4j.unsafe.impl.internal.dragons.UnsafeUtil; + +class GBPTreeLock +{ + private static final long stateOffset = UnsafeUtil.getFieldOffset( GBPTreeLock.class, "state" ); + private static final long writerLockBit = 0x00000000_00000001L; + private static final long cleanerLockBit = 0x00000000_00000002L; + private volatile long state; + + void writerLock() + { + doLock( writerLockBit ); + } + + void writerUnlock() + { + doUnlock( writerLockBit ); + } + + void cleanerLock() + { + doLock( cleanerLockBit ); + } + + void cleanerUnlock() + { + doUnlock( cleanerLockBit ); + } + + private void doLock( long targetLockBit ) + { + long currentState; + long newState; + do + { + currentState = state; + while ( isLocked( currentState, targetLockBit ) ) + { + // sleep + sleep(); + currentState = state; + } + newState = currentState | targetLockBit; + } while ( !UnsafeUtil.compareAndSwapLong( this, stateOffset, currentState, newState ) ); + } + + private boolean isLocked( long state, long targetLockBit ) + { + return (state & targetLockBit) == targetLockBit; + } + + private void doUnlock( long targetLockBit ) + { + long currentState; + long newState; + do + { + currentState = state; + if ( !isLocked( currentState, targetLockBit) ) + { + throw new IllegalStateException( "Can not unlock lock that is already locked" ); + } + newState = currentState & ~targetLockBit; + } + while ( !UnsafeUtil.compareAndSwapLong( this, stateOffset, currentState, newState ) ); + } + + private void sleep() + { + try + { + Thread.sleep( 10 ); + } + catch ( InterruptedException e ) + { + // todo what to do in this case + throw new RuntimeException( e ); + } + } +} diff --git a/community/index/src/test/java/org/neo4j/index/internal/gbptree/GBPTreeLockTest.java b/community/index/src/test/java/org/neo4j/index/internal/gbptree/GBPTreeLockTest.java new file mode 100644 index 0000000000000..2a8540b0a8d75 --- /dev/null +++ b/community/index/src/test/java/org/neo4j/index/internal/gbptree/GBPTreeLockTest.java @@ -0,0 +1,163 @@ +/* + * Copyright (c) 2002-2017 "Neo Technology," + * Network Engine for Objects in Lund AB [http://neotechnology.com] + * + * This file is part of Neo4j. + * + * Neo4j is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + */ +package org.neo4j.index.internal.gbptree; + +import org.junit.Test; + +import java.util.concurrent.ExecutionException; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; + +import static org.junit.Assert.fail; + +public class GBPTreeLockTest +{ + // Lock can be in following states and this test verify transitions back and forth between states + // and also verify expected behaviour after each transition. + // Writer | Cleaner + // State UU - unlocked | unlocked + // State UL - unlocked | locked + // State LU - locked | unlocked + // State LL - locked | locked + + private GBPTreeLock lock = new GBPTreeLock(); + private final ExecutorService executor = Executors.newSingleThreadExecutor(); + + @Test + public void test_UU_UL_UU() throws Exception + { + // given + assertUU(); + + // then + lock.cleanerLock(); + assertUL(); + + lock.cleanerUnlock(); + assertUU(); + } + + @Test + public void test_UL_LL_UL() throws Exception + { + // given + lock.cleanerLock(); + assertUL(); + + // then + lock.writerLock(); + assertLL(); + + lock.writerUnlock(); + assertUL(); + } + + @Test + public void test_LL_LU_LL() throws Exception + { + // given + lock.writerLock(); + lock.cleanerLock(); + assertLL(); + + // then + lock.cleanerUnlock(); + assertLU(); + + lock.cleanerLock(); + assertLL(); + } + + @Test + public void test_LU_UU_LU() throws Exception + { + // given + lock.writerLock(); + assertLU(); + + // then + lock.writerUnlock(); + assertUU(); + + lock.writerLock(); + assertLU(); + } + + private void assertThrow( Runnable unlock ) + { + try + { + unlock.run(); + fail( "Should have failed" ); + } + catch ( IllegalStateException e ) + { + // good + } + } + + private void assertBlock( Runnable lock, Runnable unlock ) throws ExecutionException, InterruptedException + { + Future future = executor.submit( lock ); + shouldWait( future ); + unlock.run(); + future.get(); + } + + private void shouldWait( Future future )throws InterruptedException, ExecutionException + { + try + { + future.get( 200, TimeUnit.MILLISECONDS ); + fail( "Expected timeout" ); + } + catch ( TimeoutException e ) + { + // good + } + } + + private void assertUU() + { + assertThrow( lock::writerUnlock ); + assertThrow( lock::cleanerUnlock ); + } + + private void assertUL() throws ExecutionException, InterruptedException + { + assertThrow( lock::writerUnlock ); + assertBlock( lock::cleanerLock, lock::cleanerUnlock ); + } + + private void assertLU() throws ExecutionException, InterruptedException + { + assertBlock( lock::writerLock, lock::writerUnlock ); + assertThrow( lock::cleanerUnlock ); + } + + private void assertLL() throws ExecutionException, InterruptedException + { + assertBlock( lock::writerLock, lock::writerUnlock ); + assertBlock( lock::cleanerLock, lock::cleanerUnlock ); + } +}