You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by bostko <gi...@git.apache.org> on 2017/01/10 19:34:57 UTC
[GitHub] brooklyn-server pull request #513: Use stripped version of ScriptBytecodeAda...
GitHub user bostko opened a pull request:
https://github.com/apache/brooklyn-server/pull/513
Use stripped version of ScriptBytecodeAdapter.isCase
- Optimize performance and avoid locks used in groovy dynamic invocation
Should fix BROOKLYN-424
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/bostko/brooklyn-server less_dynamic_groovy_is_case
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/brooklyn-server/pull/513.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 #513
----
commit da6b4a0e5d92ac1836879a0903b52e2cd454eaaa
Author: Valentin Aitken <bo...@gmail.com>
Date: 2017-01-10T19:29:25Z
Use stripped version of ScriptBytecodeAdapter.isCase
- Optimize performance and avoid locks used in groovy dynamic invocation
Should fix BROOKLYN-424
----
---
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] brooklyn-server issue #513: Use stripped version of ScriptBytecodeAdapter.is...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the issue:
https://github.com/apache/brooklyn-server/pull/513
@bostko see https://github.com/bostko/brooklyn-server/pull/3
---
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] brooklyn-server pull request #513: Use stripped version of ScriptBytecodeAda...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/brooklyn-server/pull/513
---
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] brooklyn-server pull request #513: Use stripped version of ScriptBytecodeAda...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/513#discussion_r95593265
--- Diff: utils/groovy/src/main/java/org/apache/brooklyn/util/groovy/GroovyJavaMethods.java ---
@@ -75,6 +76,20 @@
}
}
+ /**
+ * Alternative implementation of {@link ScriptBytecodeAdapter#isCase(Object, Object)}<br>
+ * Stripped down to work only for caseExpression of type <code>java.lang.Class</code>.<br>
--- End diff --
I wondered about that as well - that was my initial reaction. However, I'm fine with doing it like this for two reasons.
First, we can change most callers to instead use `JavaGroovyEquivalents` to avoid calling groovy code at all. Second, groovy allows some weird stuff with meta-programming, so I think it's safest to call the real Groovy 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.
---
[GitHub] brooklyn-server issue #513: Use stripped version of ScriptBytecodeAdapter.is...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the issue:
https://github.com/apache/brooklyn-server/pull/513
LGTM; merging - thanks @bostko
---
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] brooklyn-server pull request #513: Use stripped version of ScriptBytecodeAda...
Posted by geomacy <gi...@git.apache.org>.
Github user geomacy commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/513#discussion_r95553319
--- Diff: utils/groovy/src/main/java/org/apache/brooklyn/util/groovy/GroovyJavaMethods.java ---
@@ -75,6 +76,20 @@
}
}
+ /**
+ * Alternative implementation of {@link ScriptBytecodeAdapter#isCase(Object, Object)}<br>
+ * Stripped down to work only for caseExpression of type <code>java.lang.Class</code>.<br>
+ * It behaves the same way only for cases when caseExpression <code>java.lang.Class</code> does <b>not</b> implement <code>isCase</code> method.<br>
+ * It goes directly to {@link DefaultGroovyMethods#isCase(Object, Object)} method instead of using Groovy dynamic invocation.<br>
+ * This safes extra operations and locks used in Groovy dynamic invocation. See <a href="https://issues.apache.org/jira/browse/BROOKLYN-424">BROOKLYN-424</a>.
--- End diff --
Typo "safes". Maybe something like "This saves extra operations and avoids the locks used ...."
---
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] brooklyn-server pull request #513: Use stripped version of ScriptBytecodeAda...
Posted by geomacy <gi...@git.apache.org>.
Github user geomacy commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/513#discussion_r95572556
--- Diff: utils/groovy/src/main/java/org/apache/brooklyn/util/groovy/GroovyJavaMethods.java ---
@@ -75,6 +76,20 @@
}
}
+ /**
+ * Alternative implementation of {@link ScriptBytecodeAdapter#isCase(Object, Object)}<br>
+ * Stripped down to work only for caseExpression of type <code>java.lang.Class</code>.<br>
--- End diff --
My point is that _if_ we don't need the functionality that `ScriptBytecodeAdapter` was giving us, then we should keep it as simple as possible; otherwise people will be looking at this code thinking "why is the call to this Groovy stuff needed?" when in fact it's not. It will be confusing. There's no need for it to do "isCase" any more, is there?
---
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] brooklyn-server issue #513: Use stripped version of ScriptBytecodeAdapter.is...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the issue:
https://github.com/apache/brooklyn-server/pull/513
@bostko I'm reviewing this now, and will submit an additional commit on top of this PR shortly. I think a lot of things can be improved by instead calling `JavaGroovyEquivalents.groovyTruth(...)` and `JavaGroovyEquivalents.elvis(...)` so that we entirely avoid groovy code! I'm making those changes now and will let you know when that's done/tested.
---
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] brooklyn-server pull request #513: Use stripped version of ScriptBytecodeAda...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/513#discussion_r95590807
--- Diff: utils/groovy/src/main/java/org/apache/brooklyn/util/groovy/GroovyJavaMethods.java ---
@@ -75,6 +76,20 @@
}
}
+ /**
+ * Alternative implementation of {@link ScriptBytecodeAdapter#isCase(Object, Object)}<br>
+ * Stripped down to work only for caseExpression of type <code>java.lang.Class</code>.<br>
+ * It behaves the same way only for cases when caseExpression <code>java.lang.Class</code> does <b>not</b> implement <code>isCase</code> method.<br>
+ * It goes directly to {@link DefaultGroovyMethods#isCase(Object, Object)} method instead of using Groovy dynamic invocation.<br>
+ * This safes extra operations and locks used in Groovy dynamic invocation. See <a href="https://issues.apache.org/jira/browse/BROOKLYN-424">BROOKLYN-424</a>.
+ */
+ public static boolean scriptBytecodeAdapter_isCase(Object switchValue, Class caseExpression) {
--- End diff --
If we keep it, let's make it package-private (so tests can still call it).
---
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] brooklyn-server pull request #513: Use stripped version of ScriptBytecodeAda...
Posted by bostko <gi...@git.apache.org>.
Github user bostko commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/513#discussion_r95570678
--- Diff: utils/groovy/src/main/java/org/apache/brooklyn/util/groovy/GroovyJavaMethods.java ---
@@ -75,6 +76,20 @@
}
}
+ /**
+ * Alternative implementation of {@link ScriptBytecodeAdapter#isCase(Object, Object)}<br>
+ * Stripped down to work only for caseExpression of type <code>java.lang.Class</code>.<br>
--- End diff --
I think it is the same.
I prefer reusing the actual method being used so you can easily compare implementations of `ScriptBytecodeAdapter#isCase(Object, Object)` and `safeGroovyIsCase` just by checking that at the end both refer to `DefaultGroovyMethods.isCase`
---
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] brooklyn-server pull request #513: Use stripped version of ScriptBytecodeAda...
Posted by geomacy <gi...@git.apache.org>.
Github user geomacy commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/513#discussion_r95554328
--- Diff: utils/groovy/src/main/java/org/apache/brooklyn/util/groovy/GroovyJavaMethods.java ---
@@ -75,6 +76,20 @@
}
}
+ /**
+ * Alternative implementation of {@link ScriptBytecodeAdapter#isCase(Object, Object)}<br>
+ * Stripped down to work only for caseExpression of type <code>java.lang.Class</code>.<br>
+ * It behaves the same way only for cases when caseExpression <code>java.lang.Class</code> does <b>not</b> implement <code>isCase</code> method.<br>
+ * It goes directly to {@link DefaultGroovyMethods#isCase(Object, Object)} method instead of using Groovy dynamic invocation.<br>
+ * This safes extra operations and locks used in Groovy dynamic invocation. See <a href="https://issues.apache.org/jira/browse/BROOKLYN-424">BROOKLYN-424</a>.
+ */
+ public static boolean scriptBytecodeAdapter_isCase(Object switchValue, Class caseExpression) {
--- End diff --
If we do keep this method, I'd suggest we change the name, since it now has nothing to do with ScriptByteCodeAdapter, maybe call it something like `safeGroovyIsCase`.
---
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] brooklyn-server pull request #513: Use stripped version of ScriptBytecodeAda...
Posted by geomacy <gi...@git.apache.org>.
Github user geomacy commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/513#discussion_r95552960
--- Diff: utils/groovy/src/main/java/org/apache/brooklyn/util/groovy/GroovyJavaMethods.java ---
@@ -75,6 +76,20 @@
}
}
+ /**
+ * Alternative implementation of {@link ScriptBytecodeAdapter#isCase(Object, Object)}<br>
+ * Stripped down to work only for caseExpression of type <code>java.lang.Class</code>.<br>
--- End diff --
If we're OK to do that then do we even need this method? Couldn't we just use instanceof/isAssignableFrom as appropriate?
```java
if ( job instanceof Callable) {
```
etc?
---
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.
---