Optimize VisitorState#getConstantExpression #4586

Stephan202 · 2024-09-21T10:40:35Z

By avoiding a second pass over the string.

Stephan202

I filed this PR as a result of work in PicnicSupermarket/error-prone-support#1138, where I found out that I overlooked VisitorState#getConstantExpression and instead reinvented the wheel.

Stephan202 · 2024-09-21T10:41:23Z

check_api/src/main/java/com/google/errorprone/VisitorState.java

-        } else {
-          sb.append(c);
-        }
+    if (!(value instanceof CharSequence str)) {


I added a second commit that enables a further optimization by accepting any CharSequence; this can be backed out if desired.

cushon · 2024-09-21T23:00:31Z

check_api/src/main/java/com/google/errorprone/VisitorState.java

+      if (c == '\'') {
+        sb.append('\'');
+      } else {
+        sb.append(Convert.quote(c));


Incidentally I recently learned that Convert.quote does the thing we'd want now, but only for JDK 23+: openjdk/jdk@144a08e

Er, and as a result of that change Convert.quote(char) was replaced with Convert.quote(char, boolean), so this change doesn't work with the latest JDK EA builds.

I'm not sure what the best option is. We could use reflection or MR-JARs to call the method.

Ah, nice catch! I haven't looked at the MR-JAR setup yet, but indeed reflection should work. TBD whether I'll find some time today for a new pass.

cushon · 2024-09-21T23:01:58Z

check_api/src/main/java/com/google/errorprone/VisitorState.java

-        } else {
-          sb.append(c);
-        }
+    if (!(value instanceof CharSequence str)) {


Unfortunately we're not quite ready to start using Java > 11 features in non-test code internally. I can remove the instanceof pattern when importing this, I'm just leaving a note here in case you wonder why it disappears.

cushon

The Convert.quote part will need more work to be compatible with the latest JDK EA builds

By avoiding a second pass over the string. Fixes #4586 FUTURE_COPYBARA_INTEGRATE_REVIEW=#4586 from PicnicSupermarket:sschroevers/optimize-getConstantExpression ff02145 PiperOrigin-RevId: 677291745

Stephan202

Rebased and added a commit with a multi-release JAR approach. Locally this makes the build pass with JDK 25.ea.3-open.

I suspect that changes to .github/workflows/ci.yml are required, but as the GitHub Actions workflow isn't auto-triggered, I'm not sure 👀.

I'll cross-reference this PR in #3756.

Stephan202 · 2024-12-28T22:44:37Z

check_api/pom.xml

+          <execution>
+            <id>java23</id>
+            <goals>
+              <goal>compile</goal>
+            </goals>
+            <configuration>
+              <jdkToolchain>
+                <version>23</version>
+              </jdkToolchain>
+              <compileSourceRoots>
+                <compileSourceRoot>${basedir}/src/main/java23</compileSourceRoot>
+              </compileSourceRoots>
+              <!-- multiReleaseOutput requires setting release -->
+              <outputDirectory>${project.build.outputDirectory}/META-INF/versions/23</outputDirectory>
+            </configuration>
+          </execution>


A few things are of note here:

Without <goal>compile</goal> this execution is skipped. This also means that the java24 execution below is currently a no-op.

This configuration doesn't override the inherited <source>17</source> and <target>17</target> settings, though it could. (If so, we should do the same below.)

I'm not sure each of these executions should use an exactly-matching toolchain JDK version. Perhaps it's better to use the latest JDK whenever possible, so as to minimize the number of JDKs required.

This also means that the java24 execution below is currently a no-op.

Huh, thanks for pointing that out. This is also used with an Bazel-based build that does the multi-release jar stuff differently, so maybe there are no uses (or test coverage) for the maven build and ErrorProneSignatureGenerator that require JDK 24 support.

I'm not sure each of these executions should use an exactly-matching toolchain JDK version. Perhaps it's better to use the latest JDK whenever possible, so as to minimize the number of JDKs required.

The answer might be 'it depends'. For javac API changes, it's probably best to try to use an exact match, but that does require installing extra JDKs for the github actions setup. If the API changes once and is stable on newer versions we'd get away with a newer version, though.

Stephan202 · 2024-12-28T22:46:03Z

check_api/pom.xml

+      <plugin>
+        <groupId>org.apache.maven.plugins</groupId>
+        <artifactId>maven-jar-plugin</artifactId>
+        <configuration>
+          <archive>
+            <manifestEntries>
+              <Multi-Release>true</Multi-Release>
+            </manifestEntries>
+          </archive>
+        </configuration>
+      </plugin>


Also had to add this to make sure that the class file bundled under META-INF/versions/23 is picked up.

Stephan202 · 2024-12-28T22:47:08Z

check_api/src/main/java/com/google/errorprone/ConstantStringExpressions.java

+/**
+ * @see VisitorState#getConstantExpression(Object)
+ */
+final class ConstantStringExpressions {
+  private ConstantStringExpressions() {}


Certainly open to approaches other than a special-purpose static utility class named com.google.errorprone.ConstantStringExpressions.

This seems good for now, thanks.

Stephan202 · 2024-12-28T22:47:16Z

check_api/src/main/java/com/google/errorprone/ConstantStringExpressions.java

+    }
+
+    // Don't escape single-quotes in string literals.
+    CharSequence str = (CharSequence) value;


Dropped the instanceof pattern matching as suggested here.

We're finally able to use Java 21 features here internally, the earlier comment is obsolete.

Stephan202 · 2024-12-28T22:49:04Z

check_api/src/main/java23/com/google/errorprone/ConstantStringExpressions.java

+    return state
+        .getElements()
+        .getConstantExpression(
+            value instanceof CharSequence charSequence ? charSequence.toString() : value);


Here I did keep the instanceof pattern matching, because this code anyway only applies to JDK 23+, but if that anyway impacts the Google-internal setup, that can be changed, of course.

cushon · 2025-01-02T16:52:29Z

check_api/src/main/java/com/google/errorprone/ConstantStringExpressions.java

+    }
+
+    // Don't escape single-quotes in string literals.
+    CharSequence str = (CharSequence) value;


We're finally able to use Java 21 features here internally, the earlier comment is obsolete.

cushon · 2025-01-02T16:53:13Z

check_api/src/main/java/com/google/errorprone/ConstantStringExpressions.java

+/**
+ * @see VisitorState#getConstantExpression(Object)
+ */
+final class ConstantStringExpressions {
+  private ConstantStringExpressions() {}


This seems good for now, thanks.

cushon · 2025-01-02T16:58:37Z

check_api/pom.xml

+          <execution>
+            <id>java23</id>
+            <goals>
+              <goal>compile</goal>
+            </goals>
+            <configuration>
+              <jdkToolchain>
+                <version>23</version>
+              </jdkToolchain>
+              <compileSourceRoots>
+                <compileSourceRoot>${basedir}/src/main/java23</compileSourceRoot>
+              </compileSourceRoots>
+              <!-- multiReleaseOutput requires setting release -->
+              <outputDirectory>${project.build.outputDirectory}/META-INF/versions/23</outputDirectory>
+            </configuration>
+          </execution>


This also means that the java24 execution below is currently a no-op.

Huh, thanks for pointing that out. This is also used with an Bazel-based build that does the multi-release jar stuff differently, so maybe there are no uses (or test coverage) for the maven build and ErrorProneSignatureGenerator that require JDK 24 support.

I'm not sure each of these executions should use an exactly-matching toolchain JDK version. Perhaps it's better to use the latest JDK whenever possible, so as to minimize the number of JDKs required.

The answer might be 'it depends'. For javac API changes, it's probably best to try to use an exact match, but that does require installing extra JDKs for the github actions setup. If the API changes once and is stable on newer versions we'd get away with a newer version, though.

By avoiding a second pass over the string. Fixes #4586 FUTURE_COPYBARA_INTEGRATE_REVIEW=#4586 from PicnicSupermarket:sschroevers/optimize-getConstantExpression 45f3401 PiperOrigin-RevId: 711440462

cushon · 2025-01-03T19:27:09Z

check_api/src/main/java/com/google/errorprone/ConstantStringExpressions.java

+final class ConstantStringExpressions {
+  private ConstantStringExpressions() {}
+
+  static String toConstantStringExpression(Object value, VisitorState state) {


I'd prefer to pass in the javax.lang.model.util.Elements here instead of VisitorState, it's helpful if the multi-release part is more self contained and does depend on other parts of check_api

cushon · 2025-01-03T19:28:59Z

I tried merging this an am seeing failures like the ones in https://github.com/google/error-prone/actions/runs/12602521683/job/35125772504?pr=4744

I'm able to reproduce those locally with:

mvn clean install -DskipTests=true
mvn test -e
...
Caused by: org.apache.maven.surefire.booter.SurefireBooterForkException: There was an error in the forked process
com/google/errorprone/InvalidCommandLineOptionException
java.lang.NoClassDefFoundError: com/google/errorprone/InvalidCommandLineOptionException
	at java.base/java.lang.Class.getDeclaredMethods0(Native Method)
	at java.base/java.lang.Class.privateGetDeclaredMethods(Class.java:3578)
	at java.base/java.lang.Class.getMethodsRecursive(Class.java:3719)
	at java.base/java.lang.Class.getMethod0(Class.java:3705)
	at java.base/java.lang.Class.getMethod(Class.java:2393)
	at org.apache.maven.surefire.api.util.ReflectionUtils.tryGetMethod(ReflectionUtils.java:50)
	at org.apache.maven.surefire.common.junit3.JUnit3TestChecker.isSuiteOnly(JUnit3TestChecker.java:60)
	at org.apache.maven.surefire.common.junit3.JUnit3TestChecker.isValidJUnit3Test(JUnit3TestChecker.java:56)
	at org.apache.maven.surefire.common.junit3.JUnit3TestChecker.accept(JUnit3TestChecker.java:52)
	at org.apache.maven.surefire.common.junit4.JUnit4TestChecker.accept(JUnit4TestChecker.java:48)
	at org.apache.maven.surefire.api.util.DefaultScanResult.applyFilter(DefaultScanResult.java:87)
	at org.apache.maven.surefire.junit4.JUnit4Provider.scanClassPath(JUnit4Provider.java:272)
	at org.apache.maven.surefire.junit4.JUnit4Provider.setTestsToRun(JUnit4Provider.java:174)
	at org.apache.maven.surefire.junit4.JUnit4Provider.invoke(JUnit4Provider.java:132)
	at org.apache.maven.surefire.booter.ForkedBooter.runSuitesInProcess(ForkedBooter.java:385)
	at org.apache.maven.surefire.booter.ForkedBooter.execute(ForkedBooter.java:162)
	at org.apache.maven.surefire.booter.ForkedBooter.run(ForkedBooter.java:507)
	at org.apache.maven.surefire.booter.ForkedBooter.main(ForkedBooter.java:495)
Caused by: java.lang.ClassNotFoundException: com.google.errorprone.InvalidCommandLineOptionException
	at java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:641)
	at java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:188)
	at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:526)
	... 18 more

Any ideas what's going wrong?

By avoiding a second pass over the string.

Stephan202 · 2025-01-04T12:43:57Z

@cushon this one was tricky! First to debug, then to find a (hacky...) workaround. It's a shame that we can't use multiReleaseOutput, as that would likely have avoided these shenanigans. Maybe one of the other watchers of this project knows of a more elegant solution 👀 🤞. I rebased the branch and added a commit; PTAL :)

NB: instead of introducing a new reset-output-directory step I also tried to move down the default-compile step, but that did not cause the order of the compilation steps to change.

Stephan202 · 2025-01-04T20:27:15Z

Added another commit to apply the Elements change.

One issue I just identified: with my change mvn clean install -DskipTests=true && mvn test -e still doesn't fully work when building with JDK 23+, with test failures due to the wrong Convert.quote being invoked. I think that this is because without reference to the JAR, the META-INF/MANIFEST.MF file is ignored. As-is ./check_api/target/classes/META-INF/MANIFEST.MF also doesn't contain Multi-Release: true, thought the following patch would "fix" that:

diff --git a/check_api/pom.xml b/check_api/pom.xml
index 9e8bc1e8df..a5de90556e 100644
--- a/check_api/pom.xml
+++ b/check_api/pom.xml
@@ -229,17 +229,6 @@
         </executions>
 
       </plugin>
-      <plugin>
-        <groupId>org.apache.maven.plugins</groupId>
-        <artifactId>maven-jar-plugin</artifactId>
-        <configuration>
-          <archive>
-            <manifestEntries>
-              <Multi-Release>true</Multi-Release>
-            </manifestEntries>
-          </archive>
-        </configuration>
-      </plugin>
     </plugins>
   </build>
 
diff --git a/pom.xml b/pom.xml
index ac716fb30f..11238fe292 100644
--- a/pom.xml
+++ b/pom.xml
@@ -147,6 +147,7 @@
             </goals>
             <configuration>
               <bnd><![CDATA[
+                Multi-Release: true
                 Bundle-SymbolicName: com.google.$<replacestring;$<replacestring;${project.artifactId};^error_prone;errorprone>;_;.>
                 Automatic-Module-Name: $<Bundle-SymbolicName>
                 -exportcontents: com.google.errorprone*

... but as said, that doesn't fix mvn clean install -DskipTests=true && mvn test -e on JDK 23+.

A simple fix would be to update the CI configuration to just combine installation and testing:

diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
index 25684fd42f..afcc2c6970 100644
--- a/.github/workflows/ci.yml
+++ b/.github/workflows/ci.yml
@@ -84,12 +84,9 @@ jobs:
           java-version: ${{ matrix.java }}
           distribution: 'zulu'
           cache: 'maven'
-      - name: 'Install'
+      - name: 'Install and test'
         shell: bash
-        run: mvn install -DskipTests=true -Dmaven.javadoc.skip=true -B -V
-      - name: 'Test'
-        shell: bash
-        run: mvn test -B
+        run: mvn install -Dmaven.javadoc.skip=true -B -V
       - name: 'Javadoc'
         shell: bash
         run: mvn -P '!examples' javadoc:javadoc

Right now I don't have other ideas. 🤔

cushon · 2025-01-07T00:50:37Z

I'm fine with changing the invoke in CI invocation, but if separate mvn install and mvn test don't work that seems like it might also affect non-CI development builds? Or would you not expect this to affect development builds?

(I guess another option here would be to give up on multi-release jars for now and just use reflection for this method. But there isn't a good reflection-based alternative alternative for the problem 08e0d10 solves, so it would be nice to make mr-jars work.)

Stephan202 commented Sep 21, 2024

View reviewed changes

Stephan202 mentioned this pull request Sep 21, 2024

Introduce RedundantStringEscape check PicnicSupermarket/error-prone-support#1138

Merged

cushon approved these changes Sep 21, 2024

View reviewed changes

cushon requested changes Sep 21, 2024

View reviewed changes

copybara-service bot mentioned this pull request Sep 21, 2024

Optimize VisitorState#getConstantExpression #4589

Closed

Stephan202 force-pushed the sschroevers/optimize-getConstantExpression branch from ff02145 to 45f3401 Compare December 28, 2024 22:28

Stephan202 commented Dec 28, 2024

View reviewed changes

Stephan202 mentioned this pull request Dec 28, 2024

Consider using Multi-Release jars for Error Prone #3756

Open

cushon approved these changes Jan 2, 2025

View reviewed changes

copybara-service bot mentioned this pull request Jan 3, 2025

Optimize VisitorState#getConstantExpression #4744

Open

cushon reviewed Jan 3, 2025

View reviewed changes

Stephan202 added 4 commits January 4, 2025 12:57

Optimize VisitorState#getConstantExpression

c29a38e

By avoiding a second pass over the string.

Optional additional performance improvement

dec3f87

Support JDK 23+

624fa2d

Proposal to fix the build

07dfafa

Stephan202 force-pushed the sschroevers/optimize-getConstantExpression branch from 45f3401 to 07dfafa Compare January 4, 2025 12:43

Stephan202 added 2 commits January 4, 2025 13:45

Clarify

4f61c1b

Also address the other feedback

10b06e7

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize VisitorState#getConstantExpression #4586

Optimize VisitorState#getConstantExpression #4586

Stephan202 commented Sep 21, 2024

Stephan202 left a comment

Stephan202 Sep 21, 2024

cushon Sep 21, 2024

cushon Sep 21, 2024

Stephan202 Sep 22, 2024

cushon Sep 21, 2024

cushon left a comment

Stephan202 left a comment

Stephan202 Dec 28, 2024

cushon Jan 2, 2025

Stephan202 Dec 28, 2024

Stephan202 Dec 28, 2024

cushon Jan 2, 2025

Stephan202 Dec 28, 2024

cushon Jan 2, 2025

Stephan202 Dec 28, 2024

cushon Jan 2, 2025

cushon Jan 2, 2025

cushon Jan 2, 2025

cushon Jan 3, 2025

cushon commented Jan 3, 2025 •

edited

Loading

Stephan202 commented Jan 4, 2025

Stephan202 commented Jan 4, 2025

cushon commented Jan 7, 2025

Optimize VisitorState#getConstantExpression #4586

Are you sure you want to change the base?

Optimize VisitorState#getConstantExpression #4586

Conversation

Stephan202 commented Sep 21, 2024

Stephan202 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cushon left a comment

Choose a reason for hiding this comment

Stephan202 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cushon commented Jan 3, 2025 • edited Loading

Stephan202 commented Jan 4, 2025

Stephan202 commented Jan 4, 2025

cushon commented Jan 7, 2025

cushon commented Jan 3, 2025 •

edited

Loading