TINKERPOP-1642 Improved performance of addV() and chained mutating statements
Added more tests for Parameters. Performance improved by about 2.5x given the benchmarks. Also long chained mutating traversals are now inserting roughly on par with single insert traversals (prior to this change they were about 25% slower). Project: http://git-wip-us.apache.org/repos/asf/tinkerpop/repo Commit: http://git-wip-us.apache.org/repos/asf/tinkerpop/commit/da02d965 Tree: http://git-wip-us.apache.org/repos/asf/tinkerpop/tree/da02d965 Diff: http://git-wip-us.apache.org/repos/asf/tinkerpop/diff/da02d965 Branch: refs/heads/tp32 Commit: da02d96594139d3187c93ada35fd670d688ba81d Parents: 0fd2666 Author: Stephen Mallette <sp...@genoprime.com> Authored: Thu Mar 2 10:44:58 2017 -0500 Committer: Stephen Mallette <sp...@genoprime.com> Committed: Wed Mar 29 11:20:43 2017 -0400 ---------------------------------------------------------------------- .../traversal/dsl/graph/GraphTraversal.java | 9 +- .../process/traversal/step/util/Parameters.java | 33 ++++- .../traversal/step/util/ParametersTest.java | 119 ++++++++++++++++++- 3 files changed, 146 insertions(+), 15 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/da02d965/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/dsl/graph/GraphTraversal.java ---------------------------------------------------------------------- diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/dsl/graph/GraphTraversal.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/dsl/graph/GraphTraversal.java index bde1dea..afa23d7 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/dsl/graph/GraphTraversal.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/dsl/graph/GraphTraversal.java @@ -1010,7 +1010,6 @@ public interface GraphTraversal<S, E> extends Traversal<S, E> { for (int i = 0; i < propertyKeyValues.length; i = i + 2) { this.property(propertyKeyValues[i], propertyKeyValues[i + 1]); } - //((AddVertexStep) this.asAdmin().getEndStep()).addPropertyMutations(propertyKeyValues); return (GraphTraversal<S, Vertex>) this; } @@ -2055,11 +2054,13 @@ public interface GraphTraversal<S, E> extends Traversal<S, E> { this.asAdmin().getBytecode().addStep(Symbols.property, key, value, keyValues); else this.asAdmin().getBytecode().addStep(Symbols.property, cardinality, key, value, keyValues); + // if it can be detected that this call to property() is related to an addV/E() then we can attempt to fold // the properties into that step to gain an optimization for those graphs that support such capabilities. - if ((this.asAdmin().getEndStep() instanceof AddVertexStep || this.asAdmin().getEndStep() instanceof AddEdgeStep - || this.asAdmin().getEndStep() instanceof AddVertexStartStep) && keyValues.length == 0 && null == cardinality) { - ((Mutating) this.asAdmin().getEndStep()).addPropertyMutations(key, value); + final Step endStep = this.asAdmin().getEndStep(); + if ((endStep instanceof AddVertexStep || endStep instanceof AddEdgeStep || endStep instanceof AddVertexStartStep) && + keyValues.length == 0 && null == cardinality) { + ((Mutating) endStep).addPropertyMutations(key, value); } else { this.asAdmin().addStep(new AddPropertyStep(this.asAdmin(), cardinality, key, value)); ((AddPropertyStep) this.asAdmin().getEndStep()).addPropertyMutations(keyValues); http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/da02d965/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/util/Parameters.java ---------------------------------------------------------------------- diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/util/Parameters.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/util/Parameters.java index 3584c87..67cb2f9 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/util/Parameters.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/util/Parameters.java @@ -49,6 +49,11 @@ public final class Parameters implements Cloneable, Serializable { private Map<Object, List<Object>> parameters = new HashMap<>(); /** + * Determines if there are traversals present in the parameters {@code Map}. + */ + private boolean traversalsPresent = false; + + /** * Checks for existence of key in parameter set. * * @param key the key to check @@ -138,6 +143,11 @@ public final class Parameters implements Cloneable, Serializable { Parameters.legalPropertyKeyValueArray(keyValues); for (int i = 0; i < keyValues.length; i = i + 2) { if (keyValues[i + 1] != null) { + // track whether or not traversals are present so that elsewhere in Parameters there is no need + // to iterate all values to not find any. + if (!traversalsPresent && keyValues[i + 1] instanceof Traversal.Admin) + traversalsPresent = true; + List<Object> values = this.parameters.get(keyValues[i]); if (null == values) { values = new ArrayList<>(); @@ -150,18 +160,31 @@ public final class Parameters implements Cloneable, Serializable { } } + /** + * Calls {@link TraversalParent#integrateChild(Traversal.Admin)} on any traversal objects in the parameter list. + * This method requires that {@link #set(Object...)} is called prior to this method as the it will switch the + * {@code traversalsPresent} flag field if a {@link Traversal.Admin} object is present and allow this method to + * do its work. + */ public void integrateTraversals(final TraversalParent step) { - for (final List<Object> values : this.parameters.values()) { - for (final Object object : values) { - if (object instanceof Traversal.Admin) { - step.integrateChild((Traversal.Admin) object); + if (traversalsPresent) { + for (final List<Object> values : this.parameters.values()) { + for (final Object object : values) { + if (object instanceof Traversal.Admin) { + step.integrateChild((Traversal.Admin) object); + } } } } } + /** + * Gets all the {@link Traversal.Admin} objects in the map of parameters. + */ public <S, E> List<Traversal.Admin<S, E>> getTraversals() { - List<Traversal.Admin<S, E>> result = new ArrayList<>(); + if (!traversalsPresent) return Collections.emptyList(); + + final List<Traversal.Admin<S, E>> result = new ArrayList<>(); for (final List<Object> list : this.parameters.values()) { for (final Object object : list) { if (object instanceof Traversal.Admin) { http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/da02d965/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/util/ParametersTest.java ---------------------------------------------------------------------- diff --git a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/util/ParametersTest.java b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/util/ParametersTest.java index 7490dea..27b9293 100644 --- a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/util/ParametersTest.java +++ b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/util/ParametersTest.java @@ -18,8 +18,12 @@ */ package org.apache.tinkerpop.gremlin.process.traversal.step.util; +import org.apache.tinkerpop.gremlin.process.traversal.Traverser; +import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__; +import org.apache.tinkerpop.gremlin.process.traversal.step.TraversalParent; import org.junit.Test; +import java.util.Arrays; import java.util.Collections; import java.util.List; import java.util.Map; @@ -28,12 +32,66 @@ import static org.hamcrest.CoreMatchers.is; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.contains; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotEquals; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; /** * @author Stephen Mallette (http://stephen.genoprime.com) */ public class ParametersTest { @Test + public void shouldGetKeyValuesEmpty() { + final Parameters parameters = new Parameters(); + assertThat(Arrays.equals(parameters.getKeyValues(mock(Traverser.Admin.class)), new Object[0]), is(true)); + } + + @Test + public void shouldGetKeyValues() { + final Parameters parameters = new Parameters(); + parameters.set("a", "axe", "b", "bat", "c", "cat"); + + final Object[] params = parameters.getKeyValues(mock(Traverser.Admin.class)); + assertEquals(6, params.length); + assertThat(Arrays.equals(new Object[] {"a", "axe", "b", "bat", "c", "cat"}, params), is(true)); + } + + @Test + public void shouldGetKeyValuesIgnoringSomeKeys() { + final Parameters parameters = new Parameters(); + parameters.set("a", "axe", "b", "bat", "c", "cat"); + + final Object[] params = parameters.getKeyValues(mock(Traverser.Admin.class), "b"); + assertEquals(4, params.length); + assertThat(Arrays.equals(new Object[] {"a", "axe", "c", "cat"}, params), is(true)); + } + + @Test + public void shouldGetUsingTraverserOverload() { + final Parameters parameters = new Parameters(); + parameters.set("a", "axe", "b", "bat", "c", "cat"); + + assertEquals(Collections.singletonList("axe"), parameters.get(mock(Traverser.Admin.class), "a", () -> "x")); + assertEquals(Collections.singletonList("bat"), parameters.get(mock(Traverser.Admin.class), "b", () -> "x")); + assertEquals(Collections.singletonList("cat"), parameters.get(mock(Traverser.Admin.class), "c", () -> "x")); + assertEquals(Collections.singletonList("zebra"), parameters.get(mock(Traverser.Admin.class), "z", () -> "zebra")); + } + + @Test + public void shouldGetMultipleUsingTraverserOverload() { + final Parameters parameters = new Parameters(); + parameters.set("a", "axe", "a", "ant", "b", "bat", "b", "ball", "c", "cat"); + + final Map<Object,List<Object>> params = parameters.getRaw(); + assertEquals(3, params.size()); + assertEquals(Arrays.asList("axe", "ant"), parameters.get(mock(Traverser.Admin.class), "a", () -> "x")); + assertEquals(Arrays.asList("bat", "ball"), parameters.get(mock(Traverser.Admin.class), "b", () -> "x")); + assertEquals(Collections.singletonList("cat"), parameters.get(mock(Traverser.Admin.class), "c", () -> "x")); + assertEquals(Collections.singletonList("zebra"), parameters.get(mock(Traverser.Admin.class), "z", () -> "zebra")); + } + + @Test public void shouldGetRaw() { final Parameters parameters = new Parameters(); parameters.set("a", "axe", "b", "bat", "c", "cat"); @@ -143,11 +201,60 @@ public class ParametersTest { final Parameters parameters = new Parameters(); parameters.set("a", "axe", "b", "bat", "c", "cat"); - final Map<Object,List<Object>> params = parameters.getRaw(); - assertEquals(3, params.size()); - assertEquals("axe", params.get("a").get(0)); - assertEquals("bat", params.get("b").get(0)); - assertEquals("cat", params.get("c").get(0)); - assertEquals("zebra", parameters.get("z", () -> "zebra").get(0)); + assertEquals(Collections.singletonList("axe"), parameters.get("a", () -> "x")); + assertEquals(Collections.singletonList("bat"), parameters.get("b", () -> "x")); + assertEquals(Collections.singletonList("cat"), parameters.get("c", () -> "x")); + assertEquals(Collections.singletonList("zebra"), parameters.get("z", () -> "zebra")); + } + + @Test + public void shouldClone() { + final Parameters parameters = new Parameters(); + parameters.set("a", "axe", "a", "ant", "b", "bat", "b", "ball", "c", "cat"); + + final Parameters parametersClone = parameters.clone(); + + assertEquals(parameters.getRaw(), parametersClone.getRaw()); + } + + @Test + public void shouldBeDifferent() { + final Parameters parameters = new Parameters(); + parameters.set("a", "axe", "a", "ant", "b", "bat", "b", "ball", "c", "cat"); + + final Parameters parametersDifferent = new Parameters(); + parametersDifferent.set("a", "ant", "a", "axe", "b", "bat", "b", "ball", "c", "cat"); + + assertNotEquals(parameters.getRaw(), parametersDifferent.getRaw()); + } + + @Test + public void shouldGetNoTraversals() { + final Parameters parameters = new Parameters(); + parameters.set("a", "axe", "a", "ant", "b", "bat", "b", "ball", "c", "cat"); + assertEquals(Collections.emptyList(), parameters.getTraversals()); + } + + @Test + public void shouldGetTraversals() { + final Parameters parameters = new Parameters(); + parameters.set("a", "axe", "a", "ant", "b", "bat", "b", "ball", "c", "cat", "t", __.outE("knows")); + assertEquals(Collections.singletonList(__.outE("knows")), parameters.getTraversals()); + } + + @Test + public void shouldIntegrateTraversals() { + final Parameters parameters = new Parameters(); + parameters.set("a", "axe", "a", "ant", "b", "bat", "b", "ball", "c", "cat", "t", __.outE("knows")); + + final TraversalParent mock = mock(TraversalParent.class); + + // the mock can return nothing of consequence as the return isn't used in this case. we just need to + // validate that the method gets called as a result of calls to set/integrateTraversals() + when(mock.integrateChild(__.outE("knows").asAdmin())).thenReturn(null); + + parameters.integrateTraversals(mock); + + verify(mock).integrateChild(__.outE("knows").asAdmin()); } }
