Neat find Pieter.  With regards to the update to ImmutablePath, I think
that defensive copying of inbound collections is generally a good idea but
if we can target specific areas where we can reap big gains from not
creating new collections it may be worth it to relax that constraint,
especially if we're only talking about internal APIs.  If we do relax that
constraint though, we'll need to careful during code review and unit
testing because those bugs can be difficult to track down.  The other nice
thing that the defensive copy gets us, particularly in the case of the
ImmutablePath, is that it isolates ImmutablePath from whatever the subclass
of set was that the caller passed in.  I think that's what is causing the
serialization test failure in this case since the caller passed in an
unmodifiable set.

--Ted

On Fri, Aug 5, 2016, 2:31 PM Marko Rodriguez <okramma...@gmail.com> wrote:

> Hello,
>
> This is cool. Check out also ImmutablePath.extend(labels) as that is
> ultimately what Traverser.addLabels() calls. We have a lot of set copying
> and I don’t know if its needed (as you seem to be demonstrating). What I
> don’t like about your solution is the explicit reference to the
> B_L_P…Traverser in AbstractStep. See if you can work your solution without
> it.
>
> Good luck,
> Marko.
>
> http://markorodriguez.com
>
>
>
> > On Aug 5, 2016, at 12:44 PM, pieter-gmail <pieter.mar...@gmail.com>
> wrote:
> >
> > Sorry forgot to add a rather important part.
> >
> > I changed ImmutablePath's constructor to
> >
> >    private ImmutablePath(final ImmutablePathImpl previousPath, final
> > Object currentObject, final Set<String> currentLabels) {
> >        this.previousPath = previousPath;
> >        this.currentObject = currentObject;
> >        this.currentLabels = currentLabels;
> > //        this.currentLabels.addAll(currentLabels);
> >    }
> >
> > Setting the collection directly as oppose to `addAll`
> >
> > Thanks
> > Pieter
> >
> >
> > On 05/08/2016 20:40, pieter-gmail wrote:
> >> Hi,
> >>
> >> I have been optimizing Sqlg of late and eventually arrived at TinkerPop
> >> code.
> >>
> >> The gremlin in particular that I am interested is path queries.
> >>
> >> Here is the test that I am running in jmh.
> >>
> >>        //@Setup
> >>        Vertex a = graph.addVertex(T.label, "A", "name", "a1");
> >>        for (int i = 1; i < 1_000_001; i++) {
> >>            Vertex b = graph.addVertex(T.label, "B", "name", "name_" +
> i);
> >>            a.addEdge("outB", b);
> >>            for (int j = 0; j < 1; j++) {
> >>                Vertex c = graph.addVertex(T.label, "C", "name", "name_"
> >> + i + " " + j);
> >>                b.addEdge("outC", c);
> >>            }
> >>        }
> >>
> >> And the query being benchmarked is
> >>
> >>        GraphTraversal<Vertex, Path> traversal =
> >> g.V(a).as("a").out().as("b").out().as("c").path();
> >>        while (traversal.hasNext()) {
> >>            Path path = traversal.next();
> >>        }
> >>
> >> Before the optimization, (as things are now)
> >>
> >> Benchmark                                 Mode Cnt  Score Error
>  Units
> >> GremlinPathBenchmark.g_path  avgt  100  1.086 ± 0.020   s/op
> >>
> >> The optimization I did is in AbstractStep.prepareTraversalForNextStep,
> >> to not call addLabels() for path gremlins as the labels are known by the
> >> step and do not change again so there is not need to keep adding them.
> >>
> >>    private final Traverser.Admin<E> prepareTraversalForNextStep(final
> >> Traverser.Admin<E> traverser) {
> >>        if (!this.traverserStepIdAndLabelsSetByChild) {
> >>            traverser.setStepId(this.nextStep.getId());
> >>            if (traverser instanceof B_LP_O_P_S_SE_SL_Traverser) {
> >>            } else {
> >>                traverser.addLabels(this.labels);
> >>            }
> >>        }
> >>        return traverser;
> >>    }
> >>
> >> After optimization,
> >>
> >> Benchmark                                 Mode Cnt  Score Error
>  Units
> >> GremlinPathBenchmark.g_path  avgt  100  0.680 ± 0.004   s/op
> >>
> >> 1.086 vs 0.689 seconds for the traversal.
> >>
> >> I ran the Structured and Process test suites. 2 tests are failing with
> >> this optimization.
> >>
> >> InjectTest.g_VX1X_out_name_injectXdanielX_asXaX_mapXlengthX_path fails
> with
> >>
> >> "java.lang.IllegalArgumentException: The step with label a does not
> exist"
> >>
> >> and
> >>
> >> SerializationTest.shouldSerializePathAsDetached fails with
> >>
> >> "Caused by: java.lang.IllegalArgumentException: Class is not
> registered:
> >> java.util.Collections$UnmodifiableSet"
> >>
> >> Before investigating the failures is this optimization worth pursuing?
> >>
> >> Thanks
> >> Pieter
> >>
> >
>
>

Reply via email to