Skip to content

Refactor the expressions compiler to use official ClassData BSM with indexed lookup #14602

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
uschindler merged 2 commits into from
May 1, 2025
Merged

Refactor the expressions compiler to use official ClassData BSM with indexed lookup #14602

uschindler merged 2 commits into from
May 1, 2025

Conversation

@uschindler
Copy link
Contributor

@uschindler uschindler commented May 1, 2025 •
edited
Loading

With more review of the code, I thought that it would be better to use one of the default Bootstrap methods available for ClassData. This allows to get rid of our custom BSM method and just use one of the defaults (MethodHandles#classDataAt()). By default it is possible to supply a List as ClassData and the bootstrap gets the MethodHandle by index. The code for that index was already there and is now reused. Like for the variables, the index is incremented on each new constant (like on each variable) and the index used for lookup.

As this change is simple it can also be backported, in the backport I just need to adapt the ASM 'Handle` to point to the default ClassData bootstrap method.

@uschindler uschindler added this to the 10.3.0 milestone May 1, 2025
@uschindler uschindler self-assigned this May 1, 2025
@uschindler
Copy link
Contributor Author

uschindler commented May 1, 2025 •
edited
Loading

I also added a new test to make sure that using the same function two times works correct.

  public void testSameFunctionTwoTimes() throws Exception {
    Expression expr = compile("sqrt(20)+abs(-7)+sqrt(30)");
    assertEquals(Math.sqrt(20) + 7 + Math.sqrt(30), expr.evaluate(null), DELTA);
  }

I also verified the bytecode, now looks like this:

//  (version 24 : 68.0, no super bit)
public final class org.apache.lucene.expressions.js.JavascriptCompiler$CompiledExpression extends org.apache.lucene.expressions.Expression {
  
  // Method descriptor #6 (Ljava/lang/String;[Ljava/lang/String;)V
  // Stack: 3, Locals: 3
  public JavascriptCompiler$CompiledExpression(java.lang.String arg0, java.lang.String[] arg1);
    0  aload_0 [this]
    1  aload_1 [arg0]
    2  aload_2 [arg1]
    3  invokespecial org.apache.lucene.expressions.Expression(java.lang.String, java.lang.String[]) [8]
    6  return

  
  // Method descriptor #10 ([Lorg/apache/lucene/search/DoubleValues;)D
  // Stack: 5, Locals: 2
  public double evaluate(org.apache.lucene.search.DoubleValues[] arg0) throws java.io.IOException;
     0  ldc <Dynamic> 0 _ java.lang.invoke.MethodHandle [22]
     2  ldc2_w <Double 20.0> [23]
     5  invokevirtual java.lang.invoke.MethodHandle.invokeExact(double) : double [30]
     8  ldc <Dynamic> 1 _ java.lang.invoke.MethodHandle [32]
    10  ldc2_w <Double 7.0> [33]
    13  dneg
    14  invokevirtual java.lang.invoke.MethodHandle.invokeExact(double) : double [30]
    17  dadd
    18  ldc <Dynamic> 0 _ java.lang.invoke.MethodHandle [22]
    20  ldc2_w <Double 30.0> [35]
    23  invokevirtual java.lang.invoke.MethodHandle.invokeExact(double) : double [30]
    26  dadd
    27  dreturn
    28  aload_0 [this]
    29  invokestatic org.apache.lucene.expressions.js.JavascriptCompiler.patchStackTrace(java.lang.Throwable, org.apache.lucene.expressions.Expression) : java.lang.Throwable [44]
    32  athrow
      Exception Table:
        [pc: 0, pc: 28] -> 28 when : java.lang.Throwable
      Stack map table: number of frames 1
        [pc: 28, same_locals_1_stack_item, stack: {java.lang.Throwable}]

Bootstrap methods:
  0 : # 18 invokestatic java/lang/invoke/MethodHandles.classDataAt:(Ljava/lang/invoke/MethodHandles$Lookup;Ljava/lang/String;Ljava/lang/Class;I)Ljava/lang/Object;
	Method arguments:
		#11 0,
  1 : # 18 invokestatic java/lang/invoke/MethodHandles.classDataAt:(Ljava/lang/invoke/MethodHandles$Lookup;Ljava/lang/String;Ljava/lang/Class;I)Ljava/lang/Object;
	Method arguments:
		#31 1
}

As you see the default bootstrap method java/lang/invoke/MethodHandles.classDataAt just gets the index into the classData list.

@uschindler uschindler changed the title Refactors the Expressions compiler to use the official ClassData BSM for indexed lookup Refactor the expressions compiler to use official ClassData BSM with indexed lookup May 1, 2025
@uschindler uschindler mentioned this pull request May 1, 2025
Merged
@uschindler uschindler merged commit 87df2c8 into apache:main May 1, 2025
7 checks passed
asfgit pushed a commit that referenced this pull request May 1, 2025
@uschindler
BACKPORT: Refactor the expressions compiler to use official ClassData…
6fb9dee 
… BSM with indexed lookup (#14602)

# Conflicts:
#	lucene/expressions/src/java/org/apache/lucene/expressions/js/JavascriptCompiler.java
@uschindler
Copy link
Contributor Author

uschindler commented May 1, 2025

The backport is the following commit: 6fb9dee

@uschindler uschindler deleted the dev/expressionsUseDefaultIndexedBSM branch May 1, 2025 18:28
asfgit pushed a commit that referenced this pull request May 1, 2025
@uschindler
Remove duplicate test (#14602)
dae812c
asfgit pushed a commit that referenced this pull request May 1, 2025
@uschindler
Remove duplicate test (#14602)
9cf497e
asfgit pushed a commit that referenced this pull request May 2, 2025
@uschindler
Remove expressions module deprecations and use java 21 apis (followup…
98c690a 
… of #14602)
weizijun added a commit to weizijun/lucene that referenced this pull request May 7, 2025
@weizijun
Merge branch 'main' into reduce-NeighborArray-memory
3cb42c6 
* main: (27 commits)
  deps(java): bump com.github.luben:zstd-jni from 1.5.7-2 to 1.5.7-3 (apache#14621)
  Improve user-facing docs for geo package (apache#14534)
  Enabling histogram collection for PointRangeQuery (apache#14560)
  Move sloppySin into SloppyMath from GeoUtils (apache#14516)
  Rewrite APIJAR extractor to use Java 24 classfile API and kill ASM dependency also for build system (apache#14613)
  CHANGES entry for apache#14226 (optimistic KNN Query)
  OptimisticKnnVectorQuery (apache#14226)
  Fix for Windows (spaces in paths) apache#14608
  Update jdk requirements in README to OpenJDK 24 (apache#14610)
  Always check gradle wrapper sha checksum and download if necessary (apache#14608)
  Fix changelog verifier (apache#14606)
  MultiRange query for SortedNumeric DocValues (apache#14404)
  Remove RANDOM_PRELOAD read advice, which is not actually used (apache#14593)
  Remove duplicate test (apache#14602)
  Refactor the expressions compiler to use official ClassData BSM with indexed lookup (apache#14602)
  Disallow EA versions to run Gradle (apache#14601)
  Add back-compat indices for 10.2.1
  Add Lucene 10.2.1 version constant
  DOAP changes for release 10.2.1
  Revert "An attempt to make jenkins pass with the currently installed jdk24-ea. To be reverted later. apache#14600"
  ...
@uschindler uschindler mentioned this pull request May 9, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@uschindler uschindler

Labels

module:expressions

Projects

None yet

Milestone

10.3.0

Development

Successfully merging this pull request may close these issues.

1 participant

@uschindler