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 2018/04/24 22:50:28 UTC
[GitHub] tinkerpop pull request #855: TINKERPOP-1950 Cached global strategy lookups d...
GitHub user spmallette opened a pull request:
https://github.com/apache/tinkerpop/pull/855
TINKERPOP-1950 Cached global strategy lookups during traversal construction
https://issues.apache.org/jira/browse/TINKERPOP-1950
This change leads to a 1.5x to 2x speed improvement in traversal construction. It is especially effective when processing traversals that have many child traversals within them as this method is called for not only the parent traversal but all the children as well.
All tests pass with `docker/build.sh -t -i`
VOTE +1
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/apache/tinkerpop TINKERPOP-1950
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/tinkerpop/pull/855.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 #855
----
commit d7d0aafc08cf8600b03d4d9d79ce52056d694a4f
Author: Stephen Mallette <sp...@...>
Date: 2018-04-20T20:18:13Z
TINKERPOP-1950 Cached global strategy lookups during traversal construction
This change leads to a 1.5x to 2x speed improvement in traversal construction. It is especially effective when processing traversals that have many child traversals within them as this method is called for not only the parent traversal but all the children as well.
----
---
[GitHub] tinkerpop issue #855: TINKERPOP-1950 Cached global strategy lookups during t...
Posted by robertdale <gi...@git.apache.org>.
Github user robertdale commented on the issue:
https://github.com/apache/tinkerpop/pull/855
I think `computeIfAbsent()` would have been better :goat:
---
[GitHub] tinkerpop issue #855: TINKERPOP-1950 Cached global strategy lookups during t...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the issue:
https://github.com/apache/tinkerpop/pull/855
That method isn't available on `ConcurrentHashMap.newKeySet()` - is there some other way to get to that?
---
[GitHub] tinkerpop pull request #855: TINKERPOP-1950 Cached global strategy lookups d...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/855#discussion_r184028279
--- Diff: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/TraversalStrategies.java ---
@@ -63,6 +63,8 @@
*/
public interface TraversalStrategies extends Serializable, Cloneable {
+ static Set<Class> LOADED = new HashSet<>();
--- End diff --
ha - i was so excited that this changed worked i didn't bother to think too hard about what i was doing. good suggestions in both cases - i've made those changes now. thanks
---
[GitHub] tinkerpop issue #855: TINKERPOP-1950 Cached global strategy lookups during t...
Posted by dkuppitz <gi...@git.apache.org>.
Github user dkuppitz commented on the issue:
https://github.com/apache/tinkerpop/pull/855
VOTE: +1
---
[GitHub] tinkerpop issue #855: TINKERPOP-1950 Cached global strategy lookups during t...
Posted by jorgebay <gi...@git.apache.org>.
Github user jorgebay commented on the issue:
https://github.com/apache/tinkerpop/pull/855
VOTE +1
---
[GitHub] tinkerpop issue #855: TINKERPOP-1950 Cached global strategy lookups during t...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the issue:
https://github.com/apache/tinkerpop/pull/855
ah - ok, just use the Map rather than the Set....fair enough
---
[GitHub] tinkerpop pull request #855: TINKERPOP-1950 Cached global strategy lookups d...
Posted by jorgebay <gi...@git.apache.org>.
Github user jorgebay commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/855#discussion_r183959745
--- Diff: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/TraversalStrategies.java ---
@@ -63,6 +63,8 @@
*/
public interface TraversalStrategies extends Serializable, Cloneable {
+ static Set<Class> LOADED = new HashSet<>();
--- End diff --
Shouldn't we use a collection that supports concurrent modification? Different threads might be adding items to the collection.
Also, we could move the declaration to `GlobalCache`?.
---
[GitHub] tinkerpop issue #855: TINKERPOP-1950 Cached global strategy lookups during t...
Posted by robertdale <gi...@git.apache.org>.
Github user robertdale commented on the issue:
https://github.com/apache/tinkerpop/pull/855
Well, you would do use ConcurrentHashMap ;-)
```
static Map<Class, Boolean> LOADED = new ConcurrentHashMap<>();
try {
LOADED.computeIfAbsent(graphOrGraphComputerClass, (unused) -> {
final String graphComputerClassName = null != graphOrGraphComputerClass.getDeclaringClass()
? graphOrGraphComputerClass.getCanonicalName().replace(
"." + graphOrGraphComputerClass.getSimpleName(),
"$" + graphOrGraphComputerClass.getSimpleName())
: graphOrGraphComputerClass.getCanonicalName();
try {
Class.forName(graphComputerClassName);
} catch (ClassNotFoundException e) {
throw new RuntimeException(e);
}
// keep track of stuff we already loaded once - stuff in this if/statement isn't
// cheap and this
// method gets called a lot, basically every time a new traversal gets spun up
// (that includes
// child traversals. perhaps it is possible to just check the cache keys for
// this information, but
// it's not clear if this method will be called with something not in the cache
// and if it is and
// it results in error, then we'd probably not want to deal with this block
// again anyway
return Boolean.TRUE;
});
} catch (Exception e) {
throw new IllegalStateException(e.getCause().getMessage(), e.getCause());
}
```
---
[GitHub] tinkerpop pull request #855: TINKERPOP-1950 Cached global strategy lookups d...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/tinkerpop/pull/855
---
[GitHub] tinkerpop issue #855: TINKERPOP-1950 Cached global strategy lookups during t...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the issue:
https://github.com/apache/tinkerpop/pull/855
done - https://github.com/apache/tinkerpop/commit/1d9e6dc6d30c5c7d56e4007527365793eb1f223e
---