Skip to content

Commit 880d0ba

Browse files
authored
Rewrite JavaScriptCompiler to use modern JVM features (Java 17) (apache#12873)
* Rewrite Javascript expression compiler to use hidden classes and MethodHandles for functions * Use dynamic constants for MethodHandles * Remove invokestatic code and handle everything through dynamic constants * Rewrite code to patch stack trace (keep Expressions class unmodified) * Improve generating of constant names * Remove classloader test (no longer needed) * Add benchmark * use better exception in benchmark * Add documentation, migration guide and a utility method to convert legacy function maps * also ignore SecurityException here while checking compatibility (if it happens only an imprecise error message is thrown) * Use Map.copyOf to not clone the map each time we compile an expression * Add another test with same method multiple times * Update ASM to 9.6 and set classfile version to Java 17 * Cleanup classloader permissions, unfortunately "createClassLoader" is still needed for Jacoco for God knows what
1 parent e852dfe commit 880d0ba

File tree

23 files changed

+513
-329
lines changed

23 files changed

+513
-329
lines changed

gradle/testing/randomization/policies/replicator-tests.policy

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,8 +23,6 @@ grant {
2323
// jetty-specific:
2424
permission java.lang.RuntimePermission "getenv.JETTY_AVAILABLE_PROCESSORS";
2525
permission java.lang.RuntimePermission "getenv.JETTY_WORKER_INSTANCE";
26-
// servlet stuff
27-
permission java.lang.RuntimePermission "setContextClassLoader";
2826
// allow TestNRTReplication fork its jvm
2927
permission java.io.FilePermission "${java.home}${/}-", "read,execute";
3028
// read/write access to all system properties (required by jetty in these tests)

gradle/testing/randomization/policies/tests.policy

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -50,14 +50,11 @@ grant {
5050
permission java.lang.RuntimePermission "getStackTrace";
5151
// needed for mock filesystems in tests
5252
permission java.lang.RuntimePermission "fileSystemProvider";
53-
// analyzers/uima: needed by lucene expressions' JavascriptCompiler
54-
permission java.lang.RuntimePermission "createClassLoader";
5553
// needed to test unmap hack on platforms that support it
5654
permission java.lang.RuntimePermission "accessClassInPackage.sun.misc";
5755
permission java.lang.reflect.ReflectPermission "suppressAccessChecks";
5856
// needed by cyberneko usage by benchmarks on J9
5957
permission java.lang.RuntimePermission "accessClassInPackage.org.apache.xerces.util";
60-
permission java.lang.RuntimePermission "getClassLoader";
6158

6259
// Needed for loading native library (lucene:misc:native) in lucene:misc
6360
permission java.lang.RuntimePermission "getFileStoreAttributes";
@@ -111,6 +108,8 @@ grant {
111108
permission java.lang.RuntimePermission "shutdownHooks";
112109
// needed by jacoco to instrument classes
113110
permission java.lang.RuntimePermission "defineClass";
111+
// needed by jacoco for God knows what.
112+
permission java.lang.RuntimePermission "createClassLoader";
114113
};
115114

116115
// Grant all permissions to Gradle test runner classes.

gradle/validation/jar-checks.gradle

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,9 @@ allprojects {
5959
}
6060

6161
subprojects {
62+
// initialize empty, because no checks for benchmark-jmh module.
63+
ext.jarInfos = []
64+
6265
// Configure jarValidation configuration for all projects. Any dependency
6366
// declared on this configuration (or any configuration it extends from) will
6467
// be verified.

lucene/CHANGES.txt

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,9 @@ API Changes
6767

6868
* GITHUB#11023: Adding -level param to CheckIndex, making the old -fast param the default behaviour. (Jakub Slowinski)
6969

70+
* GITHUB#12873: Expressions module now uses MethodHandles to define custom functions. Support for
71+
custom classloaders was removed. (Uwe Schindler)
72+
7073
New Features
7174
---------------------
7275

@@ -89,6 +92,9 @@ Improvements
8992

9093
* GITHUB#12447: Hunspell: speed up the dictionary enumeration on suggestion (Peter Gromov)
9194

95+
* GITHUB#12873: Expressions module now uses JEP 371 "Hidden Classes" with JEP 309
96+
"Dynamic Class-File Constants" to implement Javascript expressions. (Uwe Schindler)
97+
9298
Optimizations
9399
---------------------
94100

lucene/MIGRATE.md

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -108,6 +108,20 @@ Added a new parameter: `-level X`, to set the detail level of the index check. T
108108
Sample `-level` usage: `1` (Default) - Checksum checks only, `2` - all level 1 checks as well as logical integrity checks, `3` - all
109109
level 2 checks as well as slow checks.
110110

111+
### Expressions module now uses `MethodHandle` and hidden classes (GITHUB#12873)
112+
113+
Custom functions in the expressions module must now be passed in a `Map` using `MethodHandle` as values.
114+
To convert legacy code using maps of reflective `java.lang.reflect.Method`, use the converter method
115+
`JavascriptCompiler#convertLegacyFunctions`. This should make the mapping mostly compatible.
116+
The use of `MethodHandle` and [Dynamic Class-File Constants (JEP 309)](https://openjdk.org/jeps/309)
117+
now also allows to pass private methods or methods from different classloaders. It is also possible
118+
to adapt guards or filters using the `MethodHandles` class.
119+
120+
The new implementation of the Javascript expressions compiler no longer supports use of custom
121+
`ClassLoader`, because it uses the new JDK 15 feature [hidden classes (JEP 371)](https://openjdk.org/jeps/371).
122+
Due to the use of `MethodHandle`, classloader isolation is no longer needed, because JS code can only call
123+
MHs that were resolved by the application before using the expressions module.
124+
111125
## Migration from Lucene 9.0 to Lucene 9.1
112126

113127
### Test framework package migration and module (LUCENE-10301)

lucene/benchmark-jmh/build.gradle

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ description = 'Lucene JMH micro-benchmarking module'
2323

2424
dependencies {
2525
moduleImplementation project(':lucene:core')
26+
moduleImplementation project(':lucene:expressions')
2627

2728
moduleImplementation "org.openjdk.jmh:jmh-core:1.37"
2829
annotationProcessor "org.openjdk.jmh:jmh-generator-annprocess:1.37"
@@ -42,7 +43,7 @@ tasks.matching { it.name == "forbiddenApisMain" }.configureEach {
4243
tasks.matching { it.name in [
4344
// Turn off JMH dependency checksums and licensing (it's GPL w/ classpath exception
4445
// but this seems fine for test/build only tools).
45-
"validateJarChecksums", "validateJarLicenses",
46+
"validateJarChecksums", "validateJarLicenses", "collectJarInfos",
4647
// No special javadocs for JMH benchmarks.
4748
"renderSiteJavadoc",
4849
"renderJavadoc",

lucene/benchmark-jmh/src/java/module-info.java

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
requires jmh.core;
2121
requires jdk.unsupported;
2222
requires org.apache.lucene.core;
23+
requires org.apache.lucene.expressions;
2324

2425
exports org.apache.lucene.benchmark.jmh;
2526
exports org.apache.lucene.benchmark.jmh.jmh_generated;
Lines changed: 153 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,153 @@
1+
/*
2+
* Licensed to the Apache Software Foundation (ASF) under one or more
3+
* contributor license agreements. See the NOTICE file distributed with
4+
* this work for additional information regarding copyright ownership.
5+
* The ASF licenses this file to You under the Apache License, Version 2.0
6+
* (the "License"); you may not use this file except in compliance with
7+
* the License. You may obtain a copy of the License at
8+
*
9+
* http://www.apache.org/licenses/LICENSE-2.0
10+
*
11+
* Unless required by applicable law or agreed to in writing, software
12+
* distributed under the License is distributed on an "AS IS" BASIS,
13+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
14+
* See the License for the specific language governing permissions and
15+
* limitations under the License.
16+
*/
17+
package org.apache.lucene.benchmark.jmh;
18+
19+
import java.io.IOException;
20+
import java.io.UncheckedIOException;
21+
import java.lang.invoke.MethodHandle;
22+
import java.lang.invoke.MethodHandles;
23+
import java.lang.invoke.MethodType;
24+
import java.text.ParseException;
25+
import java.util.HashMap;
26+
import java.util.Map;
27+
import java.util.Objects;
28+
import java.util.concurrent.ThreadLocalRandom;
29+
import java.util.concurrent.TimeUnit;
30+
import org.apache.lucene.expressions.Expression;
31+
import org.apache.lucene.expressions.js.JavascriptCompiler;
32+
import org.apache.lucene.search.DoubleValues;
33+
import org.openjdk.jmh.annotations.Benchmark;
34+
import org.openjdk.jmh.annotations.BenchmarkMode;
35+
import org.openjdk.jmh.annotations.Fork;
36+
import org.openjdk.jmh.annotations.Level;
37+
import org.openjdk.jmh.annotations.Measurement;
38+
import org.openjdk.jmh.annotations.Mode;
39+
import org.openjdk.jmh.annotations.OutputTimeUnit;
40+
import org.openjdk.jmh.annotations.Param;
41+
import org.openjdk.jmh.annotations.Scope;
42+
import org.openjdk.jmh.annotations.Setup;
43+
import org.openjdk.jmh.annotations.State;
44+
import org.openjdk.jmh.annotations.Warmup;
45+
46+
@BenchmarkMode(Mode.Throughput)
47+
@OutputTimeUnit(TimeUnit.MILLISECONDS)
48+
@State(Scope.Benchmark)
49+
@Warmup(iterations = 5, time = 5)
50+
@Measurement(iterations = 12, time = 8)
51+
@Fork(value = 1)
52+
public class ExpressionsBenchmark {
53+
54+
/**
55+
* Some extra functions to bench "identity" in various variants, another one is named
56+
* "native_identity" (see below).
57+
*/
58+
private static final Map<String, MethodHandle> FUNCTIONS = getFunctions();
59+
60+
private static final String NATIVE_IDENTITY_NAME = "native_identity";
61+
62+
private static Map<String, MethodHandle> getFunctions() {
63+
try {
64+
var lookup = MethodHandles.lookup();
65+
Map<String, MethodHandle> m = new HashMap<>(JavascriptCompiler.DEFAULT_FUNCTIONS);
66+
m.put(
67+
"func_identity",
68+
lookup.findStatic(
69+
lookup.lookupClass(), "ident", MethodType.methodType(double.class, double.class)));
70+
m.put("mh_identity", MethodHandles.identity(double.class));
71+
return m;
72+
} catch (ReflectiveOperationException e) {
73+
throw new AssertionError(e);
74+
}
75+
}
76+
77+
@SuppressWarnings("unused")
78+
private static double ident(double v) {
79+
return v;
80+
}
81+
82+
/** A native implementation of an expression to compare performance */
83+
private static final Expression NATIVE_IDENTITY_EXPRESSION =
84+
new Expression(NATIVE_IDENTITY_NAME, new String[] {"x"}) {
85+
@Override
86+
public double evaluate(DoubleValues[] functionValues) {
87+
try {
88+
return functionValues[0].doubleValue();
89+
} catch (IOException e) {
90+
throw new UncheckedIOException(e);
91+
}
92+
}
93+
};
94+
95+
private double[] randomData;
96+
private Expression expression;
97+
98+
@Param({"x", "func_identity(x)", "mh_identity", "native_identity", "cos(x)", "cos(x) + sin(x)"})
99+
String js;
100+
101+
@Setup(Level.Iteration)
102+
public void init() throws ParseException {
103+
ThreadLocalRandom random = ThreadLocalRandom.current();
104+
randomData = random.doubles().limit(1024).toArray();
105+
expression =
106+
Objects.equals(js, NATIVE_IDENTITY_NAME)
107+
? NATIVE_IDENTITY_EXPRESSION
108+
: JavascriptCompiler.compile(js, FUNCTIONS);
109+
}
110+
111+
@Benchmark
112+
public double expression() throws IOException {
113+
var it = new ValuesIterator(randomData);
114+
var values = it.getDoubleValues();
115+
double result = 0d;
116+
while (it.next()) {
117+
result += expression.evaluate(values);
118+
}
119+
return result;
120+
}
121+
122+
static final class ValuesIterator {
123+
final double[] data;
124+
final DoubleValues[] dv;
125+
int pos = -1;
126+
127+
ValuesIterator(double[] data) {
128+
this.data = data;
129+
var dv =
130+
new DoubleValues() {
131+
@Override
132+
public double doubleValue() throws IOException {
133+
return data[pos];
134+
}
135+
136+
@Override
137+
public boolean advanceExact(int doc) throws IOException {
138+
throw new UnsupportedOperationException();
139+
}
140+
};
141+
this.dv = new DoubleValues[] {dv};
142+
}
143+
144+
boolean next() {
145+
pos++;
146+
return (pos < data.length);
147+
}
148+
149+
DoubleValues[] getDoubleValues() {
150+
return dv;
151+
}
152+
}
153+
}

0 commit comments

Comments
 (0)