From 339366a5e4a1a67a1fb4b98b2096d01e670c6700 Mon Sep 17 00:00:00 2001 From: Mattias Persson Date: Wed, 31 Aug 2016 09:19:05 +0200 Subject: [PATCH] Code cleanup, like using constant for -1 --- .../kernel/impl/store/record/Record.java | 5 +++ .../batchimport/CalculateDenseNodesStep.java | 10 +++-- .../CalculateRelationshipsStep.java | 3 +- .../impl/batchimport/RecordIdIterator.java | 12 ++++-- .../batchimport/RelationshipEncoderStep.java | 2 +- .../RelationshipLinkbackProcessor.java | 7 ++-- .../batchimport/RelationshipLinkbackStep.java | 8 ++-- .../RelationshipRecordPreparationStep.java | 4 +- .../RelationshipTypeCheckerStep.java | 41 ++++++------------- .../cache/NodeRelationshipCache.java | 2 +- .../batchimport/cache/idmapping/IdMapper.java | 2 + .../idmapping/string/EncodingIdMapper.java | 26 ++++++------ .../idmapping/string/SameGroupDetector.java | 4 +- .../impl/batchimport/CapturingStep.java | 5 ++- .../string/EncodingIdMapperTest.java | 19 +++++---- 15 files changed, 78 insertions(+), 72 deletions(-) diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/store/record/Record.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/store/record/Record.java index af88238d335c1..2b8ee7709c8fa 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/store/record/Record.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/store/record/Record.java @@ -83,6 +83,11 @@ public int intValue() return intValue; } + public long longValue() + { + return intValue; + } + public boolean is( long value ) { return value == intValue; diff --git a/community/kernel/src/main/java/org/neo4j/unsafe/impl/batchimport/CalculateDenseNodesStep.java b/community/kernel/src/main/java/org/neo4j/unsafe/impl/batchimport/CalculateDenseNodesStep.java index 99479d4d7abe7..dc8538e994b8d 100644 --- a/community/kernel/src/main/java/org/neo4j/unsafe/impl/batchimport/CalculateDenseNodesStep.java +++ b/community/kernel/src/main/java/org/neo4j/unsafe/impl/batchimport/CalculateDenseNodesStep.java @@ -27,6 +27,8 @@ import org.neo4j.unsafe.impl.batchimport.staging.ForkedProcessorStep; import org.neo4j.unsafe.impl.batchimport.staging.StageControl; +import static org.neo4j.unsafe.impl.batchimport.cache.idmapping.IdMapper.ID_NOT_FOUND; + /** * Increments counts for each visited relationship, once for start node and once for end node * (unless for loops). This to be able to determine which nodes are dense before starting to import relationships. @@ -53,8 +55,8 @@ protected void forkedProcess( int id, int processors, Batch batch, Batch protected void done() { long highestId = relationshipStore.getHighId() + numberOfRelationships; - relationshipStore.setHighestPossibleIdInUse( Math.max( highestId, maxSpecific ) ); + relationshipStore.setHighestPossibleIdInUse( highestId ); super.done(); } } diff --git a/community/kernel/src/main/java/org/neo4j/unsafe/impl/batchimport/RecordIdIterator.java b/community/kernel/src/main/java/org/neo4j/unsafe/impl/batchimport/RecordIdIterator.java index 642cc14fcceb3..f84183b6e30eb 100644 --- a/community/kernel/src/main/java/org/neo4j/unsafe/impl/batchimport/RecordIdIterator.java +++ b/community/kernel/src/main/java/org/neo4j/unsafe/impl/batchimport/RecordIdIterator.java @@ -62,7 +62,7 @@ static RecordIdIterator allInReversed( RecordStore return backwards( store.getNumberOfReservedLowIds(), store.getHighId(), config ); } - static class Forwards extends PrimitiveLongBaseIterator implements RecordIdIterator + class Forwards extends PrimitiveLongBaseIterator implements RecordIdIterator { private final long lowIncluded; private final long highExcluded; @@ -73,7 +73,8 @@ static class Forwards extends PrimitiveLongBaseIterator implements RecordIdItera public Forwards( long lowIncluded, long highExcluded, Configuration config ) { - this.lowIncluded = this.nextId = lowIncluded; + this.lowIncluded = lowIncluded; + this.nextId = lowIncluded; this.highExcluded = highExcluded; this.batchSize = config.batchSize(); } @@ -119,7 +120,7 @@ public String toString() } } - static class Backwards extends PrimitiveLongBaseIterator implements RecordIdIterator + class Backwards extends PrimitiveLongBaseIterator implements RecordIdIterator { private final int batchSize; private final long lowIncluded; @@ -134,7 +135,10 @@ public Backwards( long lowIncluded, long highExcluded, Configuration config ) { this.lowIncluded = lowIncluded; this.batchSize = config.batchSize(); - this.highExcluded = this.nextId = this.nextRoofId = this.floorId = highExcluded; + this.highExcluded = highExcluded; + this.nextId = highExcluded; + this.nextRoofId = highExcluded; + this.floorId = highExcluded; } @Override diff --git a/community/kernel/src/main/java/org/neo4j/unsafe/impl/batchimport/RelationshipEncoderStep.java b/community/kernel/src/main/java/org/neo4j/unsafe/impl/batchimport/RelationshipEncoderStep.java index 0b2dd54f7f972..520e91e73443f 100644 --- a/community/kernel/src/main/java/org/neo4j/unsafe/impl/batchimport/RelationshipEncoderStep.java +++ b/community/kernel/src/main/java/org/neo4j/unsafe/impl/batchimport/RelationshipEncoderStep.java @@ -53,7 +53,7 @@ protected void forkedProcess( int id, int processors, Batch batch, Batch InputRelationship batchRelationship = batch.input[i]; long startNodeId = batch.ids[idIndex++]; long endNodeId = batch.ids[idIndex++]; - if ( startNodeId == -1 || endNodeId == -1 ) + if ( startNodeId == ID_NOT_FOUND || endNodeId == ID_NOT_FOUND ) { relationship.setInUse( false ); } diff --git a/community/kernel/src/main/java/org/neo4j/unsafe/impl/batchimport/RelationshipTypeCheckerStep.java b/community/kernel/src/main/java/org/neo4j/unsafe/impl/batchimport/RelationshipTypeCheckerStep.java index 67a0c73205366..b1e4a512cdf0c 100644 --- a/community/kernel/src/main/java/org/neo4j/unsafe/impl/batchimport/RelationshipTypeCheckerStep.java +++ b/community/kernel/src/main/java/org/neo4j/unsafe/impl/batchimport/RelationshipTypeCheckerStep.java @@ -28,6 +28,9 @@ import java.util.List; import java.util.Map; import java.util.concurrent.ConcurrentHashMap; +import java.util.function.Function; +import java.util.stream.Stream; + import org.neo4j.kernel.impl.store.record.RelationshipRecord; import org.neo4j.unsafe.impl.batchimport.input.InputRelationship; import org.neo4j.unsafe.impl.batchimport.staging.BatchSender; @@ -36,12 +39,15 @@ import org.neo4j.unsafe.impl.batchimport.staging.StageControl; import org.neo4j.unsafe.impl.batchimport.store.BatchingTokenRepository.BatchingRelationshipTypeTokenRepository; +import static java.lang.Thread.currentThread; + /** * Counts relationships per type to later be able to provide all types, even sorted in descending order * of number of relationships per type. */ public class RelationshipTypeCheckerStep extends ProcessorStep> { + private static final Function NEW_MUTABLE_LONG = type -> new MutableLong(); private static final Comparator> SORT_BY_COUNT_DESC = (e1,e2) -> Long.compare( e2.getValue().longValue(), e1.getValue().longValue() ); private static final Comparator> SORT_BY_ID_DESC = @@ -60,22 +66,10 @@ public RelationshipTypeCheckerStep( StageControl control, Configuration config, @Override protected void process( Batch batch, BatchSender sender ) throws Throwable { - Map types = typeCheckers.get( Thread.currentThread() ); - if ( types == null ) - { - typeCheckers.put( Thread.currentThread(), types = new HashMap<>() ); - } - - for ( InputRelationship relationship : batch.input ) - { - Object type = relationship.typeAsObject(); - MutableLong count = types.get( type ); - if ( count == null ) - { - types.put( type, count = new MutableLong() ); - } - count.increment(); - } + Map typeMap = typeCheckers.computeIfAbsent( currentThread(), ( t ) -> new HashMap<>() ); + Stream.of( batch.input ) + .map( InputRelationship::typeAsObject ) + .forEach( type -> typeMap.computeIfAbsent( type, NEW_MUTABLE_LONG ).increment() ); sender.send( batch ); } @@ -84,18 +78,9 @@ protected void process( Batch batch, Batch protected void done() { Map mergedTypes = new HashMap<>(); - for ( Map localTypes : typeCheckers.values() ) - { - for ( Map.Entry localType : localTypes.entrySet() ) - { - MutableLong count = mergedTypes.get( localType.getKey() ); - if ( count == null ) - { - mergedTypes.put( localType.getKey(), count = new MutableLong() ); - } - count.add( localType.getValue().longValue() ); - } - } + typeCheckers.forEach( (thread,localTypes) -> + localTypes.forEach( (type,localCount) -> + mergedTypes.computeIfAbsent( type, t -> new MutableLong() ).add( localCount.longValue() ) ) ); sortedTypes = mergedTypes.entrySet().toArray( new Map.Entry[mergedTypes.size()] ); if ( sortedTypes.length > 0 ) diff --git a/community/kernel/src/main/java/org/neo4j/unsafe/impl/batchimport/cache/NodeRelationshipCache.java b/community/kernel/src/main/java/org/neo4j/unsafe/impl/batchimport/cache/NodeRelationshipCache.java index 24997557f831d..a038f72209e88 100644 --- a/community/kernel/src/main/java/org/neo4j/unsafe/impl/batchimport/cache/NodeRelationshipCache.java +++ b/community/kernel/src/main/java/org/neo4j/unsafe/impl/batchimport/cache/NodeRelationshipCache.java @@ -412,7 +412,7 @@ private static class RelGroupCache implements AutoCloseable, MemoryStatsVisitor. { this.base = base; assert chunkSize > 0; - this.array = arrayFactory.newDynamicByteArray( 1_000_000, + this.array = arrayFactory.newDynamicByteArray( chunkSize, minusOneBytes( ID_SIZE/*next*/ + (ID_AND_COUNT_SIZE) * Direction.values().length ) ); this.nextFreeId = new AtomicLong( base ); } diff --git a/community/kernel/src/main/java/org/neo4j/unsafe/impl/batchimport/cache/idmapping/IdMapper.java b/community/kernel/src/main/java/org/neo4j/unsafe/impl/batchimport/cache/idmapping/IdMapper.java index 7d741ec803b05..75bd0c533ee85 100644 --- a/community/kernel/src/main/java/org/neo4j/unsafe/impl/batchimport/cache/idmapping/IdMapper.java +++ b/community/kernel/src/main/java/org/neo4j/unsafe/impl/batchimport/cache/idmapping/IdMapper.java @@ -33,6 +33,8 @@ */ public interface IdMapper extends MemoryStatsVisitor.Visitable { + long ID_NOT_FOUND = -1; + /** * Maps an {@code inputId} to an actual node id. * @param inputId an id of an unknown type, coming from input. diff --git a/community/kernel/src/main/java/org/neo4j/unsafe/impl/batchimport/cache/idmapping/string/EncodingIdMapper.java b/community/kernel/src/main/java/org/neo4j/unsafe/impl/batchimport/cache/idmapping/string/EncodingIdMapper.java index 8e2ceb588b483..bfeb25a595522 100644 --- a/community/kernel/src/main/java/org/neo4j/unsafe/impl/batchimport/cache/idmapping/string/EncodingIdMapper.java +++ b/community/kernel/src/main/java/org/neo4j/unsafe/impl/batchimport/cache/idmapping/string/EncodingIdMapper.java @@ -168,7 +168,7 @@ public EncodingIdMapper( NumberArrayFactory cacheFactory, Encoder encoder, Facto this.encoder = encoder; this.radixFactory = radixFactory; this.radix = radixFactory.newInstance(); - this.collisionNodeIdCache = cacheFactory.newDynamicLongArray( chunkSize, -1 ); + this.collisionNodeIdCache = cacheFactory.newDynamicLongArray( chunkSize, ID_NOT_FOUND ); } /** @@ -314,7 +314,7 @@ private long binarySearch( Object inputId, int groupId ) } long returnVal = binarySearch( x, inputId, low, high, groupId ); - if ( returnVal == -1 ) + if ( returnVal == ID_NOT_FOUND ) { low = 0; high = highestSetIndex; @@ -368,7 +368,7 @@ private int detectAndMarkCollisions( ProgressListener progress ) { long dataIndexA = trackerCache.get( i ); long dataIndexB = trackerCache.get( i+1 ); - if ( dataIndexA == -1 || dataIndexB == -1 ) + if ( dataIndexA == ID_NOT_FOUND || dataIndexB == ID_NOT_FOUND ) { sameGroupDetector.reset(); continue; @@ -399,7 +399,7 @@ dataIndexA, groupOf( dataIndexA ).id(), trackerCache.swap( i, i+1, 1 ); } - if ( collision != -1 ) + if ( collision != ID_NOT_FOUND ) { if ( markAsCollision( collision ) ) { @@ -452,7 +452,7 @@ private void buildCollisionInfo( InputIterator ids, int numberOfCollisio Radix radix = radixFactory.newInstance(); List sourceDescriptions = new ArrayList<>(); String lastSourceDescription = null; - collisionSourceDataCache = cacheFactory.newLongArray( numberOfCollisions, -1 ); + collisionSourceDataCache = cacheFactory.newLongArray( numberOfCollisions, ID_NOT_FOUND ); collisionTrackerCache = trackerFactory.create( cacheFactory, numberOfCollisions ); for ( long i = 0; ids.hasNext(); ) { @@ -654,9 +654,9 @@ private long binarySearch( long x, Object inputId, long low, long high, int grou { long mid = low + (high - low)/2;//(low + high) / 2; long dataIndex = trackerCache.get( mid ); - if ( dataIndex == -1 ) + if ( dataIndex == ID_NOT_FOUND ) { - return -1; + return ID_NOT_FOUND; } long midValue = dataCache.get( dataIndex ); switch ( unsignedDifference( clearCollision( midValue ), x ) ) @@ -673,7 +673,7 @@ private long binarySearch( long x, Object inputId, long low, long high, int grou return findFromEIdRange( mid, midValue, inputId, x, groupId ); } // This is the only value here, let's do a simple comparison with correct group id and return - return groupOf( dataIndex ).id() == groupId ? dataIndex : -1; + return groupOf( dataIndex ).id() == groupId ? dataIndex : ID_NOT_FOUND; case LT: low = mid + 1; break; @@ -682,7 +682,7 @@ private long binarySearch( long x, Object inputId, long low, long high, int grou break; } } - return -1; + return ID_NOT_FOUND; } private long dataValue( long index ) @@ -710,7 +710,7 @@ private long findIndex( LongArray array, long value ) break; } } - return -1; + return ID_NOT_FOUND; } private long findFromEIdRange( long index, long val, Object inputId, long x, int groupId ) @@ -734,7 +734,7 @@ private long findFromEIdRange( long index, long val, Object inputId, long x, int private long findFromEIdRange( long fromIndex, long toIndex, int groupId, Object inputId ) { - long lowestFound = -1; // lowest data index means "first put" + long lowestFound = ID_NOT_FOUND; // lowest data index means "first put" for ( long index = fromIndex; index <= toIndex; index++ ) { long dataIndex = trackerCache.get( index ); @@ -750,7 +750,7 @@ private long findFromEIdRange( long fromIndex, long toIndex, int groupId, Object if ( inputId.equals( value ) ) { // :) - lowestFound = lowestFound == -1 ? dataIndex : min( lowestFound, dataIndex ); + lowestFound = lowestFound == ID_NOT_FOUND ? dataIndex : min( lowestFound, dataIndex ); // continue checking so that we can find the lowest one. It's not up to us here to // consider multiple equal ids in this group an error or not. That should have been // decided in #prepare. @@ -774,7 +774,7 @@ static boolean compareDataCache( LongArray dataCache, IntArray tracker, int a, i { int indexA = tracker.get( a ); int indexB = tracker.get( b ); - if ( indexA == -1 || indexB == -1 ) + if ( indexA == ID_NOT_FOUND || indexB == ID_NOT_FOUND ) { return false; } diff --git a/community/kernel/src/main/java/org/neo4j/unsafe/impl/batchimport/cache/idmapping/string/SameGroupDetector.java b/community/kernel/src/main/java/org/neo4j/unsafe/impl/batchimport/cache/idmapping/string/SameGroupDetector.java index 30d21339b299a..5b4a57a7ed9fa 100644 --- a/community/kernel/src/main/java/org/neo4j/unsafe/impl/batchimport/cache/idmapping/string/SameGroupDetector.java +++ b/community/kernel/src/main/java/org/neo4j/unsafe/impl/batchimport/cache/idmapping/string/SameGroupDetector.java @@ -21,6 +21,8 @@ import java.util.Arrays; +import static org.neo4j.unsafe.impl.batchimport.cache.idmapping.IdMapper.ID_NOT_FOUND; + /** * Used by {@link EncodingIdMapper} to help detect collisions of encoded values within the same group. * Same values for different groups are not considered collisions. @@ -47,7 +49,7 @@ long collisionWithinSameGroup( long dataIndexA, int groupIdA, long dataIndexB, i add( dataIndexA, groupIdA ); } - long collision = -1; + long collision = ID_NOT_FOUND; for ( int i = 0; i < cursor; i++ ) { long dataIndexAtCursor = seen[i++]; diff --git a/community/kernel/src/test/java/org/neo4j/unsafe/impl/batchimport/CapturingStep.java b/community/kernel/src/test/java/org/neo4j/unsafe/impl/batchimport/CapturingStep.java index 6543dcd7f4a84..2d1b40d6e770d 100644 --- a/community/kernel/src/test/java/org/neo4j/unsafe/impl/batchimport/CapturingStep.java +++ b/community/kernel/src/test/java/org/neo4j/unsafe/impl/batchimport/CapturingStep.java @@ -20,6 +20,7 @@ package org.neo4j.unsafe.impl.batchimport; import java.util.ArrayList; +import java.util.Collections; import java.util.List; import org.neo4j.unsafe.impl.batchimport.staging.BatchSender; @@ -30,7 +31,7 @@ public class CapturingStep extends ProcessorStep { - private final List receivedBatches = new ArrayList<>(); + private final List receivedBatches = Collections.synchronizedList( new ArrayList<>() ); public CapturingStep( StageControl control, String name, Configuration config, StatsProvider... additionalStatsProvider ) @@ -38,7 +39,7 @@ public CapturingStep( StageControl control, String name, Configuration config, super( control, name, config, 1, additionalStatsProvider ); } - public Iterable receivedBatches() + public List receivedBatches() { return receivedBatches; } diff --git a/community/kernel/src/test/java/org/neo4j/unsafe/impl/batchimport/cache/idmapping/string/EncodingIdMapperTest.java b/community/kernel/src/test/java/org/neo4j/unsafe/impl/batchimport/cache/idmapping/string/EncodingIdMapperTest.java index 979782014f73a..e9a0b10d37609 100644 --- a/community/kernel/src/test/java/org/neo4j/unsafe/impl/batchimport/cache/idmapping/string/EncodingIdMapperTest.java +++ b/community/kernel/src/test/java/org/neo4j/unsafe/impl/batchimport/cache/idmapping/string/EncodingIdMapperTest.java @@ -68,6 +68,7 @@ import static org.mockito.Mockito.when; import static org.neo4j.helpers.progress.ProgressListener.NONE; +import static org.neo4j.unsafe.impl.batchimport.cache.idmapping.IdMapper.ID_NOT_FOUND; import static org.neo4j.unsafe.impl.batchimport.cache.idmapping.string.EncodingIdMapper.NO_MONITOR; import static org.neo4j.unsafe.impl.batchimport.input.Collectors.badCollector; import static org.neo4j.unsafe.impl.batchimport.input.Group.GLOBAL; @@ -138,7 +139,7 @@ public boolean supportsMultiplePasses() for ( Object id : ids ) { // the UUIDs here will be generated in the same sequence as above because we reset the random - if ( idMapper.get( id, GLOBAL ) == -1 ) + if ( idMapper.get( id, GLOBAL ) == ID_NOT_FOUND ) { fail( "Couldn't find " + id + " even though I added it just previously" ); } @@ -156,7 +157,7 @@ public void shouldReturnExpectedValueForNotFound() throws Exception long id = idMapper.get( "123", GLOBAL ); // THEN - assertEquals( -1L, id ); + assertEquals( ID_NOT_FOUND, id ); } @Test @@ -171,7 +172,7 @@ public void shouldReportyProgressForSortAndDetect() throws Exception long id = idMapper.get( "123", GLOBAL ); // THEN - assertEquals( -1L, id ); + assertEquals( ID_NOT_FOUND, id ); verify( progress, times( 3 ) ).started( anyString() ); verify( progress, times( 3 ) ).done(); } @@ -408,15 +409,15 @@ public void shouldOnlyFindInputIdsInSpecificGroup() throws Exception // WHEN/THEN assertEquals( 0L, mapper.get( "8", firstGroup ) ); - assertEquals( -1L, mapper.get( "8", secondGroup ) ); - assertEquals( -1L, mapper.get( "8", thirdGroup ) ); + assertEquals( ID_NOT_FOUND, mapper.get( "8", secondGroup ) ); + assertEquals( ID_NOT_FOUND, mapper.get( "8", thirdGroup ) ); - assertEquals( -1L, mapper.get( "9", firstGroup ) ); + assertEquals( ID_NOT_FOUND, mapper.get( "9", firstGroup ) ); assertEquals( 1L, mapper.get( "9", secondGroup ) ); - assertEquals( -1L, mapper.get( "9", thirdGroup ) ); + assertEquals( ID_NOT_FOUND, mapper.get( "9", thirdGroup ) ); - assertEquals( -1L, mapper.get( "10", firstGroup ) ); - assertEquals( -1L, mapper.get( "10", secondGroup ) ); + assertEquals( ID_NOT_FOUND, mapper.get( "10", firstGroup ) ); + assertEquals( ID_NOT_FOUND, mapper.get( "10", secondGroup ) ); assertEquals( 2L, mapper.get( "10", thirdGroup ) ); }