You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tinkerpop.apache.org by BrynCooke <gi...@git.apache.org> on 2017/03/06 13:57:31 UTC
[GitHub] tinkerpop pull request #567: TINKERPOP-1644 Improve script compilation syncr...
GitHub user BrynCooke opened a pull request:
https://github.com/apache/tinkerpop/pull/567
TINKERPOP-1644 Improve script compilation syncronisation
Script compilation is synchronised.
Script compilation times are placed in to logs.
Failed scripts will not be recompiled.
Scripts that take over 5 seconds to compile are logged as a warning.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/BrynCooke/incubator-tinkerpop TINKERPOP-1644
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/tinkerpop/pull/567.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 #567
----
commit ca229694dc036210ad9e7ae2fc32134720b48d1b
Author: BrynCooke <br...@gmail.com>
Date: 2017-03-02T19:07:28Z
TINKERPOP-1644 Improve script compilation syncronisation
Script compilation is synchronised.
Script compilation times are placed in to logs.
Failed scripts will not be recompiled.
Scripts that take over 5 seconds to compile are logged as a warning.
----
---
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 #567: TINKERPOP-1644 Improve script compilation syncronisati...
Posted by okram <gi...@git.apache.org>.
Github user okram commented on the issue:
https://github.com/apache/tinkerpop/pull/567
Yes. `computeIfAbsent()` is much better than synchronized `get()`, check, `put()`.
---
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 #567: TINKERPOP-1644 Improve script compilation syncronisati...
Posted by robertdale <gi...@git.apache.org>.
Github user robertdale commented on the issue:
https://github.com/apache/tinkerpop/pull/567
That sounds like a good alternative.
---
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 #567: TINKERPOP-1644 Improve script compilation syncronisati...
Posted by robertdale <gi...@git.apache.org>.
Github user robertdale commented on the issue:
https://github.com/apache/tinkerpop/pull/567
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.
---
RE: [GitHub] tinkerpop issue #567: TINKERPOP-1644 Improve script
compilation syncronisati...
Posted by "Paul A. Jackson" <Pa...@pb.com>.
This code is using the double-checked locking idiom, which does not work in Java:
Class clazz = getScriptClassFromMap(script);
if (clazz != null) {
return clazz;
}
synchronized (this) {
clazz = getScriptClassFromMap(script);
if (clazz != null) {
return clazz;
}
See https://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html
I don't know the details well enough to judge whether the failure modes will be harmful in this case. I think it's best avoided or replaced with one of the suggestions in the link above.
-Paul
-----Original Message-----
From: spmallette [mailto:git@git.apache.org]
Sent: Tuesday, March 07, 2017 6:25 AM
To: dev@tinkerpop.apache.org
Subject: [GitHub] tinkerpop issue #567: TINKERPOP-1644 Improve script compilation syncronisati...
Github user spmallette commented on the issue:
https://github.com/apache/tinkerpop/pull/567
I will clean up a few code formatting things on merge, but this looks good to me. All tests pass with `docker/build.sh -t -n -i`
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 #567: TINKERPOP-1644 Improve script compilation syncronisati...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the issue:
https://github.com/apache/tinkerpop/pull/567
I will clean up a few code formatting things on merge, but this looks good to me. All tests pass with `docker/build.sh -t -n -i`
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 #567: TINKERPOP-1644 Improve script compilation syncronisati...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the issue:
https://github.com/apache/tinkerpop/pull/567
I like this way better than the synchronization. I have some reservations about:
```java
CompletableFuture.supplyAsync(() -> loader.parseClass(script, generateScriptName()));
```
That's going to use the `ForkJoinPool` to execute the compilation, which is a shared thread pool for the JVM that isn't configurable and basically outside of our control. Of course, getting rid of it means adding a new `ExecutorService` that we can configure and monitor in `GremlinGroovyScriptEngine`. Making that kind of change goes pretty deep into various aspects of TinkerPop. If @BrynCooke is ok with the changes I can merge what he has to a branch since it seems like we have general consensus on the approach and then make some additional changes and re-issue the PR for a final VOTE. If there's no objections I'll start work on that first thing tomorrow.
---
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 #567: TINKERPOP-1644 Improve script compilation syncronisati...
Posted by dkuppitz <gi...@git.apache.org>.
Github user dkuppitz commented on the issue:
https://github.com/apache/tinkerpop/pull/567
Hmm, I haven't done any benchmarks, but this seem to be valid points. Just a thought: Would it help to have the class map be like `ManagedConcurrentValueMap<String, Future<Class>> classMap`?
---
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 #567: TINKERPOP-1644 Improve script compilation syncronisati...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the issue:
https://github.com/apache/tinkerpop/pull/567
I've merged these changes to a branch so that I can continue with the development needed on this one:
https://github.com/apache/tinkerpop/tree/TINKERPOP-1644
@BrynCooke you can feel free to close this PR now. I'll issue a new one from the TinkerPop branch when I'm done.
---
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 #567: TINKERPOP-1644 Improve script compilation syncronisati...
Posted by BrynCooke <gi...@git.apache.org>.
Github user BrynCooke commented on the issue:
https://github.com/apache/tinkerpop/pull/567
1. ManagedConcurrentValueMap is not a map. I don't really know why we use this. but computeIfAbsent is not present on this class.
2. Guava LoadingCache and MoreExecutors.directExecutor() is the correct way of solving this problem. It allows soft values and does all the syncronisation stuff for us. Is there a reason why guava is not used by Tinkerpop?
If we want avoid guava we need to have a striped lock on script, and to create our own direct executor.
---
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 #567: TINKERPOP-1644 Improve script compilation syncr...
Posted by BrynCooke <gi...@git.apache.org>.
Github user BrynCooke closed the pull request at:
https://github.com/apache/tinkerpop/pull/567
---
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 #567: TINKERPOP-1644 Improve script compilation syncronisati...
Posted by dkuppitz <gi...@git.apache.org>.
Github user dkuppitz commented on the issue:
https://github.com/apache/tinkerpop/pull/567
Yep, missing some spaces here and there, but the code in general looks good.
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 #567: TINKERPOP-1644 Improve script compilation syncronisati...
Posted by robertdale <gi...@git.apache.org>.
Github user robertdale commented on the issue:
https://github.com/apache/tinkerpop/pull/567
I have a concern on overall performance. I agree that "Script compilation is synchronised" helps in the rare case when two or more identical scripts are submitted concurrently. However, in the more likely case of new, unique scripts being submitted concurrently, it would seem that this effectively serializes script compilation creating a bottleneck. Unless I'm missing something, parallel compilation seems to be the better choice.
---
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 #567: TINKERPOP-1644 Improve script compilation syncronisati...
Posted by BrynCooke <gi...@git.apache.org>.
Github user BrynCooke commented on the issue:
https://github.com/apache/tinkerpop/pull/567
I like the idea of using a Future.
---
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 #567: TINKERPOP-1644 Improve script compilation syncronisati...
Posted by dkuppitz <gi...@git.apache.org>.
Github user dkuppitz commented on the issue:
https://github.com/apache/tinkerpop/pull/567
This part:
```
if (clazz != null) {
return clazz.get();
}
clazz = CompletableFuture.supplyAsync(() -> loader.parseClass(script, generateScriptName()));
classMap.put(script, clazz);
return clazz.get();
```
should be more like:
```
classMap.computeIfAbsent(script, s ->
CompletableFuture.supplyAsync(() -> loader.parseClass(s, generateScriptName())))
return classMap.get(script).get();
```
No?
---
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 #567: TINKERPOP-1644 Improve script compilation syncronisati...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the issue:
https://github.com/apache/tinkerpop/pull/567
`ManagedConcurrentValueMap` allows you to configure a `Map` with weak/soft/phantom/hard references. Groovy has "Managed" classes for lots of different types of Lists/Maps.
---
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.
---