You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tinkerpop.apache.org by spmallette <gi...@git.apache.org> on 2016/06/16 19:36:33 UTC
[GitHub] tinkerpop pull request #342: TINKERPOP-1063 TinkerGraph Performance
GitHub user spmallette opened a pull request:
https://github.com/apache/tinkerpop/pull/342
TINKERPOP-1063 TinkerGraph Performance
https://issues.apache.org/jira/browse/TINKERPOP-1063
Got rid of `Stream` usage in TinkerGraph vertex/edge iteration. Tests all pass with `mvn clean install`.
VOTE +1
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/apache/tinkerpop TINKERPOP-1063
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/tinkerpop/pull/342.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #342
----
commit 70dd3245eadcf2dd69720c50fbffd3b0c9e92922
Author: Stephen Mallette <sp...@genoprime.com>
Date: 2015-12-22T21:43:45Z
Improved performance of TinkerGraph around element lookup.
Primary change was to drop use of Stream and use IteratorUtils.
commit e59b93ca4ec7ffbde4a4b91319f35f5d3fd9caeb
Author: Stephen Mallette <sp...@genoprime.com>
Date: 2016-06-16T19:08:30Z
Made TinkerGraph validate that ids be homogeneous up front.
Take this approach rather than doing it during iteration so that TinkerPop semantics are respected and appropriate validation exceptions are thrown at the right time.
commit 099e6bc62468516974a3e76531d00444dbe89edc
Author: Stephen Mallette <sp...@genoprime.com>
Date: 2016-06-16T19:32:58Z
Updated changelog.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop pull request #342: TINKERPOP-1063 TinkerGraph Performance
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/342#discussion_r67417561
--- Diff: tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerGraph.java ---
@@ -358,6 +350,14 @@ public Features features() {
return features;
}
+ private void validateHomogenousIds(final List<Object> ids) {
+ final Class firstClass = ids.get(0).getClass();
+ for (Object id : ids) {
+ if (!id.getClass().equals(firstClass))
+ throw Graph.Exceptions.idArgsMustBeEitherIdOrElement();
+ }
+ }
+
--- End diff --
cool - i will integrate - nice find. might also add a test for null
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop pull request #342: TINKERPOP-1063 TinkerGraph Performance
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/tinkerpop/pull/342
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #342: TINKERPOP-1063 TinkerGraph Performance
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the issue:
https://github.com/apache/tinkerpop/pull/342
If we drop the `TinkerGraphStepStrategy` then you lose index lookups and everything is a full scan.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop pull request #342: TINKERPOP-1063 TinkerGraph Performance
Posted by dkuppitz <gi...@git.apache.org>.
Github user dkuppitz commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/342#discussion_r67415317
--- Diff: tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerGraph.java ---
@@ -358,6 +350,14 @@ public Features features() {
return features;
}
+ private void validateHomogenousIds(final List<Object> ids) {
+ final Class firstClass = ids.get(0).getClass();
+ for (Object id : ids) {
+ if (!id.getClass().equals(firstClass))
+ throw Graph.Exceptions.idArgsMustBeEitherIdOrElement();
+ }
+ }
+
--- End diff --
This opens the door for NPE's.
```
gremlin> graph.vertices("a", null, 1)
java.lang.NullPointerException
Display stack trace? [yN] y
java.lang.NullPointerException
at org.apache.tinkerpop.gremlin.tinkergraph.structure.TinkerGraph.validateHomogenousIds(TinkerGraph.java:356)
at org.apache.tinkerpop.gremlin.tinkergraph.structure.TinkerGraph.createElementIterator(TinkerGraph.java:329)
at org.apache.tinkerpop.gremlin.tinkergraph.structure.TinkerGraph.vertices(TinkerGraph.java:269)
```
Suggestion with a few tweaks:
```
private void validateHomogenousIds(final List<Object> ids) {
final Iterator<Object> iterator = ids.iterator();
Object id = iterator.next();
if (id == null)
throw Graph.Exceptions.idArgsMustBeEitherIdOrElement();
final Class firstClass = id.getClass();
while (iterator.hasNext()) {
id = iterator.next();
if (id == null || !id.getClass().equals(firstClass))
throw Graph.Exceptions.idArgsMustBeEitherIdOrElement();
}
}
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #342: TINKERPOP-1063 TinkerGraph Performance
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the issue:
https://github.com/apache/tinkerpop/pull/342
@dkuppitz I'd forgotten to implement that change you suggested on this one - all done now.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #342: TINKERPOP-1063 TinkerGraph Performance
Posted by okram <gi...@git.apache.org>.
Github user okram commented on the issue:
https://github.com/apache/tinkerpop/pull/342
Cool stuff. `Stream` is evil. It lurks throughout the codebase... Its funny, we were all gun ho for Java8, but we barely use it :( .... VOTE +1.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #342: TINKERPOP-1063 TinkerGraph Performance
Posted by dkuppitz <gi...@git.apache.org>.
Github user dkuppitz commented on the issue:
https://github.com/apache/tinkerpop/pull/342
VOTE: +1
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #342: TINKERPOP-1063 TinkerGraph Performance
Posted by okram <gi...@git.apache.org>.
Github user okram commented on the issue:
https://github.com/apache/tinkerpop/pull/342
Do we know that its faster?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #342: TINKERPOP-1063 TinkerGraph Performance
Posted by Jotschi <gi...@git.apache.org>.
Github user Jotschi commented on the issue:
https://github.com/apache/tinkerpop/pull/342
Are TraversalStrategy optional? Or is it really needed to apply them? I just noticed that removing the TinkerGraphStepStrategy has a huge impact on the Ferma Benchmark results.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #342: TINKERPOP-1063 TinkerGraph Performance
Posted by Jotschi <gi...@git.apache.org>.
Github user Jotschi commented on the issue:
https://github.com/apache/tinkerpop/pull/342
@okram ~18% to 16% faster without suggested tweaks (I used the Ferma Benchmark and compared merged PR with tp31 branch code)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---