Hi, So I've benchmarked both GroupStep and GroupSideEffectStep (new vs. old) in both OLTP and OLAP and we are solid with the new GroupXXXStep work.
Realize, again that the new GroupXXXStep won't (if the user has a reducer on the end) run out of memory where the old GroupXXXStep can. Finally, GroupSideEffectStep OLAP is faster in the old way. I can make the new way as fast, but you run the risk of OME….. So…………. Pretty sweet, eh?, Marko. -------------------------------------------- GroupStep OLTP: Big Values -- by(songType) group: 410.383763 groupV3d0: 462.3964605 group: 427.3465912 groupV3d0: 463.2559346 Big Keys -- by(name) group: 447.6033716999999 groupV3d0: 457.03214479999997 group: 455.27886709999996 groupV3d0: 457.823757 GroupStep OLAP: Big Values -- by(songType) group: 142.5725566 groupV3d0: 411.53331769999994 group: 75.4783576 groupV3d0: 294.2621524 Big Keys -- by(name) group: 39.9930389 groupV3d0: 90.6787334 group: 36.8134341 groupV3d0: 85.4036581 GroupSideEffectStep OLTP: Big Values -- by(songType) group: 467.07767709999996 groupV3d0: 563.9450385 group: 454.1553032 groupV3d0: 547.5434134 Big Keys -- by(name) group: 464.7922198 groupV3d0: 545.6747167 group: 479.90088689999993 groupV3d0: 522.9470415 GroupSideEffectStep OLAP: Big Values -- by(songType) group: 84.4585517 groupV3d0: 48.429067599999996 group: 56.1160803 groupV3d0: 45.3152932 Big Keys -- by(name) group: 75.810446 groupV3d0: 44.7250867 group: 40.175867999999994 groupV3d0: 45.113792000000004 http://markorodriguez.com On Oct 8, 2015, at 1:23 PM, Marko Rodriguez <[email protected]> wrote: > Hello purple people eaters, > > I did some benchmarking of GroupStep vs. GroupStepV3d0. The general themes: > > 1. GroupStep is slightly faster than GroupV3d0Step (GOOD). > 2. GroupStep uses less memory if there is a reducer at the end of the > valueTraversal (e.g. max(), min(), count(), sum(), etc.). (GOOD) > - This is because every "X" inserts its processes the > traversers thus far and reduces them. > > ------------------ > > The first query produces 2 keys but lots of values for each key. (actually 3 > keys -- we have a data bug in the GratefulDead graph :|). > > > g.V().both("followedBy").both("followedBy").group().by("songType").by(count()) > > g.V().both("followedBy").both("followedBy").groupV3d0().by("songType").by().by(count(local)) > > Both produce the answer: > {cover=368579, =160968, original=771317} > > **** The empty key! is a bunch of songs with a blank string for songType. > Crap. Data bug. **** > > Runtime group (ms): 420.1875169 > Runtime groupV3d0 (ms): 461.91872409999996 > > Now, without the reducer -- thus, just a big old fold() at the end. > > Runtime group (ms): 422.8069872 > Runtime groupV3d0 (ms): 458.80180110000003 > > ------------------ > > The second query produces 338 keys with each key having less values than the > first. > > > g.V().both("followedBy").both("followedBy").group().by("name").by(count()) > > g.V().both("followedBy").both("followedBy").groupV3d0().by("name").by().by(count(local)) > > Runtime group (ms): 428.6962979 > Runtime groupV3d0 (ms): 440.7029265 > > Now, without the reducer -- thus, just a big old fold() at the end. > > Runtime group (ms): 442.3166115 > Runtime groupV3d0 (ms): 444.6889403 > > Thus, I think this is a good move as we don't have to worry about OME with > group() and we are getting slightly faster runtimes. I haven't spent much > time optimizing the code either so I suspect we can get GroupStep even faster! > > Enjoy, > Marko. > > http://markorodriguez.com > > On Oct 7, 2015, at 11:52 AM, Marko Rodriguez <[email protected]> wrote: > >> Hi everyone, >> >> I was soooooo close to having group() be backwards compatible. However, I >> found a corner case that makes it break. >> >> If you do something like: >> >> g.V.group.by('name').by(outE.weight).by(max(local)) >> >> If you did this, I would know you were using "the old way" as your provided >> a reduce by()-modulator. I would simply make your valueTraversal be >> "outE.weight.fold." Easy peasy. In fact, all the test cases passed. >> >> What about if you don't add a reduceTraversal and you want "the old way?" >> >> gremlin> g.V.group.by('name').by(outE.weight) // old way >> ==>[marko:[0.4,0.5,0.1] >> >> gremlin> g.V.group.by('name').by(outE.weight) // new way >> ==>[marko:0.4] >> >> Now, I was thinking, well we can say that if the valueTraversal doesn't end >> with a reducing barrier step, then we add fold() and tada, they are the >> same. However, what about this situation: >> >> >> >> g.V.group.by('name').by(constant(1)) >> old way would return a List[1,1,1,1,1,1,1] >> new way returns 1 >> now, if you provided a reducer, I would be like: "Ah! you are using the old >> way, let me make it constant(1).fold()" >> but if you don't provide a reducer, then we don't know if you are using old >> or new version. >> >> http://markorodriguez.com >> >> On Oct 7, 2015, at 7:28 AM, Marko Rodriguez <[email protected]> wrote: >> >>> Hi, >>> >>>> - Don't keep the old GropupStep at all, and have the new GroupStep "concat" >>>> the second and third 'by' traversals if they exist. >>> >>> This is not possible. In fact, I messed up my original examples. They >>> should be: >>> >>> g.V.group.by('name').by(outE.weight).by(max(local)) // old way >>> g.V.group.by('name').by(outE.weight.max()) // new way >>> >>> Notice the semantics of the reduce in the old way. Its looking for a >>> collection, not a stream. You can argue, well introspect and flatten/etc. >>> That is easy for max(), sum(), etc, but what about lambdas? What about >>> complex group().by()s people are using I don't even know about and mess up >>> the reasoning on? >>> >>>> - Leave the GroupStep as-is, and add the mapValues step for whoever wants a >>>> better implementation: >>> >>> mapValues() is already step in use. >>> >>> http://tinkerpop.incubator.apache.org/docs/3.1.0-SNAPSHOT/#mapvalues-step >>> >>>> g.V().group().by(keyTraversal).mapValues(valueTraversal). >>>> No deprecation needed. >>> >>> Can't do it cause of mapValues() and we need to keep modulators consistent, >>> hence by()-modulation. I don't want to introduce a new modulator just for >>> this situation as it makes the language start to look hackish. "Oh >>> mapValues() isn't a step, its a modulator like by() as as() but only works >>> for group() as the second modulator?!" Right now we have as() and by() as >>> our "special" modulating steps. >>> >>> Finally, to your notion of group(traversal). We have group(String) which >>> uses GroupSideEffectStep. Furthermore, all step(traversal) are used >>> exclusively for nesting and not for modulation (e.g. local(), repeat(), >>> etc.). Using a traversal parameter in this way would ruin the consistency >>> of the language. >>> >>> Thank you for your thoughts, >>> Marko. >>> >>> http://markorodriguez.com >>> >>> >>>> >>>> On Wed, 7 Oct 2015 at 14:42 Ran Magen <[email protected]> wrote: >>>> >>>>> My point is that group().by('key').by('value/reduce') seems kind of weird >>>>> anyway. The equivalent sql would be "select 'value/reduce' ... group by >>>>> 'key' ". >>>>> >>>>> A few alternative options: >>>>> g.V().group().by(keyTraversal).mapValues(valueTraversal) //As Marko >>>>> pointed out this doesn't address the old GroupStep deprecation >>>>> >>>>> g.V().group(keyTraversal).mapValues(valueTraversal) // group() is >>>>> @Deprecated >>>>> >>>>> g.V().group(valueTraversal).by(keyTraversal) // group() is @Deprecated >>>>> >>>>> g.V().group(keyTraversal, valueTraversal) // group() is @Deprecated >>>>> >>>>> >>>>> On Wed, 7 Oct 2015 at 01:42 Marko Rodriguez <[email protected]> wrote: >>>>> >>>>>> Uh…. First select() is already taken and by() is always a >>>>>> by()-modulator…… :/ Even with that, we still can't fix the backwards >>>>>> compatibility issue as we can't deprecate out the second by(). How do you >>>>>> say "if the second by() is applied, its Deprecated?" Perhaps Logger.WARN >>>>>> or >>>>>> something.. Dunno, sorta odd. >>>>>> >>>>>> Marko. >>>>>> >>>>>> http://markorodriguez.com >>>>>> >>>>>> On Oct 6, 2015, at 3:26 PM, Ran Magen <[email protected]> wrote: >>>>>> >>>>>>> How about changing the second 'by' to a more indicative name (could >>>>>>> 'select' work?)? >>>>>>> >>>>>>> That way we could improve on the name (which always had me confused), >>>>>> and >>>>>>> leave the second+third 'by's as an indication to use the old >>>>>>> implementation... >>>>>>> >>>>>>> On יום ד׳, 7 באוק׳ 2015 at 0:18 Marko Rodriguez <[email protected]> >>>>>>> wrote: >>>>>>> >>>>>>>> Hello everyone (and the NSA -- Kuppitz did it), >>>>>>>> >>>>>>>> This is regarding this ticket: >>>>>>>> https://issues.apache.org/jira/browse/TINKERPOP3-866 >>>>>>>> >>>>>>>> In group_step_2/ branch we have a new implementation of GroupStep. >>>>>> Here is >>>>>>>> an example: >>>>>>>> >>>>>>>> g.V.group.by('name').by(outE.weight).by(max()) // old way >>>>>>>> g.V.group.by('name').by(outE.weight.max()) // new way >>>>>>>> >>>>>>>> What is the difference, there is only two by()-modulators that can be >>>>>>>> passed to group(): keyTraversal and valueTraversal. What happened to >>>>>>>> reduceTraversal? Well, valueTraversal is that. If you want to reduce on >>>>>>>> your values, well, add a reducing barrier step -- e.g. max, min, sum, >>>>>>>> count, fold, mean, etc. In the old way, we would put all your values >>>>>> into a >>>>>>>> Collection<V> and then when the step was complete, we would feed those >>>>>> into >>>>>>>> the reduceTraversal. Now -- we can dynamically reduce on the fly. This >>>>>> will >>>>>>>> greatly improve speed and memory usage as we don't have to create a >>>>>> big old >>>>>>>> Collection<V> and instead, can reduce as new traversers are fed into >>>>>>>> valueTraversal. >>>>>>>> >>>>>>>> So this is great and no would object. What sucks is that this is NOT >>>>>>>> backwards compatible. What Stephen and I came up with (in the ticket) >>>>>> is to >>>>>>>> make: >>>>>>>> >>>>>>>> >>>>>>>> >>>>>> https://github.com/apache/incubator-tinkerpop/blob/group_step_2/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/dsl/graph/GraphTraversal.java#L637-L639 >>>>>>>> >>>>>>>> This new step groupV3d0() is a deprecated step that is what the "old >>>>>> way" >>>>>>>> was. Note the naming convention that Stephen used for Kryo is now used >>>>>> here >>>>>>>> -- V3d0 (version 3<dot>0). Thus, for users moving forward, they will >>>>>> have >>>>>>>> to do a "replace all" on their queries and rename "group()" to >>>>>>>> "groupV3d0()". Then slowly, over time, convert their traversal to use >>>>>> the >>>>>>>> new group() step and its respective semantics. >>>>>>>> >>>>>>>> Note that in the past we have been able to do backwards compatibility >>>>>> via >>>>>>>> having the new step (e.g. AddVertexStep) simulate the behavior of the >>>>>> old >>>>>>>> step and have respective @Deprecates in there to say that particular >>>>>>>> GraphTraversal methods will go away. However, in this situation, the >>>>>> best >>>>>>>> we can do is groupV3d0(). >>>>>>>> >>>>>>>> Anywho. That is that. If people have any better ideas on how to solve >>>>>> this >>>>>>>> backwards compatibility issue, I'm all (deaf) ears. >>>>>>>> >>>>>>>> Thanks, >>>>>>>> Marko. >>>>>>>> >>>>>>>> http://markorodriguez.com >>>>>>>> >>>>>>>> >>>>>> >>>>>> >>> >> >
