From 36add50736d58b548cfba77fa388fd850d80c89e Mon Sep 17 00:00:00 2001 From: lutovich Date: Tue, 3 May 2016 17:19:26 +0200 Subject: [PATCH] Skip unused property records when reading chain To be able to increase property read throughput our reads should be more tolerant to concurrent updates of the same property chains. One of the steps - skip already removed properties. To do that store property cursor will skip unused records and will read and build chain of only used records. Exception that record is not in used will not be thrown anymore. Co-authored-by: @MishaDemianenko --- .../impl/api/store/StorePropertyCursor.java | 45 ++-- .../api/store/StorePropertyPayloadCursor.java | 13 +- .../api/store/StorePropertyCursorTest.java | 212 ++++++++++++++---- .../store/StorePropertyPayloadCursorTest.java | 24 +- 4 files changed, 223 insertions(+), 71 deletions(-) diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/api/store/StorePropertyCursor.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/api/store/StorePropertyCursor.java index e10a2c9d388b..80b9a700a680 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/api/store/StorePropertyCursor.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/api/store/StorePropertyCursor.java @@ -23,7 +23,6 @@ import org.neo4j.cursor.Cursor; import org.neo4j.function.Consumer; -import org.neo4j.graphdb.NotFoundException; import org.neo4j.io.pagecache.PageCursor; import org.neo4j.kernel.api.cursor.PropertyItem; import org.neo4j.kernel.impl.locking.Lock; @@ -66,35 +65,33 @@ public boolean next() return true; } - if ( nextPropertyRecordId == Record.NO_NEXT_PROPERTY.intValue() ) + while ( nextPropertyRecordId != Record.NO_NEXT_PROPERTY.intValue() ) { - return false; - } - - long currentPropertyRecordId = nextPropertyRecordId; - try ( PageCursor cursor = propertyStore.newReadCursor( currentPropertyRecordId ) ) - { - int offset = cursor.getOffset(); - do + try ( PageCursor cursor = propertyStore.newReadCursor( nextPropertyRecordId ) ) { - cursor.setOffset( offset ); - nextPropertyRecordId = readNextPropertyRecordId( cursor ); + int offset = cursor.getOffset(); + do + { + cursor.setOffset( offset ); + nextPropertyRecordId = readNextPropertyRecordId( cursor ); + + payload.clear(); + payload.init( cursor ); + } + while ( cursor.shouldRetry() ); + } + catch ( IOException e ) + { + throw new UnderlyingStorageException( e ); + } - payload.clear(); - payload.init( cursor ); + if ( payload.next() ) + { + return true; } - while ( cursor.shouldRetry() ); - } - catch ( IOException e ) - { - throw new UnderlyingStorageException( e ); } - if ( !payload.next() ) - { - throw new NotFoundException( "Property record with id " + currentPropertyRecordId + " not in use" ); - } - return true; + return false; } @Override diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/api/store/StorePropertyPayloadCursor.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/api/store/StorePropertyPayloadCursor.java index 72bfc5e35999..981b6aca5b71 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/api/store/StorePropertyPayloadCursor.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/api/store/StorePropertyPayloadCursor.java @@ -87,6 +87,7 @@ class StorePropertyPayloadCursor private final long[] data = new long[MAX_NUMBER_OF_PAYLOAD_LONG_ARRAY]; private int position = INITIAL_POSITION; + private boolean exhausted; StorePropertyPayloadCursor( DynamicStringStore stringStore, DynamicArrayStore arrayStore ) { @@ -105,6 +106,7 @@ void init( PageCursor cursor ) void clear() { position = INITIAL_POSITION; + exhausted = false; buffer = cachedBuffer; // Array of data should be filled with '0' because it is possible to call next() without calling init(). // In such case 'false' should be returned, which might not be the case if there is stale data in the buffer. @@ -113,6 +115,11 @@ void clear() boolean next() { + if ( exhausted ) + { + return false; + } + if ( position == INITIAL_POSITION ) { position = 0; @@ -121,11 +128,13 @@ boolean next() { position += currentBlocksUsed(); } - if ( position >= data.length ) + + if ( position >= data.length || type() == null ) { + exhausted = true; return false; } - return type() != null; + return true; } PropertyType type() diff --git a/community/kernel/src/test/java/org/neo4j/kernel/impl/api/store/StorePropertyCursorTest.java b/community/kernel/src/test/java/org/neo4j/kernel/impl/api/store/StorePropertyCursorTest.java index d3e1d64a6489..7e8c7cb83e7c 100644 --- a/community/kernel/src/test/java/org/neo4j/kernel/impl/api/store/StorePropertyCursorTest.java +++ b/community/kernel/src/test/java/org/neo4j/kernel/impl/api/store/StorePropertyCursorTest.java @@ -32,17 +32,15 @@ import java.io.File; import java.io.IOException; import java.util.ArrayList; -import java.util.Arrays; import java.util.Collection; +import java.util.Collections; import java.util.List; import org.neo4j.cursor.Cursor; import org.neo4j.function.Consumer; import org.neo4j.function.Consumers; -import org.neo4j.graphdb.NotFoundException; import org.neo4j.graphdb.mockfs.EphemeralFileSystemAbstraction; import org.neo4j.io.pagecache.PageCache; -import org.neo4j.io.pagecache.PageCursor; import org.neo4j.kernel.api.cursor.PropertyItem; import org.neo4j.kernel.impl.store.DynamicArrayStore; import org.neo4j.kernel.impl.store.DynamicRecordAllocator; @@ -58,22 +56,21 @@ import org.neo4j.test.EphemeralFileSystemRule; import org.neo4j.test.PageCacheRule; +import static java.util.Arrays.asList; import static org.junit.Assert.assertArrayEquals; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; -import static org.junit.Assert.fail; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; - import static org.neo4j.kernel.impl.locking.LockService.NO_LOCK; @RunWith( Enclosed.class ) public class StorePropertyCursorTest { - private static final List PARAMETERS = Arrays.asList( + private static final List PARAMETERS = asList( new Object[]{false, PropertyType.BOOL}, new Object[]{(byte) 3, PropertyType.BYTE}, new Object[]{(short) 34, PropertyType.SHORT}, @@ -245,30 +242,6 @@ public void shouldReturnTheCursorToTheCacheOnClose() throws Throwable // then verify( cache, times( 1 ) ).accept( storePropertyCursor ); } - - @Test - public void shouldThrowIfTheRecordIsNotInUse() throws Throwable - { - // given - int recordId = 42; - PageCursor pageCursor = mock( PageCursor.class ); - when( propertyStore.newReadCursor( recordId ) ).thenReturn( pageCursor ); - when( pageCursor.shouldRetry() ).thenReturn( false ); - - StorePropertyCursor storePropertyCursor = newStorePropertyCursor( propertyStore, cache ); - - try ( Cursor cursor = storePropertyCursor.init( recordId, NO_LOCK ) ) - { - // when - cursor.next(); - fail(); - } - catch ( NotFoundException ex ) - { - // then - assertEquals( "Property record with id " + recordId + " not in use", ex.getMessage() ); - } - } } public static class PropertyStoreBasedTestSupport @@ -292,8 +265,6 @@ public static void tearDownPageCache() throws IOException pageCache.close(); } - protected final Consumer cache = Consumers.noop(); - protected PropertyStore propertyStore; private NeoStores neoStores; @@ -353,7 +324,7 @@ public void shouldReturnAProperty() throws Throwable createSinglePropertyValue( propertyStore, recordId, keyId, expectedValue ); - StorePropertyCursor storePropertyCursor = newStorePropertyCursor( propertyStore, cache ); + StorePropertyCursor storePropertyCursor = newStorePropertyCursor( propertyStore ); // when try ( Cursor cursor = storePropertyCursor.init( recordId, NO_LOCK ) ) @@ -416,7 +387,7 @@ public void shouldReturnAPropertyBySkippingOne() throws Throwable createTwoPropertyValues( propertyStore, recordId, keyId1, expectedValue1, keyId2, expectedValue2 ); - StorePropertyCursor storePropertyCursor = newStorePropertyCursor( propertyStore, cache ); + StorePropertyCursor storePropertyCursor = newStorePropertyCursor( propertyStore ); // when try ( Cursor cursor = storePropertyCursor.init( recordId, NO_LOCK ) ) @@ -448,7 +419,7 @@ public void shouldReturnTwoProperties() throws Throwable createTwoPropertyValues( propertyStore, recordId, keyId1, expectedValue1, keyId2, expectedValue2 ); - StorePropertyCursor storePropertyCursor = newStorePropertyCursor( propertyStore, cache ); + StorePropertyCursor storePropertyCursor = newStorePropertyCursor( propertyStore ); // when try ( Cursor cursor = storePropertyCursor.init( recordId, NO_LOCK ) ) @@ -496,7 +467,7 @@ public void shouldReuseCorrectlyCursor() throws Throwable createSinglePropertyValue( propertyStore, recordId, keyId, expectedValue ); - StorePropertyCursor storePropertyCursor = newStorePropertyCursor( propertyStore, cache ); + StorePropertyCursor storePropertyCursor = newStorePropertyCursor( propertyStore ); try ( Cursor cursor = storePropertyCursor.init( recordId, NO_LOCK ) ) { @@ -520,6 +491,109 @@ public void shouldReuseCorrectlyCursor() throws Throwable } } + public static class PropertyChains extends PropertyStoreBasedTestSupport + { + @Test + public void readPropertyChainWithMultipleEntries() + { + int firstPropertyId = 1; + int propertyKeyId = 42; + Object[] propertyValues = {"1", "2", 3, 4, 5L, 6L, '7', '8', "9 and 10"}; + + createPropertyChain( propertyStore, firstPropertyId, propertyKeyId, propertyValues ); + + try ( StorePropertyCursor cursor = newStorePropertyCursor( propertyStore ) ) + { + cursor.init( firstPropertyId, NO_LOCK ); + + List valuesFromCursor = asPropertyValuesList( cursor ); + assertEquals( asList( propertyValues ), valuesFromCursor ); + } + } + + @Test + public void callNextAfterReadingPropertyChain() + { + int firstPropertyId = 1; + int propertyKeyId = 42; + + createPropertyChain( propertyStore, firstPropertyId, propertyKeyId, "1", "2" ); + + try ( StorePropertyCursor cursor = newStorePropertyCursor( propertyStore ) ) + { + cursor.init( firstPropertyId, NO_LOCK ); + + assertTrue( cursor.next() ); + assertEquals( "1", cursor.value() ); + + assertTrue( cursor.next() ); + assertEquals( "2", cursor.value() ); + + assertFalse( cursor.next() ); + assertFalse( cursor.next() ); + assertFalse( cursor.next() ); + } + } + + @Test + public void skipUnusedRecordsInChain() + { + int firstPropertyId = 1; + int propertyKeyId = 42; + Object[] propertyValues = {"1", "2", 3, 4, 5L, 6L, '7', '8', "9 and 10"}; + + createPropertyChain( propertyStore, firstPropertyId, propertyKeyId, propertyValues ); + markPropertyRecordsNoInUse( propertyStore, generateRecordIds( firstPropertyId + 1, 2, 4 ) ); + + try ( StorePropertyCursor cursor = newStorePropertyCursor( propertyStore ) ) + { + cursor.init( firstPropertyId, NO_LOCK ); + + List valuesFromCursor = asPropertyValuesList( cursor ); + assertEquals( asList( "1", 3, 5L, '7', "9 and 10" ), valuesFromCursor ); + } + } + + @Test + public void skipUnusedConsecutiveRecordsInChain() + { + int firstPropertyId = 1; + int propertyKeyId = 42; + Object[] propertyValues = {"1", "2", 3, 4, 5L, 6L, '7', '8', "9 and 10"}; + + createPropertyChain( propertyStore, firstPropertyId, propertyKeyId, propertyValues ); + markPropertyRecordsNoInUse( propertyStore, generateRecordIds( firstPropertyId + 2, 1, 3 ) ); + + try ( StorePropertyCursor cursor = newStorePropertyCursor( propertyStore ) ) + { + cursor.init( firstPropertyId, NO_LOCK ); + + List valuesFromCursor = asPropertyValuesList( cursor ); + assertEquals( asList( "1", "2", 6L, '7', '8', "9 and 10" ), valuesFromCursor ); + } + } + + @Test + public void skipAllRecordsWhenWholeChainNotInUse() + { + int firstPropertyId = 1; + int propertyKeyId = 42; + Object[] propertyValues = {"1", "2", 3, 4, 5L, 6L, '7', '8', "9 and 10"}; + + createPropertyChain( propertyStore, firstPropertyId, propertyKeyId, propertyValues ); + int[] recordIds = generateRecordIds( firstPropertyId, 1, propertyValues.length ); + markPropertyRecordsNoInUse( propertyStore, recordIds ); + + try ( StorePropertyCursor cursor = newStorePropertyCursor( propertyStore ) ) + { + cursor.init( firstPropertyId, NO_LOCK ); + + List valuesFromCursor = asPropertyValuesList( cursor ); + assertEquals( Collections.emptyList(), valuesFromCursor ); + } + } + } + private static void assertEqualValues( Object expectedValue, PropertyItem item ) { // fetch twice with typed methods @@ -535,13 +609,59 @@ private static void assertEqualValues( Object expectedValue, PropertyItem item ) } } - static StorePropertyCursor newStorePropertyCursor( PropertyStore propertyStore, + private static StorePropertyCursor newStorePropertyCursor( PropertyStore propertyStore ) + { + return newStorePropertyCursor( propertyStore, Consumers.noop() ); + } + + private static StorePropertyCursor newStorePropertyCursor( PropertyStore propertyStore, Consumer cache ) { return new StorePropertyCursor( propertyStore, cache ); } - private static void createSinglePropertyValue( PropertyStore store, int recordId, int keyId, Object value ) + private static void createPropertyChain( PropertyStore store, int firstRecordId, int keyId, Object... values ) + { + int nextRecordId = firstRecordId; + PropertyRecord previousRecord = null; + for ( Object value : values ) + { + PropertyRecord record = createSinglePropertyValue( store, nextRecordId, keyId, value ); + if ( previousRecord != null ) + { + record.setPrevProp( previousRecord.getId() ); + store.updateRecord( record ); + + previousRecord.setNextProp( record.getId() ); + store.updateRecord( previousRecord ); + } + previousRecord = record; + nextRecordId++; + } + } + + private static void markPropertyRecordsNoInUse( PropertyStore store, int... recordIds ) + { + for ( int recordId : recordIds ) + { + PropertyRecord record = store.forceGetRecord( recordId ); + record.setInUse( false ); + store.forceUpdateRecord( record ); + } + } + + private static int[] generateRecordIds( int startId, int step, int count ) + { + int[] ints = new int[count]; + for ( int i = 0; i < count; i++ ) + { + ints[i] = startId + i * step; + } + return ints; + } + + private static PropertyRecord createSinglePropertyValue( PropertyStore store, int recordId, int keyId, + Object value ) { DynamicRecordAllocator stringAllocator = store.getStringStore(); DynamicRecordAllocator arrayAllocator = store.getArrayStore(); @@ -549,11 +669,13 @@ private static void createSinglePropertyValue( PropertyStore store, int recordId PropertyBlock block = new PropertyBlock(); PropertyStore.encodeValue( block, keyId, value, stringAllocator, arrayAllocator ); - PropertyRecord record = new PropertyRecord( recordId ); record.addPropertyBlock( block ); record.setInUse( true ); store.updateRecord( record ); + store.setHighestPossibleIdInUse( recordId ); + + return record; } private static void createTwoPropertyValues( PropertyStore store, int recordId, @@ -581,9 +703,21 @@ private static void createTwoPropertyValues( PropertyStore store, int recordId, nextRecord.setPrevProp( record.getId() ); nextRecord.setInUse( true ); store.updateRecord( nextRecord ); + store.setHighestPossibleIdInUse( nextRecord.getId() ); } record.setInUse( true ); store.updateRecord( record ); + store.setHighestPossibleIdInUse( record.getId() ); + } + + private static List asPropertyValuesList( StorePropertyCursor cursor ) + { + List values = new ArrayList<>(); + while ( cursor.next() ) + { + values.add( cursor.value() ); + } + return values; } } diff --git a/community/kernel/src/test/java/org/neo4j/kernel/impl/api/store/StorePropertyPayloadCursorTest.java b/community/kernel/src/test/java/org/neo4j/kernel/impl/api/store/StorePropertyPayloadCursorTest.java index dc0933686211..8204d257eeb3 100644 --- a/community/kernel/src/test/java/org/neo4j/kernel/impl/api/store/StorePropertyPayloadCursorTest.java +++ b/community/kernel/src/test/java/org/neo4j/kernel/impl/api/store/StorePropertyPayloadCursorTest.java @@ -84,8 +84,8 @@ public void shouldBeOkToClearPartiallyExhaustedCursor() // Given StorePropertyPayloadCursor cursor = newCursor( 1, 2, 3L ); - cursor.next(); - cursor.next(); + assertTrue( cursor.next() ); + assertTrue( cursor.next() ); // When cursor.clear(); @@ -100,9 +100,9 @@ public void shouldBeOkToClearExhaustedCursor() // Given StorePropertyPayloadCursor cursor = newCursor( 1, 2, 3 ); - cursor.next(); - cursor.next(); - cursor.next(); + assertTrue( cursor.next() ); + assertTrue( cursor.next() ); + assertTrue( cursor.next() ); // When cursor.clear(); @@ -131,7 +131,7 @@ public void shouldBePossibleToCallNextOnEmptyCursor() StorePropertyPayloadCursor cursor = newCursor(); // When - cursor.next(); + assertFalse( cursor.next() ); // Then // next() on an empty cursor works just fine @@ -161,6 +161,18 @@ public void shouldUseDynamicStringAndArrayStoresThroughDifferentCursors() verify( dynamicStringStore ).newDynamicRecordCursor(); verify( dynamicArrayStore ).newDynamicRecordCursor(); } + + @Test + public void nextMultipleInvocations() + { + StorePropertyPayloadCursor cursor = newCursor(); + + assertFalse( cursor.next() ); + assertFalse( cursor.next() ); + assertFalse( cursor.next() ); + assertFalse( cursor.next() ); + } + } @RunWith( Parameterized.class )