You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by GitBox <gi...@apache.org> on 2020/09/06 21:47:14 UTC
[GitHub] [lucene-solr] uschindler opened a new pull request #1837: LUCENE-7882: First idea of using Java 15 hidden anonymous classes for Lucene expressions
uschindler opened a new pull request #1837:
URL: https://github.com/apache/lucene-solr/pull/1837
This PR is a first (but already working) idea of using Java 15 hidden classes (see https://openjdk.java.net/jeps/371) to implement the Lucene expressions. The big advantages:
- No classloader for every expression is needed, because the class is completely anonymous and has no strong reference to a classloader. Actually the class has a classloader to lookup any referenced other class, but it is NOT loaded by any classloader
- The class can easily be unloaded
- The performance of loading that class is better, as no locks can occur (classloaders have locks when looking up classes), see https://issues.apache.org/jira/browse/LUCENE-7882
Backside:
- The class and its methods do not appear in any stack trace. This also fails one test in the test suite. When using this for Lucene expressions, we have to think about a better way how to make the source code and method calls visible in stack traces. Like lambda frames the hidden class is not visible (unless enabled in JVM to show hidden frames).
- It currently does not work if you pass a different classloader than Lucene's to the expressions module. To allow this we need to change APIs a bit (Classloader -> Lookup).
@mikemccand can you test this with JDK 15 (release candidate) and your test. You should not see any locks anymore, speed should be higher, and the created anonymous classes should be unloaded very fast.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] dweiss commented on a change in pull request #1837: LUCENE-7882: First idea of using Java 15 hidden anonymous classes for Lucene expressions
Posted by GitBox <gi...@apache.org>.
dweiss commented on a change in pull request #1837:
URL: https://github.com/apache/lucene-solr/pull/1837#discussion_r484223431
##########
File path: lucene/expressions/src/test/org/apache/lucene/expressions/js/TestCustomFunctions.java
##########
@@ -263,7 +263,7 @@ public void testThrowingException() throws Exception {
PrintWriter pw = new PrintWriter(sw);
Review comment:
I love assertj - it really prints out stuff in more elegant fashion than pure junit+hamcrest.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] rmuir commented on pull request #1837: LUCENE-7882: First idea of using Java 15 hidden anonymous classes for Lucene expressions
Posted by GitBox <gi...@apache.org>.
rmuir commented on pull request #1837:
URL: https://github.com/apache/lucene-solr/pull/1837#issuecomment-688956220
Nice improvement here. I think we should not let the stacktrace stuff be a blocker for this optimization. Out of box, the user should never even see exceptions anyway: the expression stuff all works on doubles. So all those math functions just return NaN/Inf for special cases because that's how java Math functions want to work.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] uschindler commented on a change in pull request #1837: LUCENE-7882: First idea of using Java 15 hidden anonymous classes for Lucene expressions
Posted by GitBox <gi...@apache.org>.
uschindler commented on a change in pull request #1837:
URL: https://github.com/apache/lucene-solr/pull/1837#discussion_r484117905
##########
File path: lucene/expressions/src/java/org/apache/lucene/expressions/js/JavascriptCompiler.java
##########
@@ -85,7 +88,33 @@
}
}
- private static final int CLASSFILE_VERSION = Opcodes.V1_8;
+ /** Method handle to invoke Java 15's way to define hidden classes.
+ * The method handle uses a private lookup of {@link JavascriptCompiler} to define
+ * classes (this ensures we can create classes in our package without extra permissions).
+ * The classes are initialized and have no strong relationship to our classloader.
+ * This ensures they can be unloaded.
+ * Signature of MH: {@code static Lookup defineHiddenClass(byte[] bc)}
+ * The MH is {@code null} if an earlier JDK is used.
+ * @see "https://openjdk.java.net/jeps/371"
+ */
+ private static final MethodHandle MH_defineHiddenClass;
+ static {
+ final Lookup publicLookup = MethodHandles.publicLookup();
+ MethodHandle mh;
+ try {
+ final Object emptyOptions = Array.newInstance(
+ publicLookup.findClass(Lookup.class.getName().concat("$ClassOption")), 0);
+ mh = publicLookup.findVirtual(Lookup.class, "defineHiddenClass",
+ MethodType.methodType(Lookup.class, byte[].class, boolean.class, emptyOptions.getClass()));
+ mh = mh.bindTo(MethodHandles.lookup()); // private lookup of JavascriptCompiler!
+ mh = MethodHandles.insertArguments(mh.asFixedArity(), 1, true, emptyOptions);
+ } catch (ReflectiveOperationException e) {
+ mh = null;
+ }
+ MH_defineHiddenClass = mh;
+ }
+
+ private static final int CLASSFILE_VERSION = Opcodes.V11;
Review comment:
Actually this should have been done on master already! We are on Java 11, so classfile format should be Java 11, too.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] uschindler commented on a change in pull request #1837: LUCENE-7882: First idea of using Java 15 hidden anonymous classes for Lucene expressions
Posted by GitBox <gi...@apache.org>.
uschindler commented on a change in pull request #1837:
URL: https://github.com/apache/lucene-solr/pull/1837#discussion_r484222322
##########
File path: lucene/expressions/src/test/org/apache/lucene/expressions/js/TestCustomFunctions.java
##########
@@ -263,7 +263,7 @@ public void testThrowingException() throws Exception {
PrintWriter pw = new PrintWriter(sw);
Review comment:
Yeah, I added this only for my personal debugging, because this test fails with my change (as the expression is hidden from the stack trace, so the "contains" does not work. But when it fails, you don't see what should be there.
Assertj is really better as it automatically prints and also shortens the printout to only relevant part.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] uschindler commented on a change in pull request #1837: LUCENE-7882: First idea of using Java 15 hidden anonymous classes for Lucene expressions
Posted by GitBox <gi...@apache.org>.
uschindler commented on a change in pull request #1837:
URL: https://github.com/apache/lucene-solr/pull/1837#discussion_r484229443
##########
File path: lucene/expressions/src/java/org/apache/lucene/expressions/js/JavascriptCompiler.java
##########
@@ -191,9 +220,21 @@ private Expression compileExpression(ClassLoader parent) throws ParseException {
try {
generateClass(getAntlrParseTree(), classWriter, externalsMap);
-
- final Class<? extends Expression> evaluatorClass = new Loader(parent)
- .define(COMPILED_EXPRESSION_CLASS, classWriter.toByteArray());
+
+ final Class<? extends Expression> evaluatorClass;
Review comment:
I was thinking about that, but as it throws a checked Exception it would need a new functional interface. I moved the whole stuff to a separate method `defineClass()`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] mikemccand commented on pull request #1837: LUCENE-7882: First idea of using Java 15 hidden anonymous classes for Lucene expressions
Posted by GitBox <gi...@apache.org>.
mikemccand commented on pull request #1837:
URL: https://github.com/apache/lucene-solr/pull/1837#issuecomment-689070197
Whoa, thanks @uschindler -- this looks awesome.
> @mikemccand can you test this with JDK 15 (release candidate) and your test. You should not see any locks anymore, speed should be higher, and the created anonymous classes should be unloaded very fast.
We (Amazon customer facing product search team) make heavy use of Lucene's (powerful!) expressions module -- I will try to figure out a way to test this (we are on JDK 11 now for production).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] uschindler commented on a change in pull request #1837: LUCENE-7882: First idea of using Java 15 hidden anonymous classes for Lucene expressions
Posted by GitBox <gi...@apache.org>.
uschindler commented on a change in pull request #1837:
URL: https://github.com/apache/lucene-solr/pull/1837#discussion_r484223373
##########
File path: lucene/expressions/src/java/org/apache/lucene/expressions/js/JavascriptCompiler.java
##########
@@ -206,6 +247,12 @@ private Expression compileExpression(ClassLoader parent) throws ParseException {
throw new IllegalStateException("An internal error occurred attempting to compile the expression (" + sourceText + ").", exception);
}
}
+
Review comment:
I changed it. The way how to do this is correct as it only intends to work around a problem of type inference in javac. For a methodhandle we know for sure, which exceptions are thrown (it's different to core reflection, where everything is wrapped in InvocationTargetException).
I moved the whole thing to a separate method and I catched and rethrow all "known" Exceptions (IllegalAccessException, RuntimeException, Error), all others can't happen, so it wraps with AssertionError.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] dweiss commented on a change in pull request #1837: LUCENE-7882: First idea of using Java 15 hidden anonymous classes for Lucene expressions
Posted by GitBox <gi...@apache.org>.
dweiss commented on a change in pull request #1837:
URL: https://github.com/apache/lucene-solr/pull/1837#discussion_r484194383
##########
File path: lucene/expressions/src/test/org/apache/lucene/expressions/js/TestCustomFunctions.java
##########
@@ -263,7 +263,7 @@ public void testThrowingException() throws Exception {
PrintWriter pw = new PrintWriter(sw);
Review comment:
Perhaps we should import assertj for tests. These assertions are so much cleaner with assertj. Don't know whether hamcrest equivalent exist (maybe it does).
##########
File path: lucene/expressions/src/java/org/apache/lucene/expressions/js/JavascriptCompiler.java
##########
@@ -206,6 +247,12 @@ private Expression compileExpression(ClassLoader parent) throws ParseException {
throw new IllegalStateException("An internal error occurred attempting to compile the expression (" + sourceText + ").", exception);
}
}
+
Review comment:
I wouldn't do this, to be honest. Rethrow Error and RuntimeException subclasses as they were, wrap anything else in a runtime exception with an appropriate caused-by delegate. Makes it easier to reason about the code should something happen.
##########
File path: lucene/expressions/src/java/org/apache/lucene/expressions/js/JavascriptCompiler.java
##########
@@ -191,9 +220,21 @@ private Expression compileExpression(ClassLoader parent) throws ParseException {
try {
generateClass(getAntlrParseTree(), classWriter, externalsMap);
-
- final Class<? extends Expression> evaluatorClass = new Loader(parent)
- .define(COMPILED_EXPRESSION_CLASS, classWriter.toByteArray());
+
+ final Class<? extends Expression> evaluatorClass;
Review comment:
Just a suggestion. My personal preference is to set up a supplier (or a function) statically, depending on conditions, then just use that later forever. In this case you could have something like:
````
static Function<byte[], Class<? extends Expression>> classLoader;
static {
if (MH_...) {
classLoader = (bytes) -> {};
} else {
classLoader = (bytes) -> {};
}
}
````
which would be defined once and used without the knowledge of its internal implementation details later on, forever. Just a thought.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org