From a1fcae1e766ed760adb9a98e184fe8f0d10836f5 Mon Sep 17 00:00:00 2001 From: Anton Persson Date: Thu, 28 Dec 2017 11:08:06 +0100 Subject: [PATCH] TreeNodeDynamicSize impl part 11 - Pass GBPTreeIT TreeNodeDynamicSize - In doDefragment, avoid copyTo with lastBlockSize==0. Because then targetOffset is pageSize and that causes OutOfBoundsException - Don't rebalance from left to right if left has a smaller active space than right. This can be the case for the leftmost leaf. - Update deadSpace and keyCount when moving keys and values. - Update deadSpace when moving keys and children. Also - (GBPTree) Remove can now cause split in root, handle structure changes accordingly - (GBPTree) printNode to make debugging easier - (GBPTreeITBase) Remove @After for simplicity - (SimpleByteArrayLayout) Better safety in getSeed to not throw when reading crap data inside shouldRetry. --- .../neo4j/index/internal/gbptree/GBPTree.java | 70 +++--- .../internal/gbptree/TreeNodeDynamicSize.java | 35 ++- .../gbptree/GBPTreeDynamixSizeIT.java | 37 ++++ .../index/internal/gbptree/GBPTreeITBase.java | 204 ++++++++++-------- .../gbptree/SimpleByteArrayLayout.java | 16 +- 5 files changed, 234 insertions(+), 128 deletions(-) create mode 100644 community/index/src/test/java/org/neo4j/index/internal/gbptree/GBPTreeDynamixSizeIT.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 97a1533a8d9fc..16b0ba40502c1 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 @@ -1046,7 +1046,7 @@ void printTree() throws IOException * @param printState whether or not to print the tree state. * @throws IOException on I/O error. */ - public void printTree( boolean printValues, boolean printPosition, boolean printState ) throws IOException + void printTree( boolean printValues, boolean printPosition, boolean printState ) throws IOException { try ( PageCursor cursor = openRootCursor( PagedFile.PF_SHARED_READ_LOCK ) ) { @@ -1054,6 +1054,27 @@ public void printTree( boolean printValues, boolean printPosition, boolean print .printTree( cursor, cursor, System.out, printValues, printPosition, printState ); } } + // Utility method + /** + * Print node with given id to System.out, if node with id exists. + * @param id the page id of node to print + */ + void printNode( int id ) throws IOException + { + if ( id < freeList.lastId() ) + { + // Use write lock to avoid adversary interference + try ( PageCursor cursor = pagedFile.io( id, PagedFile.PF_SHARED_WRITE_LOCK ) ) + { + cursor.next(); + byte nodeType = TreeNode.nodeType( cursor ); + if ( nodeType == TreeNode.NODE_TYPE_TREE_NODE ) + { + bTreeNode.printNode( cursor, false, true, stableGeneration( generation ), unstableGeneration( generation ) ); + } + } + } + } // Utility method boolean consistencyCheck() throws IOException @@ -1185,25 +1206,7 @@ public void merge( KEY key, VALUE value, ValueMerger valueMerger ) th throw e; } - if ( structurePropagation.hasRightKeyInsert ) - { - // New root - long newRootId = freeList.acquireNewId( stableGeneration, unstableGeneration ); - PageCursorUtil.goTo( cursor, "new root", newRootId ); - - bTreeNode.initializeInternal( cursor, stableGeneration, unstableGeneration ); - bTreeNode.setChildAt( cursor, structurePropagation.midChild, 0, - stableGeneration, unstableGeneration ); - bTreeNode.insertKeyAndRightChildAt( cursor, structurePropagation.rightKey, structurePropagation.rightChild, 0, 0, - stableGeneration, unstableGeneration ); - TreeNode.setKeyCount( cursor, 1 ); - setRoot( newRootId ); - } - else if ( structurePropagation.hasMidChildUpdate ) - { - setRoot( structurePropagation.midChild ); - } - structurePropagation.clear(); + handleStructureChanges(); checkOutOfBounds( cursor ); } @@ -1230,14 +1233,33 @@ public VALUE remove( KEY key ) throws IOException throw e; } - if ( structurePropagation.hasMidChildUpdate ) + handleStructureChanges(); + + checkOutOfBounds( cursor ); + return result; + } + + private void handleStructureChanges() throws IOException + { + if ( structurePropagation.hasRightKeyInsert ) + { + // New root + long newRootId = freeList.acquireNewId( stableGeneration, unstableGeneration ); + PageCursorUtil.goTo( cursor, "new root", newRootId ); + + bTreeNode.initializeInternal( cursor, stableGeneration, unstableGeneration ); + bTreeNode.setChildAt( cursor, structurePropagation.midChild, 0, + stableGeneration, unstableGeneration ); + bTreeNode.insertKeyAndRightChildAt( cursor, structurePropagation.rightKey, structurePropagation.rightChild, 0, 0, + stableGeneration, unstableGeneration ); + TreeNode.setKeyCount( cursor, 1 ); + setRoot( newRootId ); + } + else if ( structurePropagation.hasMidChildUpdate ) { setRoot( structurePropagation.midChild ); } structurePropagation.clear(); - - checkOutOfBounds( cursor ); - return result; } @Override diff --git a/community/index/src/main/java/org/neo4j/index/internal/gbptree/TreeNodeDynamicSize.java b/community/index/src/main/java/org/neo4j/index/internal/gbptree/TreeNodeDynamicSize.java index ecf59d20919db..251e5e2de0a83 100644 --- a/community/index/src/main/java/org/neo4j/index/internal/gbptree/TreeNodeDynamicSize.java +++ b/community/index/src/main/java/org/neo4j/index/internal/gbptree/TreeNodeDynamicSize.java @@ -483,9 +483,12 @@ For each dead space of size X (can be multiple consecutive dead keys) } // Move the last piece int lastBlockSize = deadRangeOffset - moveOffset; - deadRangeOffset -= lastBlockSize; - aliveRangeOffset -= lastBlockSize; - cursor.copyTo( deadRangeOffset, cursor, aliveRangeOffset, lastBlockSize ); + if ( lastBlockSize > 0 ) + { + deadRangeOffset -= lastBlockSize; + aliveRangeOffset -= lastBlockSize; + cursor.copyTo( deadRangeOffset, cursor, aliveRangeOffset, lastBlockSize ); + } } while ( !aliveKeysOffset.isEmpty() ); // Update allocOffset @@ -541,6 +544,11 @@ int canRebalanceLeaves( PageCursor leftCursor, int leftKeyCount, PageCursor righ // We can merge return -1; } + if ( leftActiveSpace < rightActiveSpace ) + { + // Moving keys to the right will only create more imbalance + return 0; + } int prevDelta; int currentDelta = Math.abs( leftActiveSpace - rightActiveSpace ); @@ -765,9 +773,11 @@ private void recordDeadAndAliveInternal( PageCursor cursor, PrimitiveIntStack de } } + // NOTE: Does update keyCount private void moveKeysAndValues( PageCursor fromCursor, int fromPos, PageCursor toCursor, int toPos, int count ) { int toAllocOffset = getAllocOffset( toCursor ); + int firstAllocOffset = toAllocOffset; for ( int i = 0; i < count; i++, toPos++ ) { toAllocOffset = transferRawKeyValue( fromCursor, fromPos + i, toCursor, toAllocOffset ); @@ -775,8 +785,17 @@ private void moveKeysAndValues( PageCursor fromCursor, int fromPos, PageCursor t putKeyOffset( toCursor, toAllocOffset ); } setAllocOffset( toCursor, toAllocOffset ); + + // Update deadspace + int deadSpace = getDeadSpace( fromCursor ); + int totalMovedBytes = firstAllocOffset - toAllocOffset; + setDeadSpace( fromCursor, deadSpace + totalMovedBytes ); + + // Key count + setKeyCount( fromCursor, fromPos ); } + // NOTE: Does NOT update keyCount private void moveKeysAndChildren( PageCursor fromCursor, int fromPos, PageCursor toCursor, int toPos, int count, boolean includeLeftMostChild ) { @@ -790,6 +809,7 @@ private void moveKeysAndChildren( PageCursor fromCursor, int fromPos, PageCursor // Move actual keys and update pointers int toAllocOffset = getAllocOffset( toCursor ); + int firstAllocOffset = toAllocOffset; for ( int i = 0; i < count; i++, toPos++ ) { // Key @@ -799,13 +819,18 @@ private void moveKeysAndChildren( PageCursor fromCursor, int fromPos, PageCursor } setAllocOffset( toCursor, toAllocOffset ); + // Update deadspace + int deadSpace = getDeadSpace( fromCursor ); + int totalMovedBytes = firstAllocOffset - toAllocOffset; + setDeadSpace( fromCursor, deadSpace + totalMovedBytes ); + // Zero pad empty area zeroPad( fromCursor, childFromOffset, lengthInBytes ); } - private void zeroPad( PageCursor fromCursor, int childFromOffset, int lengthInBytes ) + private void zeroPad( PageCursor fromCursor, int fromOffset, int lengthInBytes ) { - fromCursor.setOffset( childFromOffset ); + fromCursor.setOffset( fromOffset ); fromCursor.putBytes( lengthInBytes, (byte) 0 ); } diff --git a/community/index/src/test/java/org/neo4j/index/internal/gbptree/GBPTreeDynamixSizeIT.java b/community/index/src/test/java/org/neo4j/index/internal/gbptree/GBPTreeDynamixSizeIT.java new file mode 100644 index 0000000000000..474507808f3af --- /dev/null +++ b/community/index/src/test/java/org/neo4j/index/internal/gbptree/GBPTreeDynamixSizeIT.java @@ -0,0 +1,37 @@ +/* + * 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.test.rule.RandomRule; + +public class GBPTreeDynamixSizeIT extends GBPTreeITBase +{ + @Override + TestLayout getLayout( RandomRule random ) + { + return new SimpleByteArrayLayout(); + } + + @Override + Class getKeyClass() + { + return RawBytes.class; + } +} diff --git a/community/index/src/test/java/org/neo4j/index/internal/gbptree/GBPTreeITBase.java b/community/index/src/test/java/org/neo4j/index/internal/gbptree/GBPTreeITBase.java index bc6fe1eea7de8..a00817b84bf4d 100644 --- a/community/index/src/test/java/org/neo4j/index/internal/gbptree/GBPTreeITBase.java +++ b/community/index/src/test/java/org/neo4j/index/internal/gbptree/GBPTreeITBase.java @@ -19,7 +19,6 @@ */ package org.neo4j.index.internal.gbptree; -import org.junit.After; import org.junit.Rule; import org.junit.Test; import org.junit.rules.RuleChain; @@ -31,8 +30,6 @@ import java.util.Map; import java.util.Random; import java.util.TreeMap; -import java.util.concurrent.ExecutorService; -import java.util.concurrent.Executors; import org.neo4j.cursor.RawCursor; import org.neo4j.io.pagecache.IOLimiter; @@ -43,7 +40,6 @@ import org.neo4j.test.rule.fs.DefaultFileSystemRule; import static java.lang.Integer.max; -import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; @@ -62,7 +58,6 @@ public abstract class GBPTreeITBase private TestLayout layout; private GBPTree index; - private final ExecutorService threadPool = Executors.newFixedThreadPool( Runtime.getRuntime().availableProcessors() ); private GBPTree createIndex() throws IOException @@ -77,89 +72,80 @@ private GBPTree createIndex() abstract Class getKeyClass(); - @After - public void consistencyCheckAndClose() throws IOException - { - try - { - threadPool.shutdownNow(); - index.consistencyCheck(); - } - finally - { - index.close(); - } - } - @Test public void shouldStayCorrectAfterRandomModifications() throws Exception { // GIVEN - GBPTree index = createIndex(); - Comparator keyComparator = layout; - Map data = new TreeMap<>( keyComparator ); - int count = 100; - int totalNumberOfRounds = 10; - for ( int i = 0; i < count; i++ ) + try ( GBPTree index = createIndex() ) { - data.put( randomKey( random.random() ), randomValue( random.random() ) ); - } - - // WHEN - try ( Writer writer = index.writer() ) - { - for ( Map.Entry entry : data.entrySet() ) + Comparator keyComparator = layout; + Map data = new TreeMap<>( keyComparator ); + int count = 100; + int totalNumberOfRounds = 10; + for ( int i = 0; i < count; i++ ) { - writer.put( entry.getKey(), entry.getValue() ); + data.put( randomKey( random.random() ), randomValue( random.random() ) ); } - } - for ( int round = 0; round < totalNumberOfRounds; round++ ) - { - // THEN - for ( int i = 0; i < count; i++ ) + // WHEN + try ( Writer writer = index.writer() ) { - KEY first = randomKey( random.random() ); - KEY second = randomKey( random.random() ); - KEY from; - KEY to; - if ( layout.getSeed( first ) < layout.getSeed( second ) ) - { - from = first; - to = second; - } - else + for ( Map.Entry entry : data.entrySet() ) { - from = second; - to = first; + writer.put( entry.getKey(), entry.getValue() ); } - Map expectedHits = expectedHits( data, from, to, keyComparator ); - try ( RawCursor,IOException> result = index.seek( from, to ) ) + } + + for ( int round = 0; round < totalNumberOfRounds; round++ ) + { + // THEN + for ( int i = 0; i < count; i++ ) { - while ( result.next() ) + KEY first = randomKey( random.random() ); + KEY second = randomKey( random.random() ); + KEY from; + KEY to; + if ( layout.getSeed( first ) < layout.getSeed( second ) ) { - KEY key = result.get().key(); - if ( expectedHits.remove( key ) == null ) + from = first; + to = second; + } + else + { + from = second; + to = first; + } + Map expectedHits = expectedHits( data, from, to, keyComparator ); + try ( RawCursor,IOException> result = index.seek( from, to ) ) + { + while ( result.next() ) { - fail( "Unexpected hit " + key + " when searching for " + from + " - " + to ); - } + KEY key = result.get().key(); + if ( expectedHits.remove( key ) == null ) + { + fail( "Unexpected hit " + key + " when searching for " + from + " - " + to ); + } - assertTrue( keyComparator.compare( key, from ) >= 0 ); - if ( keyComparator.compare( from, to ) != 0 ) + assertTrue( keyComparator.compare( key, from ) >= 0 ); + if ( keyComparator.compare( from, to ) != 0 ) + { + assertTrue( keyComparator.compare( key, to ) < 0 ); + } + } + if ( !expectedHits.isEmpty() ) { - assertTrue( keyComparator.compare( key, to ) < 0 ); + fail( "There were results which were expected to be returned, but weren't:" + expectedHits + + " when searching range " + from + " - " + to ); } } - if ( !expectedHits.isEmpty() ) - { - fail( "There were results which were expected to be returned, but weren't:" + expectedHits + - " when searching range " + from + " - " + to ); - } } + + index.checkpoint( IOLimiter.unlimited() ); + randomlyModifyIndex( index, data, random.random(), (double) round / totalNumberOfRounds ); } - index.checkpoint( IOLimiter.unlimited() ); - randomlyModifyIndex( index, data, random.random(), (double) round / totalNumberOfRounds ); + // and finally + index.consistencyCheck(); } } @@ -167,46 +153,55 @@ public void shouldStayCorrectAfterRandomModifications() throws Exception public void shouldHandleRemoveEntireTree() throws Exception { // given - GBPTree index = createIndex(); - int numberOfNodes = 200_000; - try ( Writer writer = index.writer() ) + try ( GBPTree index = createIndex() ) { - for ( int i = 0; i < numberOfNodes; i++ ) + int numberOfNodes = 200_000; + try ( Writer writer = index.writer() ) { - writer.put( key( i ), value( i ) ); + for ( int i = 0; i < numberOfNodes; i++ ) + { + writer.put( key( i ), value( i ) ); + } } - } - // when - BitSet removed = new BitSet(); - try ( Writer writer = index.writer() ) - { - for ( int i = 0; i < numberOfNodes - numberOfNodes / 10; i++ ) + // when + BitSet removed = new BitSet(); + try ( Writer writer = index.writer() ) { - int candidate; - do + for ( int i = 0; i < numberOfNodes - numberOfNodes / 10; i++ ) { - candidate = random.nextInt( max( 1, random.nextInt( numberOfNodes ) ) ); - } - while ( removed.get( candidate ) ); - removed.set( candidate ); + int candidate; + do + { + candidate = random.nextInt( max( 1, random.nextInt( numberOfNodes ) ) ); + } + while ( removed.get( candidate ) ); + removed.set( candidate ); + + writer.remove( key( candidate ) ); - writer.remove( key( candidate ) ); + } } int next = 0; - for ( int i = 0; i < numberOfNodes / 10; i++ ) + try ( Writer writer = index.writer() ) + { + for ( int i = 0; i < numberOfNodes / 10; i++ ) + { + next = removed.nextClearBit( next ); + removed.set( next ); + writer.remove( key( next ) ); + } + } + + // then + try ( RawCursor,IOException> seek = index.seek( key( 0 ), key( numberOfNodes ) ) ) { - next = removed.nextClearBit( next ); - removed.set( next ); - writer.remove( key( next ) ); + assertFalse( seek.next() ); } - } - // then - try ( RawCursor,IOException> seek = index.seek( key(0 ), key( numberOfNodes ) ) ) - { - assertFalse( seek.next() ); + // and finally + index.consistencyCheck(); } } @@ -223,7 +218,7 @@ private void randomlyModifyIndex( GBPTree index, Map data, KEY key = randomKey( data, random ); VALUE value = data.remove( key ); VALUE removedValue = writer.remove( key ); - assertEquals( "For " + key, value, removedValue ); + assertEqualsValue( value, removedValue ); } else { // put @@ -280,4 +275,23 @@ private KEY key( long seed ) { return layout.key( seed ); } + + private void assertEqualsValue( VALUE expected, VALUE actual ) + { + assertTrue( String.format( "expected equal, expected=%s, actual=%s", expected.toString(), actual.toString() ), + layout.compareValue( expected, actual ) == 0 ); + } + + // KEEP even if unused + @SuppressWarnings( "unused" ) + private void printTree() throws IOException + { + index.printTree( false, false, false ); + } + + @SuppressWarnings( "unused" ) + private void printNode( @SuppressWarnings( "SameParameterValue" ) int id ) throws IOException + { + index.printNode( id ); + } } diff --git a/community/index/src/test/java/org/neo4j/index/internal/gbptree/SimpleByteArrayLayout.java b/community/index/src/test/java/org/neo4j/index/internal/gbptree/SimpleByteArrayLayout.java index 409f890e26950..d15d0a71da36b 100644 --- a/community/index/src/test/java/org/neo4j/index/internal/gbptree/SimpleByteArrayLayout.java +++ b/community/index/src/test/java/org/neo4j/index/internal/gbptree/SimpleByteArrayLayout.java @@ -180,10 +180,18 @@ public RawBytes value( long seed ) @Override public long getSeed( RawBytes rawBytes ) { - ByteBuffer buffer = ByteBuffer.allocate(Long.BYTES); - buffer.put( rawBytes.bytes, 0, Long.BYTES ); - buffer.flip(); - return buffer.getLong(); + ByteBuffer buffer = ByteBuffer.allocate( Long.BYTES ); + // Because keySearch is done inside the same shouldRetry block as keyCount() + // We risk reading crap data. This is not a problem because we will retry + // but buffer will throw here if we don't take that into consideration. + byte[] bytes = rawBytes.bytes; + if ( bytes.length >= Long.BYTES ) + { + buffer.put( bytes, 0, Long.BYTES ); + buffer.flip(); + return buffer.getLong(); + } + return 0; } private byte[] fromSeed( long seed )