From 55792128a3c4eab108294d856e40a1b40f5be7fb Mon Sep 17 00:00:00 2001 From: Davide Grohmann Date: Thu, 16 Mar 2017 17:03:06 +0100 Subject: [PATCH] Use page cursor for reading records from the relationship group store --- .../store/StoreNodeRelationshipCursor.java | 41 ++++++++++++++---- .../kernel/impl/api/store/StoreStatement.java | 26 ++++------- .../neo4j/kernel/impl/store/NeoStores.java | 2 +- .../internal/BatchInserterImpl.java | 11 ++--- .../internal/BatchRelationshipIterable.java | 13 +++--- .../StoreNodeRelationshipCursorTest.java | 43 +++++++++++++------ .../NeoStoreTransactionApplierTest.java | 2 +- 7 files changed, 82 insertions(+), 56 deletions(-) diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/api/store/StoreNodeRelationshipCursor.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/api/store/StoreNodeRelationshipCursor.java index 41c66baefc0f..27a2cec5290c 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/api/store/StoreNodeRelationshipCursor.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/api/store/StoreNodeRelationshipCursor.java @@ -19,14 +19,17 @@ */ package org.neo4j.kernel.impl.api.store; +import java.io.IOException; import java.util.function.Consumer; import java.util.function.IntPredicate; import org.neo4j.collection.primitive.PrimitiveLongIterator; +import org.neo4j.io.pagecache.PageCursor; import org.neo4j.kernel.impl.locking.LockService; import org.neo4j.kernel.impl.store.InvalidRecordException; -import org.neo4j.kernel.impl.store.RecordCursors; +import org.neo4j.kernel.impl.store.RelationshipGroupStore; import org.neo4j.kernel.impl.store.RelationshipStore; +import org.neo4j.kernel.impl.store.UnderlyingStorageException; import org.neo4j.kernel.impl.store.record.Record; import org.neo4j.kernel.impl.store.record.RelationshipGroupRecord; import org.neo4j.kernel.impl.store.record.RelationshipRecord; @@ -47,7 +50,10 @@ public class StoreNodeRelationshipCursor extends StoreAbstractIteratorRelationshipCursor { private final RelationshipGroupRecord groupRecord; + private final RelationshipGroupStore relationshipGroupStore; private final Consumer instanceCache; + private final PageCursor groupStorePageCursor; + private boolean isDense; private long relationshipId; private long fromNodeId; @@ -55,18 +61,17 @@ public class StoreNodeRelationshipCursor extends StoreAbstractIteratorRelationsh private IntPredicate allowedTypes; private int groupChainIndex; private boolean end; - private final RecordCursors cursors; public StoreNodeRelationshipCursor( RelationshipStore relationshipStore, - RelationshipGroupRecord groupRecord, + RelationshipGroupStore relationshipGroupStore, Consumer instanceCache, - RecordCursors cursors, LockService lockService ) { super( relationshipStore, lockService ); - this.groupRecord = groupRecord; + this.relationshipGroupStore = relationshipGroupStore; + this.groupRecord = relationshipGroupStore.newRecord(); + this.groupStorePageCursor = relationshipGroupStore.newPageCursor(); this.instanceCache = instanceCache; - this.cursors = cursors; } public StoreNodeRelationshipCursor init( boolean isDense, long firstRelId, long fromNodeId, Direction direction, @@ -97,7 +102,7 @@ private StoreNodeRelationshipCursor init( boolean isDense, long firstRelId, long if ( isDense && relationshipId != Record.NO_NEXT_RELATIONSHIP.intValue() ) { - cursors.relationshipGroup().next( firstRelId, groupRecord, FORCE ); + readGroupRecord( firstRelId ); relationshipId = nextChainStart(); } else @@ -108,6 +113,19 @@ private StoreNodeRelationshipCursor init( boolean isDense, long firstRelId, long return this; } + private void readGroupRecord( long id ) + { + try + { + groupRecord.clear(); + relationshipGroupStore.readIntoRecord( id, groupRecord, FORCE, groupStorePageCursor ); + } + catch ( IOException e ) + { + throw new UnderlyingStorageException( e ); + } + } + private PrimitiveLongIterator addedNodeRelationships( long fromNodeId, Direction direction, int[] allowedTypes, ReadableTransactionState state ) { @@ -237,7 +255,7 @@ private long nextChainStart() // Go to the next group if ( !NULL_REFERENCE.is( groupRecord.getNext() ) ) { - cursors.relationshipGroup().next( groupRecord.getNext(), groupRecord, FORCE ); + readGroupRecord( groupRecord.getNext() ); } else { @@ -311,4 +329,11 @@ public String toString() return String .format( "RelationShipItem[id=%d, type=%d, start=%d, end=%d]", id(), type(), startNode(), endNode() ); } + + @Override + public void dispose() + { + super.dispose(); + groupStorePageCursor.close(); + } } diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/api/store/StoreStatement.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/api/store/StoreStatement.java index 5f3b1b44f2e7..fcc91acc4aee 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/api/store/StoreStatement.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/api/store/StoreStatement.java @@ -28,11 +28,7 @@ import org.neo4j.kernel.impl.locking.Lock; import org.neo4j.kernel.impl.locking.LockService; import org.neo4j.kernel.impl.store.NeoStores; -import org.neo4j.kernel.impl.store.NodeStore; import org.neo4j.kernel.impl.store.RecordCursors; -import org.neo4j.kernel.impl.store.RecordStore; -import org.neo4j.kernel.impl.store.RelationshipStore; -import org.neo4j.kernel.impl.store.record.RelationshipGroupRecord; import org.neo4j.kernel.impl.util.InstanceCache; import org.neo4j.storageengine.api.Direction; import org.neo4j.storageengine.api.NodeItem; @@ -60,12 +56,9 @@ public class StoreStatement implements StorageStatement private final InstanceCache propertyCursorCache; private final InstanceCache singlePropertyCursorCache; private final NeoStores neoStores; - private final NodeStore nodeStore; - private final RelationshipStore relationshipStore; private final Supplier indexReaderFactorySupplier; - private final RecordCursors recordCursors; private final Supplier labelScanStore; - private final RecordStore relationshipGroupStore; + private final RecordCursors recordCursors; private IndexReaderFactory indexReaderFactory; private LabelScanReader labelScanReader; @@ -79,9 +72,6 @@ public StoreStatement( NeoStores neoStores, Supplier indexRe this.neoStores = neoStores; this.indexReaderFactorySupplier = indexReaderFactory; this.labelScanStore = labelScanReaderSupplier; - this.nodeStore = neoStores.getNodeStore(); - this.relationshipStore = neoStores.getRelationshipStore(); - this.relationshipGroupStore = neoStores.getRelationshipGroupStore(); this.recordCursors = new RecordCursors( neoStores ); nodeCursor = new InstanceCache() @@ -89,7 +79,7 @@ public StoreStatement( NeoStores neoStores, Supplier indexRe @Override protected NodeCursor create() { - return new NodeCursor( nodeStore, this, lockService ); + return new NodeCursor( neoStores.getNodeStore(), this, lockService ); } }; singleRelationshipCursor = new InstanceCache() @@ -97,7 +87,7 @@ protected NodeCursor create() @Override protected StoreSingleRelationshipCursor create() { - return new StoreSingleRelationshipCursor( relationshipStore, this, lockService ); + return new StoreSingleRelationshipCursor( neoStores.getRelationshipStore(), this, lockService ); } }; iteratorRelationshipCursor = new InstanceCache() @@ -105,7 +95,7 @@ protected StoreSingleRelationshipCursor create() @Override protected StoreIteratorRelationshipCursor create() { - return new StoreIteratorRelationshipCursor( relationshipStore, this, lockService ); + return new StoreIteratorRelationshipCursor( neoStores.getRelationshipStore(), this, lockService ); } }; nodeRelationshipsCursor = new InstanceCache() @@ -113,8 +103,8 @@ protected StoreIteratorRelationshipCursor create() @Override protected StoreNodeRelationshipCursor create() { - return new StoreNodeRelationshipCursor( relationshipStore, - relationshipGroupStore.newRecord(), this, recordCursors, lockService ); + return new StoreNodeRelationshipCursor( neoStores.getRelationshipStore(), + neoStores.getRelationshipGroupStore(), this, lockService ); } }; propertyCursorCache = new InstanceCache() @@ -147,7 +137,7 @@ public void acquire() public Cursor acquireNodeCursor( ReadableTransactionState state ) { neoStores.assertOpen(); - return nodeCursor.get().init( new AllNodeProgression( nodeStore ), state ); + return nodeCursor.get().init( new AllNodeProgression( neoStores.getNodeStore() ), state ); } @Override @@ -178,7 +168,7 @@ public Cursor acquireNodeRelationshipCursor( boolean isDense, public Cursor relationshipsGetAllCursor( ReadableTransactionState state ) { neoStores.assertOpen(); - return iteratorRelationshipCursor.get().init( new AllIdIterator( relationshipStore ), state ); + return iteratorRelationshipCursor.get().init( new AllIdIterator( neoStores.getRelationshipStore() ), state ); } @Override diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/store/NeoStores.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/store/NeoStores.java index 05bfcaa6e99b..f52ed0d37650 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/store/NeoStores.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/store/NeoStores.java @@ -396,7 +396,7 @@ private DynamicStringStore getPropertyKeyTokenNamesStore() return (DynamicStringStore) getStore( StoreType.PROPERTY_KEY_TOKEN_NAME ); } - public RecordStore getRelationshipGroupStore() + public RelationshipGroupStore getRelationshipGroupStore() { return (RelationshipGroupStore) getStore( StoreType.RELATIONSHIP_GROUP ); } diff --git a/community/kernel/src/main/java/org/neo4j/unsafe/batchinsert/internal/BatchInserterImpl.java b/community/kernel/src/main/java/org/neo4j/unsafe/batchinsert/internal/BatchInserterImpl.java index b6826011822f..f97df8de23c5 100644 --- a/community/kernel/src/main/java/org/neo4j/unsafe/batchinsert/internal/BatchInserterImpl.java +++ b/community/kernel/src/main/java/org/neo4j/unsafe/batchinsert/internal/BatchInserterImpl.java @@ -113,7 +113,6 @@ import org.neo4j.kernel.impl.store.NodeStore; import org.neo4j.kernel.impl.store.PropertyKeyTokenStore; import org.neo4j.kernel.impl.store.PropertyStore; -import org.neo4j.kernel.impl.store.RecordCursors; import org.neo4j.kernel.impl.store.RecordStore; import org.neo4j.kernel.impl.store.RelationshipStore; import org.neo4j.kernel.impl.store.RelationshipTypeTokenStore; @@ -212,14 +211,12 @@ public Label apply( long from ) private final RelationshipTypeTokenStore relationshipTypeTokenStore; private final PropertyKeyTokenStore propertyKeyTokenStore; private final PropertyStore propertyStore; - private final RecordStore relationshipGroupStore; private final SchemaStore schemaStore; private final NeoStoreIndexStoreView indexStoreView; private final LabelTokenStore labelTokenStore; private final Locks.Client noopLockClient = new NoOpClient(); private final long maxNodeId; - private final RecordCursors cursors; public BatchInserterImpl( final File storeDir, final FileSystemAbstraction fileSystem, Map stringParams, Iterable> kernelExtensions ) throws IOException @@ -266,7 +263,7 @@ public BatchInserterImpl( final File storeDir, final FileSystemAbstraction fileS relationshipTypeTokenStore = neoStores.getRelationshipTypeTokenStore(); propertyKeyTokenStore = neoStores.getPropertyKeyTokenStore(); propertyStore = neoStores.getPropertyStore(); - relationshipGroupStore = neoStores.getRelationshipGroupStore(); + RecordStore relationshipGroupStore = neoStores.getRelationshipGroupStore(); schemaStore = neoStores.getSchemaStore(); labelTokenStore = neoStores.getLabelTokenStore(); @@ -305,7 +302,6 @@ public BatchInserterImpl( final File storeDir, final FileSystemAbstraction fileS flushStrategy = new BatchedFlushStrategy( recordAccess, config.get( GraphDatabaseSettings .batch_inserter_batch_size ) ); - cursors = new RecordCursors( neoStores ); } private Map getDefaultParams() @@ -904,7 +900,7 @@ public Map getNodeProperties( long nodeId ) public Iterable getRelationshipIds( long nodeId ) { flushStrategy.forceFlush(); - return new BatchRelationshipIterable( neoStores, nodeId, cursors ) + return new BatchRelationshipIterable( neoStores, nodeId ) { @Override protected Long nextFrom( long relId, int type, long startNode, long endNode ) @@ -918,7 +914,7 @@ protected Long nextFrom( long relId, int type, long startNode, long endNode ) public Iterable getRelationships( long nodeId ) { flushStrategy.forceFlush(); - return new BatchRelationshipIterable( neoStores, nodeId, cursors ) + return new BatchRelationshipIterable( neoStores, nodeId ) { @Override protected BatchRelationship nextFrom( long relId, int type, long startNode, long endNode ) @@ -972,7 +968,6 @@ public void shutdown() } finally { - cursors.close(); neoStores.close(); try diff --git a/community/kernel/src/main/java/org/neo4j/unsafe/batchinsert/internal/BatchRelationshipIterable.java b/community/kernel/src/main/java/org/neo4j/unsafe/batchinsert/internal/BatchRelationshipIterable.java index 3310ef2f47da..822bb15ef43f 100644 --- a/community/kernel/src/main/java/org/neo4j/unsafe/batchinsert/internal/BatchRelationshipIterable.java +++ b/community/kernel/src/main/java/org/neo4j/unsafe/batchinsert/internal/BatchRelationshipIterable.java @@ -27,11 +27,9 @@ import org.neo4j.kernel.impl.store.InvalidRecordException; import org.neo4j.kernel.impl.store.NeoStores; import org.neo4j.kernel.impl.store.NodeStore; -import org.neo4j.kernel.impl.store.RecordCursors; -import org.neo4j.kernel.impl.store.RecordStore; +import org.neo4j.kernel.impl.store.RelationshipGroupStore; import org.neo4j.kernel.impl.store.RelationshipStore; import org.neo4j.kernel.impl.store.record.NodeRecord; -import org.neo4j.kernel.impl.store.record.RelationshipGroupRecord; import static org.neo4j.kernel.impl.locking.LockService.NO_LOCK_SERVICE; import static org.neo4j.kernel.impl.store.record.RecordLoad.NORMAL; @@ -41,13 +39,12 @@ abstract class BatchRelationshipIterable implements Iterable { private final StoreNodeRelationshipCursor relationshipCursor; - BatchRelationshipIterable( NeoStores neoStores, long nodeId, RecordCursors cursors ) + BatchRelationshipIterable( NeoStores neoStores, long nodeId ) { RelationshipStore relationshipStore = neoStores.getRelationshipStore(); - RecordStore relationshipGroupStore = neoStores.getRelationshipGroupStore(); - RelationshipGroupRecord relationshipGroupRecord = relationshipGroupStore.newRecord(); - this.relationshipCursor = new StoreNodeRelationshipCursor( relationshipStore, relationshipGroupRecord, - cursor -> {}, cursors, NO_LOCK_SERVICE ); + RelationshipGroupStore relationshipGroupStore = neoStores.getRelationshipGroupStore(); + this.relationshipCursor = new StoreNodeRelationshipCursor( relationshipStore, relationshipGroupStore, + cursor -> {}, NO_LOCK_SERVICE ); // TODO There's an opportunity to reuse lots of instances created here, but this isn't a // critical path instance so perhaps not necessary a.t.m. diff --git a/community/kernel/src/test/java/org/neo4j/kernel/impl/api/store/StoreNodeRelationshipCursorTest.java b/community/kernel/src/test/java/org/neo4j/kernel/impl/api/store/StoreNodeRelationshipCursorTest.java index 305b3ecbebe5..b79eb0b40bf0 100644 --- a/community/kernel/src/test/java/org/neo4j/kernel/impl/api/store/StoreNodeRelationshipCursorTest.java +++ b/community/kernel/src/test/java/org/neo4j/kernel/impl/api/store/StoreNodeRelationshipCursorTest.java @@ -48,8 +48,8 @@ import org.neo4j.kernel.impl.api.state.TxState; import org.neo4j.kernel.impl.pagecache.ConfiguringPageCacheFactory; import org.neo4j.kernel.impl.store.NeoStores; -import org.neo4j.kernel.impl.store.RecordCursors; import org.neo4j.kernel.impl.store.RecordStore; +import org.neo4j.kernel.impl.store.RelationshipGroupStore; import org.neo4j.kernel.impl.store.RelationshipStore; import org.neo4j.kernel.impl.store.StoreFactory; import org.neo4j.kernel.impl.store.record.Record; @@ -72,6 +72,7 @@ import static org.mockito.Matchers.eq; import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; import static org.neo4j.graphdb.factory.GraphDatabaseSettings.pagecache_memory; import static org.neo4j.helpers.collection.MapUtil.stringMap; @@ -141,7 +142,7 @@ public void retrieveNodeRelationships() throws Exception { createNodeRelationships(); - try ( StoreNodeRelationshipCursor cursor = getNodeRelationshipCursor() ) + try ( StoreNodeRelationshipCursor cursor = nodeRelationshipCursor() ) { cursor.init( dense, 1L, FIRST_OWNING_NODE, direction, null ); assertTrue( cursor.next() ); @@ -159,7 +160,7 @@ public void retrieveUsedRelationshipChain() { createRelationshipChain( 4 ); long expectedNodeId = 1; - try ( StoreNodeRelationshipCursor cursor = getNodeRelationshipCursor() ) + try ( StoreNodeRelationshipCursor cursor = nodeRelationshipCursor() ) { cursor.init( dense, 1, FIRST_OWNING_NODE, direction, null ); while ( cursor.next() ) @@ -177,7 +178,7 @@ public void retrieveRelationshipChainWithUnusedLink() unUseRecord( 3 ); int[] expectedRelationshipIds = new int[]{1, 2, 4}; int relationshipIndex = 0; - try ( StoreNodeRelationshipCursor cursor = getNodeRelationshipCursor() ) + try ( StoreNodeRelationshipCursor cursor = nodeRelationshipCursor() ) { cursor.init( dense, 1, FIRST_OWNING_NODE, direction, null ); while ( cursor.next() ) @@ -195,7 +196,7 @@ public void shouldHandleDenseNodeWithNoRelationships() throws Exception // but we don't downgrade dense --> sparse when we delete relationships. So if we have a dense node // which no longer has relationships, there was this assumption that we could just call getRecord // on the NodeRecord#getNextRel() value. Although that value could actually be -1 - try ( StoreNodeRelationshipCursor cursor = getNodeRelationshipCursor() ) + try ( StoreNodeRelationshipCursor cursor = nodeRelationshipCursor() ) { // WHEN cursor.init( dense, NO_NEXT_RELATIONSHIP.intValue(), FIRST_OWNING_NODE, direction, null ); @@ -279,6 +280,26 @@ public void shouldObserveCorrectAugmentedNodeRelationshipsState() throws Excepti } } + @Test + public void shouldClosePageCursorsWhenDisposed() + { + RelationshipStore relationshipStore = mock( RelationshipStore.class ); + PageCursor relationshipCursor = mock( PageCursor.class ); + when( relationshipStore.newPageCursor() ).thenReturn( relationshipCursor ); + RelationshipGroupStore relationshipGroupStore = mock( RelationshipGroupStore.class ); + PageCursor relationshipGroupCursor = mock( PageCursor.class ); + when( relationshipGroupStore.newPageCursor() ).thenReturn( relationshipGroupCursor ); + StoreNodeRelationshipCursor cursor = + new StoreNodeRelationshipCursor( relationshipStore, relationshipGroupStore, this::noCache, + NO_LOCK_SERVICE ); + + cursor.close(); + cursor.dispose(); + + verify( relationshipCursor ).close(); + verify( relationshipGroupCursor ).close(); + } + private Cursor cursor( long nodeId, Map rels, TxState state, Direction direction, int[] relationshipTypes ) throws IOException { @@ -313,8 +334,8 @@ private Cursor cursor( long nodeId, Map } ).when( relationshipStore ).readIntoRecord( anyLong(), eq( relationshipRecord ), any( RecordLoad.class ), any( PageCursor.class ) ); StoreNodeRelationshipCursor cursor = - new StoreNodeRelationshipCursor( relationshipStore, new RelationshipGroupRecord( -1 ), this::noCache, - mock( RecordCursors.class ), NO_LOCK_SERVICE ); + new StoreNodeRelationshipCursor( relationshipStore, mock( RelationshipGroupStore.class ), this::noCache, + NO_LOCK_SERVICE ); return relationshipTypes == null ? cursor.init( false, firstRelId, nodeId, direction, state ) @@ -505,12 +526,10 @@ private long getFirstNode() return direction == OUTGOING ? FIRST_OWNING_NODE : SECOND_OWNING_NODE; } - private StoreNodeRelationshipCursor getNodeRelationshipCursor() + private StoreNodeRelationshipCursor nodeRelationshipCursor() { - RelationshipGroupRecord groupRecord = new RelationshipGroupRecord( -1 ); - groupRecord.setType( -1 ); - return new StoreNodeRelationshipCursor( neoStores.getRelationshipStore(), groupRecord, this::noCache, - new RecordCursors( neoStores ), NO_LOCK_SERVICE ); + return new StoreNodeRelationshipCursor( neoStores.getRelationshipStore(), neoStores.getRelationshipGroupStore(), + this::noCache, NO_LOCK_SERVICE ); } private void noCache( Cursor cursor ) diff --git a/community/kernel/src/test/java/org/neo4j/kernel/impl/transaction/command/NeoStoreTransactionApplierTest.java b/community/kernel/src/test/java/org/neo4j/kernel/impl/transaction/command/NeoStoreTransactionApplierTest.java index 11e53ba8f09b..edf784140dbe 100644 --- a/community/kernel/src/test/java/org/neo4j/kernel/impl/transaction/command/NeoStoreTransactionApplierTest.java +++ b/community/kernel/src/test/java/org/neo4j/kernel/impl/transaction/command/NeoStoreTransactionApplierTest.java @@ -100,7 +100,7 @@ public class NeoStoreTransactionApplierTest private final NodeStore nodeStore = mock( NodeStore.class ); private final RelationshipStore relationshipStore = mock( RelationshipStore.class ); private final PropertyStore propertyStore = mock( PropertyStore.class ); - private final RecordStore relationshipGroupStore = mock( RelationshipGroupStore.class ); + private final RelationshipGroupStore relationshipGroupStore = mock( RelationshipGroupStore.class ); private final RelationshipTypeTokenStore relationshipTypeTokenStore = mock( RelationshipTypeTokenStore.class ); private final LabelTokenStore labelTokenStore = mock( LabelTokenStore.class ); private final PropertyKeyTokenStore propertyKeyTokenStore = mock( PropertyKeyTokenStore.class );