Skip to content

Commit

Permalink
SeekCursor refactorings
Browse files Browse the repository at this point in the history 
- broke out rereading from node header into method.
- wrote more comments
- simplified some conditions and made some conditions clearer
  • Loading branch information
@tinwelint
tinwelint committed Dec 16, 2016
1 parent c5b6095 commit a2b2864
Show file tree
Hide file tree
Showing 3 changed files with 114 additions and 129 deletions.
76 changes: 0 additions & 76 deletions community/index/src/main/java/org/neo4j/index/gbptree/MutableHit.java
View file

This file was deleted.

149 changes: 98 additions & 51 deletions community/index/src/main/java/org/neo4j/index/gbptree/SeekCursor.java
View file
Expand Up @@ -42,27 +42,26 @@
* <p> * <p>
* Implementation note: there are assumptions that keys are unique in the tree. * Implementation note: there are assumptions that keys are unique in the tree.
*/ */
class SeekCursor<KEY,VALUE> implements RawCursor<Hit<KEY,VALUE>,IOException> class SeekCursor<KEY,VALUE> implements RawCursor<Hit<KEY,VALUE>,IOException>, Hit<KEY,VALUE>
{ {
private final PageCursor cursor; private final PageCursor cursor;
private final KEY mutableKey; private final KEY mutableKey;
private final VALUE mutableValue; private final VALUE mutableValue;
private final KEY fromInclusive; private final KEY fromInclusive;
private final KEY toExclusive; private final KEY toExclusive;
private final Layout<KEY,VALUE> layout; private final Layout<KEY,VALUE> layout;
private final MutableHit<KEY,VALUE> hit;
private final TreeNode<KEY,VALUE> bTreeNode; private final TreeNode<KEY,VALUE> bTreeNode;
private final KEY prevKey; private final KEY prevKey;
private final LongSupplier generationSupplier; private final LongSupplier generationSupplier;
private boolean first = true; private boolean first = true;
private long stableGeneration;
private long unstableGeneration;


// data structures for the current b-tree node // data structures for the current b-tree node
private int pos; private int pos;
private int keyCount; private int keyCount;
private boolean reread; private boolean rereadHeader;
private boolean resetPosition; private boolean rediscoverKeyPosition;
private long stableGeneration;
private long unstableGeneration;


SeekCursor( PageCursor leafCursor, KEY mutableKey, VALUE mutableValue, TreeNode<KEY,VALUE> bTreeNode, SeekCursor( PageCursor leafCursor, KEY mutableKey, VALUE mutableValue, TreeNode<KEY,VALUE> bTreeNode,
KEY fromInclusive, KEY toExclusive, Layout<KEY,VALUE> layout, KEY fromInclusive, KEY toExclusive, Layout<KEY,VALUE> layout,
Expand All @@ -77,7 +76,6 @@ class SeekCursor<KEY,VALUE> implements RawCursor<Hit<KEY,VALUE>,IOException>
this.stableGeneration = stableGeneration; this.stableGeneration = stableGeneration;
this.unstableGeneration = unstableGeneration; this.unstableGeneration = unstableGeneration;
this.generationSupplier = generationSupplier; this.generationSupplier = generationSupplier;
this.hit = new MutableHit<>( mutableKey, mutableValue );
this.bTreeNode = bTreeNode; this.bTreeNode = bTreeNode;
this.prevKey = layout.newKey(); this.prevKey = layout.newKey();


Expand Down Expand Up @@ -138,15 +136,15 @@ else if ( TreeNode.isNode( newGen ) )
} }
while ( isInternal && keyCount > 0 ); 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.pos = pos - 1;
this.keyCount = keyCount; this.keyCount = keyCount;
} }


@Override @Override
public Hit<KEY,VALUE> get() public Hit<KEY,VALUE> get()
{ {
return hit; return this;
} }


@Override @Override
Expand All @@ -160,31 +158,20 @@ public boolean next() throws IOException
do do
{ {
newGen = bTreeNode.newGen( cursor, stableGeneration, unstableGeneration ); newGen = bTreeNode.newGen( cursor, stableGeneration, unstableGeneration );
if ( reread ) if ( rereadHeader )
{ {
keyCount = bTreeNode.keyCount( cursor ); rereadCurrentNodeHeader();

}
// Keys could have been moved to the left so we need to make sure we are not missing any keys by if ( rediscoverKeyPosition )
// moving position back until we find previously returned key {
if ( resetPosition ) rediscoverKeyPosition();
{
if ( !first )
{
int searchResult = KeySearch.search( cursor, bTreeNode, prevKey, mutableKey, keyCount );
pos = KeySearch.positionOf( searchResult );
}
else
{
pos = 0;
}
}
} }
// There's a condition in here, choosing between go to next sibling or value, // 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 // 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. // which has by then been consistently read. No decision can be made in here directly.
if ( pos >= keyCount ) if ( pos >= keyCount )
{ {
// Go to next sibling // Read right sibling
rightSibling = bTreeNode.rightSibling( cursor, stableGeneration, unstableGeneration ); rightSibling = bTreeNode.rightSibling( cursor, stableGeneration, unstableGeneration );
} }
else else
Expand All @@ -194,65 +181,113 @@ public boolean next() throws IOException
bTreeNode.valueAt( cursor, mutableValue, pos ); bTreeNode.valueAt( cursor, mutableValue, pos );
} }
} }
while ( resetPosition = reread = cursor.shouldRetry() ); while ( rediscoverKeyPosition = rereadHeader = cursor.shouldRetry() );
checkOutOfBounds( cursor ); checkOutOfBounds( cursor );


// Go to newGen if read successfully and
if ( pointerCheckingWithGenerationCatchup( newGen, true ) ) 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; continue;
} }
else if ( TreeNode.isNode( newGen ) ) 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 ); bTreeNode.goTo( cursor, "new gen", newGen, stableGeneration, unstableGeneration );
reread = resetPosition = true; rereadHeader = rediscoverKeyPosition = true;
continue; continue;
} }


if ( pos >= keyCount ) 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 ) ) 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; continue;
} }
else if ( TreeNode.isNode( rightSibling ) ) else if ( TreeNode.isNode( rightSibling ) )
{ {
// TODO: Check if rightSibling is within expected range before calling next. // TODO: Check if rightSibling is within expected range before calling next.
// TODO: Possibly by getting highest expected from IdProvider // TODO: Possibly by getting highest expected from IdProvider
bTreeNode.goTo( cursor, "right sibling", rightSibling, stableGeneration, unstableGeneration ); bTreeNode.goTo( cursor, "right sibling", rightSibling, stableGeneration, unstableGeneration );
pos = -1; if ( first )
reread = true; {
// 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 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 || // too far to the left possibly because page reuse
( !first && layout.compare( prevKey, mutableKey ) >= 0 ) ) rereadHeader = rediscoverKeyPosition = true;
{ continue;
// We've come across a bad read in the middle of a split }
// This is outlined in InternalTreeLogic, skip this value (it's fine) else if ( !first && layout.compare( prevKey, mutableKey ) >= 0 )
reread = true; {
continue; // 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 // A hit, it's within the range we search for
if ( first ) if ( first )
{ {
first = false; // Setting first to false include an additional check for coming potential
} // hits so that we cannot go backwards in our result. Going backwards can
layout.copyKey( mutableKey, prevKey ); // happen when reading through concurrent splits or similar and is a benign
return true; // 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; 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 ) private boolean pointerCheckingWithGenerationCatchup( long pointer, boolean allowNoNode )
{ {
if ( !GenSafePointerPair.isSuccess( pointer ) ) if ( !GenSafePointerPair.isSuccess( pointer ) )
Expand All @@ -274,6 +309,18 @@ private boolean pointerCheckingWithGenerationCatchup( long pointer, boolean allo
return false; return false;
} }


@Override
public KEY key()
{
return mutableKey;
}

@Override
public VALUE value()
{
return mutableValue;
}

@Override @Override
public void close() public void close()
{ {
Expand Down
18 changes: 16 additions & 2 deletions community/index/src/test/java/org/neo4j/index/gbptree/GBPTreeTest.java
View file
Expand Up @@ -625,7 +625,7 @@ public void shouldSplitCorrectly() throws Exception
} }


@Test @Test
public void shouldReadCorrectlyWhenConcurrentlyInserting() throws Throwable public void shouldReadCorrectlyWhenConcurrentlyInsertingInOrder() throws Throwable
{ {
// GIVEN // GIVEN
int maxCheckpointInterval = random.intBetween( 50, 400 ); int maxCheckpointInterval = random.intBetween( 50, 400 );
Expand Down Expand Up @@ -655,10 +655,15 @@ public void shouldReadCorrectlyWhenConcurrentlyInserting() throws Throwable
// Read one go, we should see up to highId // Read one go, we should see up to highId
long start = Long.max( 0, upToId - 1000 ); long start = Long.max( 0, upToId - 1000 );
long lastSeen = start - 1; long lastSeen = start - 1;
long startTime;
long startTimeLeaf;
long endTime;
startTime = System.currentTimeMillis();
try ( RawCursor<Hit<MutableLong,MutableLong>,IOException> cursor = try ( RawCursor<Hit<MutableLong,MutableLong>,IOException> cursor =
// "to" is exclusive so do +1 on that // "to" is exclusive so do +1 on that
index.seek( new MutableLong( start ), new MutableLong( upToId + 1 ) ) ) index.seek( new MutableLong( start ), new MutableLong( upToId + 1 ) ) )
{ {
startTimeLeaf = System.currentTimeMillis();
while ( cursor.next() ) while ( cursor.next() )
{ {
MutableLong hit = cursor.get().key(); MutableLong hit = cursor.get().key();
Expand All @@ -671,12 +676,16 @@ public void shouldReadCorrectlyWhenConcurrentlyInserting() throws Throwable
assertEquals( lastSeen + 1, hit.longValue() ); assertEquals( lastSeen + 1, hit.longValue() );
lastSeen = hit.longValue(); lastSeen = hit.longValue();
} }
endTime = System.currentTimeMillis();
} }
// It's possible that the writer has gone further since we started, // It's possible that the writer has gone further since we started,
// but we should at least have seen upToId // but we should at least have seen upToId
if ( lastSeen < 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 // Keep a local counter and update the global one now and then, we don't want
Expand Down Expand Up @@ -766,6 +775,11 @@ public void shouldReadCorrectlyWhenConcurrentlyInserting() throws Throwable
} }
} }


// todo shouldReadCorrectlyWhenConcurrentlyInsertingOutOfOrder
// todo shouldReadCorrectlyWhenReadSpansSingleCheckpoint
// todo shouldReadCorrectlyWhenReadSpansMultipleCheckpoints
// todo readKillerTestNextCheckpointNextCheckpointNext...

private static void randomlyModifyIndex( Index<MutableLong,MutableLong> index, private static void randomlyModifyIndex( Index<MutableLong,MutableLong> index,
Map<MutableLong,MutableLong> data, Random random ) throws IOException Map<MutableLong,MutableLong> data, Random random ) throws IOException
{ {
Expand Down

0 comments on commit a2b2864

Please sign in to comment.