TINKERPOP-1585 & TINKERPOP-1590: DedupGlobalStep and Distributed TinkerMemory #524
Conversation
… which is already dedup'd is added to the master traveral's dedup-step and then dedup'd again. I simple boolean flag added to determine if the master traversal's dedup is getting data from workers or not and thus, if so, don't dedup again.
…read before propagated Memory to TinkerMemory. This reduces synchronization issues due all threads contending to mutate the master memory.
… method). TinkerGraphComputer now how a sound distributed Memory system where each worker/thread aggregates without concurrency locally and then, at the end of the iteration, the thread-distributed memories are aggregated into the main TinkerMemory.
…ally important we have NOT been cloning the BiOperators of OrderGlobalStep and GroupStep. We have just been 'getting lucky' in that Spark and Giraph use Serialization and thus we get a clone for free. However, for parallelization within a JVM, we woulld have issues except we never realized because we had a single global Memory for TinkerGraph. Now we don't and clone()ing bi operators works.
…dkuppitz to verify performance improvement of DedupGlobalStep on SparkGraphComputer.
…r. Also, realized some dumb things I was doing in TraversalVertexProgram. Its crazy, for this benchmark that @dkuppitz and i have, if I don't touch vertex.properties that are compute keys: millisecond return times. If I do, seconds return times.... Need to figure out how to partition TinkerGraphView... Perhaps thread local..dah.
…ead has their own Iterator<Vertex> partition. In TinkerPop 3.3.0 (with Partitioner) this will be replaced (easily).
… in TraversalVertexProgram.clone().
…the addition of worker barriers into the master traversal. A slight performance increase.
…rs are the next thing. Also, detachment factory stuff will help reduce barrier sizes. Found a bug in SparkInterceptorStrategyTest. Fixed.
| this.barrier = null; | ||
| } | ||
| while (this.barrierIterator != null && this.barrierIterator.hasNext()) { | ||
| if (null == this.barrierIterator) |
There was a problem hiding this comment.
this.barrierIterator can never be null within the while() loop. Unless I overlooked something fundamental, processNextStart can be simplified to:
protected Traverser.Admin<S> processNextStart() {
if (null != this.barrier) {
this.barrierIterator = this.barrier.entrySet().iterator();
this.barrier = null;
while (this.barrierIterator.hasNext()) {
final Map.Entry<Object, Traverser.Admin<S>> entry = this.barrierIterator.next();
if (this.duplicateSet.add(entry.getKey()))
return PathProcessor.processTraverserPathLabels(entry.getValue(), this.keepLabels);
}
}
return PathProcessor.processTraverserPathLabels(super.processNextStart(), this.keepLabels);
}
There was a problem hiding this comment.
Your code fails. Did you run it? ... The problem is that this.barrier is null'd but the barrierIterator still exists and you still need to fetch results from it.
There was a problem hiding this comment.
I didn't run it. Just thought we can get rid of the multiple null checks.
| @@ -142,7 +142,7 @@ public void shouldSuccessfullyEvaluateInterceptedTraversals() throws Exception { | |||
| test(6l, g.V().out().values("name").count()); | |||
| test(2l, g.V().out("knows").values("name").count()); | |||
| test(3l, g.V().in().has("name", "marko").count()); | |||
| test(6l, g.V().repeat(__.dedup()).times(2).count()); | |||
| test(0l, g.V().repeat(__.dedup()).times(2).count()); | |||
There was a problem hiding this comment.
Why did that change? 0 as the expected result looks kinda wrong.
There was a problem hiding this comment.
This is crazy. The right answer is 0. Think about it. It goes through once, fine, all 6 vertices. The second time -- already seem them! Filter. Thus, 0.
However, I just checked OLTP and it assumes 6. Wondering what is "right" ?. ....
gremlin> g = TinkerFactory.createModern().traversal()
==>graphtraversalsource[tinkergraph[vertices:6 edges:6], standard]
gremlin> g.V().repeat(dedup()).times(2)
==>v[1]
==>v[2]
==>v[3]
==>v[4]
==>v[5]
==>v[6]
gremlin> g.withComputer().V().repeat(dedup()).times(2)
gremlin>
Whatever, is decided as the correct answer, we should definitely put this into RepeatTest. I just randomly had this query in a Spark test.
There was a problem hiding this comment.
As discussed in IM. This is actually a bug in RepeatUnrollStrategy that was introduced in 3.2, but doesn't exist in 3.1. Added a test case to DedupTest and both OLTP and OLAP now behave as expected.
…ke DedupGlobalStep can not be unrolled else you have split the global state amongst times(x) local steps. Added a test case to DedupGlobalStep g.V().repeat(dedup()).times(2).
|
VOTE: +1 |
|
Nice set of updates.
VOTE: +1 |
https://issues.apache.org/jira/browse/TINKERPOP-1585
https://issues.apache.org/jira/browse/TINKERPOP-1590
This was a week long project with @dkuppitz. In short, we made
DedupGlobalStepa bit better. It had a bug where it was deduping (in OLAP) twice at the master traversals. That is no longer and issue. Moreover, I added an explicit barrier data structure to reduce the amount of data movement between the master barrier and the master step. When working on this, I originally thought the bug was inTinkerMemory(TinkerGraphComputer) where there is lock contention for barrier data. There is! and I was able to seep up a toy traversal we had from 7 seconds to 2.9 seconds cause of it by having eachTinkerWorkermaintain its own memory data structure that is then merged into the masterTinkerMemorywhen the iteration is complete. Standard, we just never thought to do this. Lots of other nick nack goodies...VOTE +1.