From a2b2864ed93ba09a40bedf15279e7278a2986c15 Mon Sep 17 00:00:00 2001 From: Mattias Persson Date: Wed, 7 Dec 2016 13:29:27 +0100 Subject: [PATCH] SeekCursor refactorings - broke out rereading from node header into method. - wrote more comments - simplified some conditions and made some conditions clearer --- .../org/neo4j/index/gbptree/MutableHit.java | 76 --------- .../org/neo4j/index/gbptree/SeekCursor.java | 149 ++++++++++++------ .../org/neo4j/index/gbptree/GBPTreeTest.java | 18 ++- 3 files changed, 114 insertions(+), 129 deletions(-) delete mode 100644 community/index/src/main/java/org/neo4j/index/gbptree/MutableHit.java diff --git a/community/index/src/main/java/org/neo4j/index/gbptree/MutableHit.java b/community/index/src/main/java/org/neo4j/index/gbptree/MutableHit.java deleted file mode 100644 index cc3bf0e7af0a0..0000000000000 --- a/community/index/src/main/java/org/neo4j/index/gbptree/MutableHit.java +++ /dev/null @@ -1,76 +0,0 @@ -/* - * Copyright (c) 2002-2016 "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.gbptree; - -import org.neo4j.index.Hit; - -/** - * Straight-forward implement of {@link Hit} where focus is on zero garbage. This instances can be used - * for multiple hits. Key/value instances are provided, its values overwritten for every hit and - * merely be exposed in {@link #key()} and {@link #value()}, where caller must adhere to not keeping - * references to those instances after reading them. - * - * @param type of key - * @param type of value - */ -public class MutableHit implements Hit -{ - private final KEY key; - private final VALUE value; - - /** - * Constructs a new {@link MutableHit} where the provided {@code key} and {@code value} are single - * instances to be re-used and overwritten for every hit in a result set. - * - * @param key KEY instance to reuse for every hit. - * @param value VALUE instance to reuse for every hit. - */ - public MutableHit( KEY key, VALUE value ) - { - this.key = key; - this.value = value; - } - - /** - * @return key instance containing current key. This instance will have its value overwritten for next - * hit and so no reference to the returned instances must be kept after reading it. - */ - @Override - public KEY key() - { - return key; - } - - /** - * @return value instance containing current value. This instance will have its value overwritten for next - * hit and so no reference to the returned instances must be kept after reading it. - */ - @Override - public VALUE value() - { - return value; - } - - @Override - public String toString() - { - return "[key:" + key + ", value:" + value + "]"; - } -} diff --git a/community/index/src/main/java/org/neo4j/index/gbptree/SeekCursor.java b/community/index/src/main/java/org/neo4j/index/gbptree/SeekCursor.java index 608ab6e14d7d5..6c1f36b36d37b 100644 --- a/community/index/src/main/java/org/neo4j/index/gbptree/SeekCursor.java +++ b/community/index/src/main/java/org/neo4j/index/gbptree/SeekCursor.java @@ -42,7 +42,7 @@ *

* Implementation note: there are assumptions that keys are unique in the tree. */ -class SeekCursor implements RawCursor,IOException> +class SeekCursor implements RawCursor,IOException>, Hit { private final PageCursor cursor; private final KEY mutableKey; @@ -50,19 +50,18 @@ class SeekCursor implements RawCursor,IOException> private final KEY fromInclusive; private final KEY toExclusive; private final Layout layout; - private final MutableHit hit; private final TreeNode bTreeNode; private final KEY prevKey; private final LongSupplier generationSupplier; private boolean first = true; + private long stableGeneration; + private long unstableGeneration; // data structures for the current b-tree node private int pos; private int keyCount; - private boolean reread; - private boolean resetPosition; - private long stableGeneration; - private long unstableGeneration; + private boolean rereadHeader; + private boolean rediscoverKeyPosition; SeekCursor( PageCursor leafCursor, KEY mutableKey, VALUE mutableValue, TreeNode bTreeNode, KEY fromInclusive, KEY toExclusive, Layout layout, @@ -77,7 +76,6 @@ class SeekCursor implements RawCursor,IOException> this.stableGeneration = stableGeneration; this.unstableGeneration = unstableGeneration; this.generationSupplier = generationSupplier; - this.hit = new MutableHit<>( mutableKey, mutableValue ); this.bTreeNode = bTreeNode; this.prevKey = layout.newKey(); @@ -138,7 +136,7 @@ else if ( TreeNode.isNode( newGen ) ) } while ( isInternal && keyCount > 0 ); - // We've no come to the first relevant leaf, initialize the state for the coming leaf scan + // We've now come to the first relevant leaf, initialize the state for the coming leaf scan this.pos = pos - 1; this.keyCount = keyCount; } @@ -146,7 +144,7 @@ else if ( TreeNode.isNode( newGen ) ) @Override public Hit get() { - return hit; + return this; } @Override @@ -160,31 +158,20 @@ public boolean next() throws IOException do { newGen = bTreeNode.newGen( cursor, stableGeneration, unstableGeneration ); - if ( reread ) + if ( rereadHeader ) { - keyCount = bTreeNode.keyCount( cursor ); - - // Keys could have been moved to the left so we need to make sure we are not missing any keys by - // moving position back until we find previously returned key - if ( resetPosition ) - { - if ( !first ) - { - int searchResult = KeySearch.search( cursor, bTreeNode, prevKey, mutableKey, keyCount ); - pos = KeySearch.positionOf( searchResult ); - } - else - { - pos = 0; - } - } + rereadCurrentNodeHeader(); + } + if ( rediscoverKeyPosition ) + { + rediscoverKeyPosition(); } // There's a condition in here, choosing between go to next sibling or value, // this condition is mirrored outside the shouldRetry loop to act upon the data // which has by then been consistently read. No decision can be made in here directly. if ( pos >= keyCount ) { - // Go to next sibling + // Read right sibling rightSibling = bTreeNode.rightSibling( cursor, stableGeneration, unstableGeneration ); } else @@ -194,26 +181,36 @@ public boolean next() throws IOException bTreeNode.valueAt( cursor, mutableValue, pos ); } } - while ( resetPosition = reread = cursor.shouldRetry() ); + while ( rediscoverKeyPosition = rereadHeader = cursor.shouldRetry() ); checkOutOfBounds( cursor ); + // Go to newGen if read successfully and if ( pointerCheckingWithGenerationCatchup( newGen, true ) ) { - reread = resetPosition = true; + // Reading newGen pointer resulted in a bad read, but generation had changed (a checkpoint has + // occurred since we started this cursor) so the generation fields in this cursor are now updated + // with the latest, so let's try that read again. + rereadHeader = rediscoverKeyPosition = true; continue; } else if ( TreeNode.isNode( newGen ) ) { + // We ended up on a node which has a newGen set, let's go to it and read from that one instead. bTreeNode.goTo( cursor, "new gen", newGen, stableGeneration, unstableGeneration ); - reread = resetPosition = true; + rereadHeader = rediscoverKeyPosition = true; continue; } if ( pos >= keyCount ) { + // We've exhausted this node, it's time to see if there's a right sibling to go to. + if ( pointerCheckingWithGenerationCatchup( rightSibling, true ) ) { - reread = resetPosition = true; + // Reading rightSibling pointer resulted in a bad read, but generation had changed + // (a checkpoint has occurred since we started this cursor) so the generation fields in this + // cursor are now updated with the latest, so let's try that read again. + rereadHeader = rediscoverKeyPosition = true; continue; } else if ( TreeNode.isNode( rightSibling ) ) @@ -221,38 +218,76 @@ else if ( TreeNode.isNode( rightSibling ) ) // TODO: Check if rightSibling is within expected range before calling next. // TODO: Possibly by getting highest expected from IdProvider bTreeNode.goTo( cursor, "right sibling", rightSibling, stableGeneration, unstableGeneration ); - pos = -1; - reread = true; + if ( first ) + { + // Have not yet found first hit among leaves. + // First hit can be several leaves to the right. + // Continue to use binary search in right leaf + rereadHeader = rediscoverKeyPosition = true; + } + else + { + // It is likely that first key in right sibling is a next hit. + // Continue using scan + pos = -1; + rereadHeader = true; + } continue; // in the outer loop, with the position reset to the beginning of the right sibling } } - else + else if ( layout.compare( mutableKey, toExclusive ) < 0 ) { - if ( layout.compare( mutableKey, toExclusive ) < 0 ) + if ( layout.compare( mutableKey, fromInclusive ) < 0 ) { - if ( layout.compare( mutableKey, fromInclusive ) < 0 || - ( !first && layout.compare( prevKey, mutableKey ) >= 0 ) ) - { - // We've come across a bad read in the middle of a split - // This is outlined in InternalTreeLogic, skip this value (it's fine) - reread = true; - continue; - } + // too far to the left possibly because page reuse + rereadHeader = rediscoverKeyPosition = true; + continue; + } + else if ( !first && layout.compare( prevKey, mutableKey ) >= 0 ) + { + // We've come across a bad read in the middle of a split + // This is outlined in InternalTreeLogic, skip this value (it's fine) + rereadHeader = true; + continue; + } - // A hit - if ( first ) - { - first = false; - } - layout.copyKey( mutableKey, prevKey ); - return true; + // A hit, it's within the range we search for + if ( first ) + { + // Setting first to false include an additional check for coming potential + // hits so that we cannot go backwards in our result. Going backwards can + // happen when reading through concurrent splits or similar and is a benign + // temporary observed state. + first = false; } - // else we've come too far and so this means the end of the result set + layout.copyKey( mutableKey, prevKey ); + return true; } + // We've come too far and so this means the end of the result set return false; } } + private void rereadCurrentNodeHeader() + { + keyCount = bTreeNode.keyCount( cursor ); + } + + private void rediscoverKeyPosition() + { + // Keys could have been moved to the left so we need to make sure we are not missing any keys by + // moving position back until we find previously returned key + if ( !first ) + { + int searchResult = KeySearch.search( cursor, bTreeNode, prevKey, mutableKey, keyCount ); + pos = KeySearch.positionOf( searchResult ); + } + else + { + pos = 0; + } + } + private boolean pointerCheckingWithGenerationCatchup( long pointer, boolean allowNoNode ) { if ( !GenSafePointerPair.isSuccess( pointer ) ) @@ -274,6 +309,18 @@ private boolean pointerCheckingWithGenerationCatchup( long pointer, boolean allo return false; } + @Override + public KEY key() + { + return mutableKey; + } + + @Override + public VALUE value() + { + return mutableValue; + } + @Override public void close() { diff --git a/community/index/src/test/java/org/neo4j/index/gbptree/GBPTreeTest.java b/community/index/src/test/java/org/neo4j/index/gbptree/GBPTreeTest.java index 95c1c5593ef81..045b1de862f5c 100644 --- a/community/index/src/test/java/org/neo4j/index/gbptree/GBPTreeTest.java +++ b/community/index/src/test/java/org/neo4j/index/gbptree/GBPTreeTest.java @@ -625,7 +625,7 @@ public void shouldSplitCorrectly() throws Exception } @Test - public void shouldReadCorrectlyWhenConcurrentlyInserting() throws Throwable + public void shouldReadCorrectlyWhenConcurrentlyInsertingInOrder() throws Throwable { // GIVEN int maxCheckpointInterval = random.intBetween( 50, 400 ); @@ -655,10 +655,15 @@ public void shouldReadCorrectlyWhenConcurrentlyInserting() throws Throwable // Read one go, we should see up to highId long start = Long.max( 0, upToId - 1000 ); long lastSeen = start - 1; + long startTime; + long startTimeLeaf; + long endTime; + startTime = System.currentTimeMillis(); try ( RawCursor,IOException> cursor = // "to" is exclusive so do +1 on that index.seek( new MutableLong( start ), new MutableLong( upToId + 1 ) ) ) { + startTimeLeaf = System.currentTimeMillis(); while ( cursor.next() ) { MutableLong hit = cursor.get().key(); @@ -671,12 +676,16 @@ public void shouldReadCorrectlyWhenConcurrentlyInserting() throws Throwable assertEquals( lastSeen + 1, hit.longValue() ); lastSeen = hit.longValue(); } + endTime = System.currentTimeMillis(); } // It's possible that the writer has gone further since we started, // but we should at least have seen upToId if ( lastSeen < upToId ) { - fail( "Seeked " + start + " - " + upToId + " (inclusive), but only saw " + lastSeen ); + fail( "Seeked " + start + " - " + upToId + " (inclusive), but only saw " + lastSeen + + ". Read took " + (endTime - startTime) + "ms," + + " of which " + (endTime - startTimeLeaf) + "ms among leaves. " + + "MaxCheckpointInterval=" + maxCheckpointInterval ); } // Keep a local counter and update the global one now and then, we don't want @@ -766,6 +775,11 @@ public void shouldReadCorrectlyWhenConcurrentlyInserting() throws Throwable } } + // todo shouldReadCorrectlyWhenConcurrentlyInsertingOutOfOrder + // todo shouldReadCorrectlyWhenReadSpansSingleCheckpoint + // todo shouldReadCorrectlyWhenReadSpansMultipleCheckpoints + // todo readKillerTestNextCheckpointNextCheckpointNext... + private static void randomlyModifyIndex( Index index, Map data, Random random ) throws IOException {