Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TINKERPOP-1349: RepeatUnrollStrategy should unroll loops while maintaining equivalent semantics. #349

Merged
merged 8 commits into from
Jun 30, 2016

Conversation

okram
Copy link
Contributor

@okram okram commented Jun 28, 2016

https://issues.apache.org/jira/browse/TINKERPOP-1349

RepeatUnrollStrategy is a Standard-only strategy that will unroll a repeat() into a linear form if and only if it has a known loop amount (e.g. times(2)). That is repeat(out()).times(2) becomes out().barrier().out().barrier(). The barrier() insertions are necessary as repeat() itself is a barrier step and also, the benefit of bulking is leveraged. RepeatUnrollStrategy removes the need for RepeatStep, RepeatEndStep, and LoopTraversal.

RepeatUnrollStrategyTest ensures that unrolling is faster than not-unrolling. Here are some runtimes. The first item in the Pair is the runtime and the second item is the amount of unique objects being processed. With more unique objects, the faster it gets. Note that at 10 unique elements (tiny traversal), sometimes rolled is faster than unrolled and vice versa (i.e. they are equal).

rolled:[0.22843720299999998, 10]    unrolled:[0.174451055, 10]
rolled:[0.514145509, 100]       unrolled:[0.212532769, 100]
rolled:[1.4944357259999999, 1000]   unrolled:[0.441714987, 1000]
rolled:[28.502658626, 10000]        unrolled:[12.936112649999998, 10000]

Also in this ticket, I fixed a bug in BranchStep where children were not being integrated and thus, susceptible to clone-problems. Moreover, I added a timing test to IdentityRemovalStrategyTest. @spmallette, do you know how to make a test run once and only once in a Parameterized test case? If so, can you update IdentityRemoveStrategyTest and RepeatUnrollStrategyTest accordingly?


CHANGELOG

* Added `RepeatUnrollStrategy` to linearize a `repeat()`-traversal if loop amount is known at compile time.
* Fixed a bug in `BranchStep` around child integration during `clone()`.
* Fixed a bug in `AbstractStep` around label set cloning.

VOTE +1.

…le time. Fixed a bug in BranchStep around child integration.
@okram
Copy link
Contributor Author

okram commented Jun 28, 2016

NOTE: This can not go into tp31/ because NoOpBarrierStep is new to TinkerPop 3.2.0.

… Barrier, then there is no need to add a NoOpBarrier on the last loop serialization. Also, discovered a severe clone() bug in AbstractStep around label cloning. Can't believe this never popped up before now.
…fAssignableClass as it just wastes clock cycles. Also, came up with a cleaner algorithm for determing whether the final NoOpBarrierStep should be added or not. Faster and less memory usage.
@dkuppitz
Copy link
Contributor

VOTE: +1

okram and others added 3 commits June 28, 2016 14:58
…meterized sections of a test. I think all our Strategy tests should have a 'isFaster' test to ensure that our optimizations are actually making things faster. cc/ @dkuppitz.
…StrategyTest now do parameter sweeps and test a broader range of behaviors.
@spmallette
Copy link
Contributor

Travis is failing - looks like an issue from Rat - missing an Apache header in TraversalStrategyPerformanceTest.

@okram I don't think there's a way to get a Parameterized test to run a single test once. It pretty much treats the whole test case as parameterized when you use that feature. Using Enclosed as you did is the only way to make that work afaik.

@pietermartin do you intend to vote on this one? you had a fair number of comments/questions on the JIRA ticket....

@pietermartin
Copy link
Contributor

@spmallette No I do not intend to vote. The questions were about understanding why the barrier steps were needed and then to understand why barrier/bulking has such a significant performance impact. I have not read the RepeatUnrollStrategy code nor ran any tests for it.

@spmallette
Copy link
Contributor

All tests pass with docker/build.sh -t -n -i

VOTE +1

@dkuppitz
Copy link
Contributor

Same here, docker/build.sh -t -i -n reported success over night. All changes after my first vote were included in this test run.

@asfgit asfgit merged commit 857e2e2 into master Jun 30, 2016
@asfgit asfgit deleted the TINKERPOP-1349 branch August 3, 2016 12:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants