From b14967219419feebe721a18425d11b9b3db71061 Mon Sep 17 00:00:00 2001 From: Zhen Li Date: Thu, 9 Apr 2020 15:07:13 +0200 Subject: [PATCH 1/2] Improve Record#get(String key) complexity to O(1) The previous impl gives O(n) complexity because of index lookup in keys list. As the API of two Record#get methods are so similar, it is a valid assumption that the performance of two methods shall be the same. This PR introduces a lookup map to use one extra small space to improve `Record#get(String key)` complexity from O(n) to O(1). --- .../neo4j/driver/internal/InternalRecord.java | 31 +++-- .../cursor/AsyncResultCursorImpl.java | 2 +- .../internal/cursor/RxResultCursorImpl.java | 2 +- .../internal/handlers/RunResponseHandler.java | 10 +- .../internal/util/MetadataExtractor.java | 10 +- .../neo4j/driver/internal/util/QueryKeys.java | 112 ++++++++++++++++++ .../reactive/util/ListBasedPullHandler.java | 3 +- .../internal/util/MetadataExtractorTest.java | 17 ++- 8 files changed, 156 insertions(+), 31 deletions(-) create mode 100644 driver/src/main/java/org/neo4j/driver/internal/util/QueryKeys.java diff --git a/driver/src/main/java/org/neo4j/driver/internal/InternalRecord.java b/driver/src/main/java/org/neo4j/driver/internal/InternalRecord.java index a8d24e7e29..e251ce3687 100644 --- a/driver/src/main/java/org/neo4j/driver/internal/InternalRecord.java +++ b/driver/src/main/java/org/neo4j/driver/internal/InternalRecord.java @@ -22,36 +22,43 @@ import java.util.List; import java.util.Map; import java.util.NoSuchElementException; +import java.util.function.Function; -import org.neo4j.driver.internal.types.InternalMapAccessorWithDefaultValue; -import org.neo4j.driver.internal.util.Extract; import org.neo4j.driver.Record; import org.neo4j.driver.Value; import org.neo4j.driver.Values; -import java.util.function.Function; +import org.neo4j.driver.internal.types.InternalMapAccessorWithDefaultValue; +import org.neo4j.driver.internal.util.Extract; +import org.neo4j.driver.internal.util.QueryKeys; import org.neo4j.driver.util.Pair; import static java.lang.String.format; -import static org.neo4j.driver.internal.util.Format.formatPairs; import static org.neo4j.driver.Values.ofObject; import static org.neo4j.driver.Values.ofValue; +import static org.neo4j.driver.internal.util.Format.formatPairs; public class InternalRecord extends InternalMapAccessorWithDefaultValue implements Record { - private final List keys; + private final QueryKeys queryKeys; private final Value[] values; private int hashCode = 0; public InternalRecord( List keys, Value[] values ) { - this.keys = keys; + this.queryKeys = new QueryKeys( keys ); + this.values = values; + } + + public InternalRecord( QueryKeys queryKeys, Value[] values ) + { + this.queryKeys = queryKeys; this.values = values; } @Override public List keys() { - return keys; + return queryKeys.keys(); } @Override @@ -69,7 +76,7 @@ public List> fields() @Override public int index( String key ) { - int result = keys.indexOf( key ); + int result = queryKeys.indexOf( key ); if ( result == -1 ) { throw new NoSuchElementException( "Unknown key: " + key ); @@ -83,13 +90,13 @@ public int index( String key ) @Override public boolean containsKey( String key ) { - return keys.contains( key ); + return queryKeys.contains( key ); } @Override public Value get( String key ) { - int fieldIndex = keys.indexOf( key ); + int fieldIndex = queryKeys.indexOf( key ); if ( fieldIndex == -1 ) { @@ -146,7 +153,7 @@ else if ( other instanceof Record ) { return false; } - if ( ! keys.equals( otherRecord.keys() ) ) + if ( ! queryKeys.equals( otherRecord.keys() ) ) { return false; } @@ -172,7 +179,7 @@ public int hashCode() { if ( hashCode == 0 ) { - hashCode = 31 * keys.hashCode() + Arrays.hashCode( values ); + hashCode = 31 * queryKeys.hashCode() + Arrays.hashCode( values ); } return hashCode; } diff --git a/driver/src/main/java/org/neo4j/driver/internal/cursor/AsyncResultCursorImpl.java b/driver/src/main/java/org/neo4j/driver/internal/cursor/AsyncResultCursorImpl.java index e6f4f7c9e4..63b7cdef7b 100644 --- a/driver/src/main/java/org/neo4j/driver/internal/cursor/AsyncResultCursorImpl.java +++ b/driver/src/main/java/org/neo4j/driver/internal/cursor/AsyncResultCursorImpl.java @@ -45,7 +45,7 @@ public AsyncResultCursorImpl(RunResponseHandler runHandler, PullAllResponseHandl @Override public List keys() { - return runHandler.queryKeys(); + return runHandler.queryKeys().keys(); } @Override diff --git a/driver/src/main/java/org/neo4j/driver/internal/cursor/RxResultCursorImpl.java b/driver/src/main/java/org/neo4j/driver/internal/cursor/RxResultCursorImpl.java index 2ce298d3fc..a2dea23968 100644 --- a/driver/src/main/java/org/neo4j/driver/internal/cursor/RxResultCursorImpl.java +++ b/driver/src/main/java/org/neo4j/driver/internal/cursor/RxResultCursorImpl.java @@ -65,7 +65,7 @@ public RxResultCursorImpl(Throwable runError, RunResponseHandler runHandler, Pul @Override public List keys() { - return runHandler.queryKeys(); + return runHandler.queryKeys().keys(); } @Override diff --git a/driver/src/main/java/org/neo4j/driver/internal/handlers/RunResponseHandler.java b/driver/src/main/java/org/neo4j/driver/internal/handlers/RunResponseHandler.java index 3ad6677571..379409c0a3 100644 --- a/driver/src/main/java/org/neo4j/driver/internal/handlers/RunResponseHandler.java +++ b/driver/src/main/java/org/neo4j/driver/internal/handlers/RunResponseHandler.java @@ -18,15 +18,13 @@ */ package org.neo4j.driver.internal.handlers; -import java.util.List; import java.util.Map; import java.util.concurrent.CompletableFuture; +import org.neo4j.driver.Value; import org.neo4j.driver.internal.spi.ResponseHandler; import org.neo4j.driver.internal.util.MetadataExtractor; -import org.neo4j.driver.Value; - -import static java.util.Collections.emptyList; +import org.neo4j.driver.internal.util.QueryKeys; public class RunResponseHandler implements ResponseHandler { @@ -34,7 +32,7 @@ public class RunResponseHandler implements ResponseHandler private final MetadataExtractor metadataExtractor; private long queryId = MetadataExtractor.ABSENT_QUERY_ID; - private List queryKeys = emptyList(); + private QueryKeys queryKeys = QueryKeys.empty(); private long resultAvailableAfter = -1; public RunResponseHandler( MetadataExtractor metadataExtractor ) @@ -70,7 +68,7 @@ public void onRecord( Value[] fields ) throw new UnsupportedOperationException(); } - public List queryKeys() + public QueryKeys queryKeys() { return queryKeys; } diff --git a/driver/src/main/java/org/neo4j/driver/internal/util/MetadataExtractor.java b/driver/src/main/java/org/neo4j/driver/internal/util/MetadataExtractor.java index 559f8da25c..bcf5920375 100644 --- a/driver/src/main/java/org/neo4j/driver/internal/util/MetadataExtractor.java +++ b/driver/src/main/java/org/neo4j/driver/internal/util/MetadataExtractor.java @@ -18,7 +18,6 @@ */ package org.neo4j.driver.internal.util; -import java.util.ArrayList; import java.util.Collections; import java.util.List; import java.util.Map; @@ -40,11 +39,10 @@ import org.neo4j.driver.summary.Notification; import org.neo4j.driver.summary.Plan; import org.neo4j.driver.summary.ProfiledPlan; +import org.neo4j.driver.summary.QueryType; import org.neo4j.driver.summary.ResultSummary; import org.neo4j.driver.summary.ServerInfo; -import org.neo4j.driver.summary.QueryType; -import static java.util.Collections.emptyList; import static org.neo4j.driver.internal.summary.InternalDatabaseInfo.DEFAULT_DATABASE_INFO; import static org.neo4j.driver.internal.types.InternalTypeSystem.TYPE_SYSTEM; @@ -60,14 +58,14 @@ public MetadataExtractor( String resultAvailableAfterMetadataKey, String resultC this.resultConsumedAfterMetadataKey = resultConsumedAfterMetadataKey; } - public List extractQueryKeys(Map metadata ) + public QueryKeys extractQueryKeys( Map metadata ) { Value keysValue = metadata.get( "fields" ); if ( keysValue != null ) { if ( !keysValue.isEmpty() ) { - List keys = new ArrayList<>( keysValue.size() ); + QueryKeys keys = new QueryKeys( keysValue.size() ); for ( Value value : keysValue.values() ) { keys.add( value.asString() ); @@ -76,7 +74,7 @@ public List extractQueryKeys(Map metadata ) return keys; } } - return emptyList(); + return QueryKeys.empty(); } public long extractQueryId( Map metadata ) diff --git a/driver/src/main/java/org/neo4j/driver/internal/util/QueryKeys.java b/driver/src/main/java/org/neo4j/driver/internal/util/QueryKeys.java new file mode 100644 index 0000000000..ba31c0646c --- /dev/null +++ b/driver/src/main/java/org/neo4j/driver/internal/util/QueryKeys.java @@ -0,0 +1,112 @@ +/* + * Copyright (c) 2002-2020 "Neo4j," + * Neo4j Sweden AB [http://neo4j.com] + * + * This file is part of Neo4j. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.neo4j.driver.internal.util; + +import java.util.ArrayList; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.Objects; + +import static java.util.Collections.emptyList; +import static java.util.Collections.emptyMap; + +public class QueryKeys +{ + private static final QueryKeys EMPTY = new QueryKeys( emptyList(), emptyMap() ); + + private final List keys; + private final Map keyIndex; + + public QueryKeys( int size ) + { + this( new ArrayList<>( size ), new HashMap<>( size ) ); + } + + public QueryKeys( List keys ) + { + this.keys = keys; + Map keyIndex = new HashMap<>( keys.size() ); + int i = 0; + for ( String key : keys ) + { + keyIndex.put( key, i++ ); + } + this.keyIndex = keyIndex; + } + + public QueryKeys( List keys, Map keyIndex ) + { + this.keys = keys; + this.keyIndex = keyIndex; + } + + public void add( String key ) + { + int index = keys.size(); + keys.add( key ); + keyIndex.put( key, index ); + } + + public List keys() + { + return keys; + } + + public Map keyIndex() + { + return keyIndex; + } + + public static QueryKeys empty() + { + return EMPTY; + } + + public int indexOf( String key ) + { + return keyIndex.getOrDefault( key, -1 ); + } + + public boolean contains( String key ) + { + return keyIndex.containsKey( key ); + } + + @Override + public boolean equals( Object o ) + { + if ( this == o ) + { + return true; + } + if ( o == null || getClass() != o.getClass() ) + { + return false; + } + QueryKeys queryKeys = (QueryKeys) o; + return keys.equals( queryKeys.keys ) && keyIndex.equals( queryKeys.keyIndex ); + } + + @Override + public int hashCode() + { + return Objects.hash( keys, keyIndex ); + } +} diff --git a/driver/src/test/java/org/neo4j/driver/internal/reactive/util/ListBasedPullHandler.java b/driver/src/test/java/org/neo4j/driver/internal/reactive/util/ListBasedPullHandler.java index 0f49c41c4a..cb3b1ed697 100644 --- a/driver/src/test/java/org/neo4j/driver/internal/reactive/util/ListBasedPullHandler.java +++ b/driver/src/test/java/org/neo4j/driver/internal/reactive/util/ListBasedPullHandler.java @@ -27,6 +27,7 @@ import org.neo4j.driver.internal.handlers.pulln.BasicPullResponseHandler; import org.neo4j.driver.internal.spi.Connection; import org.neo4j.driver.internal.util.MetadataExtractor; +import org.neo4j.driver.internal.util.QueryKeys; import org.neo4j.driver.internal.value.BooleanValue; import org.neo4j.driver.Record; import org.neo4j.driver.Value; @@ -72,7 +73,7 @@ private ListBasedPullHandler( List list, Throwable error ) if ( list.size() > 1 ) { Record record = list.get( 0 ); - when( super.runResponseHandler.queryKeys() ).thenReturn( record.keys() ); + when( super.runResponseHandler.queryKeys() ).thenReturn( new QueryKeys( record.keys() ) ); } } diff --git a/driver/src/test/java/org/neo4j/driver/internal/util/MetadataExtractorTest.java b/driver/src/test/java/org/neo4j/driver/internal/util/MetadataExtractorTest.java index 0d14c5b169..194c341c95 100644 --- a/driver/src/test/java/org/neo4j/driver/internal/util/MetadataExtractorTest.java +++ b/driver/src/test/java/org/neo4j/driver/internal/util/MetadataExtractorTest.java @@ -20,6 +20,7 @@ import org.junit.jupiter.api.Test; +import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.concurrent.TimeUnit; @@ -77,15 +78,23 @@ class MetadataExtractorTest void shouldExtractQueryKeys() { List keys = asList( "hello", " ", "world", "!" ); - List extractedKeys = extractor.extractQueryKeys( singletonMap( "fields", value( keys ) ) ); - assertEquals( keys, extractedKeys ); + Map keyIndex = new HashMap<>(); + keyIndex.put( "hello", 0 ); + keyIndex.put( " ", 1 ); + keyIndex.put( "world", 2 ); + keyIndex.put( "!", 3 ); + + QueryKeys extracted = extractor.extractQueryKeys( singletonMap( "fields", value( keys ) ) ); + assertEquals( keys, extracted.keys() ); + assertEquals( keyIndex, extracted.keyIndex() ); } @Test void shouldExtractEmptyQueryKeysWhenNoneInMetadata() { - List extractedKeys = extractor.extractQueryKeys( emptyMap() ); - assertEquals( emptyList(), extractedKeys ); + QueryKeys extracted = extractor.extractQueryKeys( emptyMap() ); + assertEquals( emptyList(), extracted.keys() ); + assertEquals( emptyMap(), extracted.keyIndex() ); } @Test From db8a804cd9fc6a65e1657b951a581514aaed81e4 Mon Sep 17 00:00:00 2001 From: Zhen Li Date: Thu, 9 Apr 2020 16:56:12 +0200 Subject: [PATCH 2/2] Fixes red tests --- .../org/neo4j/driver/internal/InternalRecord.java | 2 +- .../internal/handlers/RunResponseHandlerTest.java | 12 ++++++++++-- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/driver/src/main/java/org/neo4j/driver/internal/InternalRecord.java b/driver/src/main/java/org/neo4j/driver/internal/InternalRecord.java index e251ce3687..099dc7fc25 100644 --- a/driver/src/main/java/org/neo4j/driver/internal/InternalRecord.java +++ b/driver/src/main/java/org/neo4j/driver/internal/InternalRecord.java @@ -153,7 +153,7 @@ else if ( other instanceof Record ) { return false; } - if ( ! queryKeys.equals( otherRecord.keys() ) ) + if ( !queryKeys.keys().equals( otherRecord.keys() ) ) { return false; } diff --git a/driver/src/test/java/org/neo4j/driver/internal/handlers/RunResponseHandlerTest.java b/driver/src/test/java/org/neo4j/driver/internal/handlers/RunResponseHandlerTest.java index b2e17a2c7b..d76bfcd780 100644 --- a/driver/src/test/java/org/neo4j/driver/internal/handlers/RunResponseHandlerTest.java +++ b/driver/src/test/java/org/neo4j/driver/internal/handlers/RunResponseHandlerTest.java @@ -20,7 +20,9 @@ import org.junit.jupiter.api.Test; +import java.util.HashMap; import java.util.List; +import java.util.Map; import java.util.concurrent.CompletableFuture; import org.neo4j.driver.internal.messaging.v1.BoltProtocolV1; @@ -83,7 +85,8 @@ void shouldReturnNoKeysWhenFailed() handler.onFailure( new RuntimeException() ); - assertEquals( emptyList(), handler.queryKeys() ); + assertEquals( emptyList(), handler.queryKeys().keys() ); + assertEquals( emptyMap(), handler.queryKeys().keyIndex() ); } @Test @@ -102,9 +105,14 @@ void shouldReturnKeysWhenSucceeded() RunResponseHandler handler = newHandler(); List keys = asList( "key1", "key2", "key3" ); + Map keyIndex = new HashMap<>(); + keyIndex.put( "key1", 0 ); + keyIndex.put( "key2", 1 ); + keyIndex.put( "key3", 2 ); handler.onSuccess( singletonMap( "fields", value( keys ) ) ); - assertEquals( keys, handler.queryKeys() ); + assertEquals( keys, handler.queryKeys().keys() ); + assertEquals( keyIndex, handler.queryKeys().keyIndex() ); } @Test