8379503: Intern constant expression of type String by mabbay · Pull Request #978 · openjdk/babylon

mabbay · 2026-03-24T21:42:08Z

Implement constant folding in the model. An Operation that model a constant expression is replaced by a ConstantOp that has the value of the expression.
This transformation eliminate the need to explicitly intern constant expression of type String in the model, as these expressions are replaced by ConstantOps with String literals as values, which are interned naturally.

I confirm that I make this contribution in accordance with the OpenJDK Interim AI Policy.

Progress

Change must not contain extraneous whitespace

Issue

JDK-8379503: Intern constant expression of type String (Enhancement - P4)

Reviewers

Paul Sandoz (@PaulSandoz - Reviewer) ⚠️ Review applies to 247c773c
Maurizio Cimadamore (@mcimadamore - Reviewer) ⚠️ Review applies to 247c773c

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/babylon.git pull/978/head:pull/978
$ git checkout pull/978

Update a local copy of the PR:
$ git checkout pull/978
$ git pull https://git.openjdk.org/babylon.git pull/978/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 978

View PR using the GUI difftool:
$ git pr show -t 978

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/babylon/pull/978.diff

Using Webrev

Link to Webrev Comment

…pls that are not required by op evaluation

# Conflicts: # test/jdk/jdk/incubator/code/TestSwitchExpressionOp.java

# Conflicts: # test/jdk/jdk/incubator/code/lib/ArithmeticAndConvOpImpls.java

bridgekeeper · 2026-03-24T21:43:10Z

👋 Welcome back mabbay! A progress list of the required criteria for merging this PR into code-reflection will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2026-03-24T21:43:15Z

@mabbay This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8379503: Intern constant expression of type String

Reviewed-by: psandoz, mcimadamore

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 2 new commits pushed to the code-reflection branch:

774621b: [hat] Roll back tensors from the main branch
e12e6dc: Updates to specification and API

Please see this link for an up-to-date comparison between the source branch of this pull request and the code-reflection branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the code-reflection branch, type /integrate in a new comment.

This reverts commit e72bdb3.

This reverts commit 204fcae.

…nal var

…StringConstantExpressionInterning.java

PaulSandoz

The reuse of the evaluator is nice.

Using NonConstantExpression works well internally for evaluating one expression, but could be a performance issue when called many times when used for transformation. We could revisit later, at the expense of more complexity with value | error union result.

mabbay · 2026-04-13T19:26:58Z

The reuse of the evaluator is nice.

Using NonConstantExpression works well internally for evaluating one expression, but could be a performance issue when called many times when used for transformation. We could revisit later, at the expense of more complexity with value | error union result.

I logged an issue for this.

# Conflicts: # src/jdk.incubator.code/share/classes/jdk/incubator/code/dialect/java/JavaOp.java

mabbay · 2026-04-14T13:33:06Z

/template append

openjdk · 2026-04-14T13:34:17Z

@mabbay The pull request template has been appended to the pull request body

mlbridge · 2026-04-23T15:54:07Z

Webrevs

mcimadamore · 2026-04-27T09:35:25Z

+                        }
+                        Object v;
+                        if ((field.getModifiers() & Modifier.STATIC) != 0) {
+                            // @@@ why using field.get fails ?


did you try field::get(null) ?

Yes, but it throws IllegalAccessException

We discussed this offline -- this failure is due to the fact that Field::get uses caller sensitive methods -- so access checks are relative to the place where Field::get is called -- in this case jdk.incubator.code.dialect.java. When using VarHandle however, access checks are defined in terms of a user-provided lookup object -- which makes this work more reliably. I'd suggest dropping the comment and maybe say that we need to use VH here.

mcimadamore · 2026-04-27T09:38:09Z

+                            throw new NonConstantExpression();
+                        }
+                        Object v;
+                        if ((field.getModifiers() & Modifier.STATIC) != 0) {


Can't you check for STATIC in the above -- e.g. when you also check for FINAL ?

The first checks are for constant variables (as per JLS). The second check is due to the limitation we have in the model, as we can't get the value of an instance field.

mcimadamore · 2026-04-27T09:53:22Z

The code seems to track JLS 15.29 quite well. Of course there's some limitations in the code model -- e.g. when it comes to variables, we don't know which variables were declared as "final" in the source code, or which also had an initializer. Maybe javac could leave an extra attribute there for a "constant field/var load" -- but not sure whether doing so would help the general issue.

mabbay · 2026-04-27T13:40:43Z

The code seems to track JLS 15.29 quite well. Of course there's some limitations in the code model -- e.g. when it comes to variables, we don't know which variables were declared as "final" in the source code, or which also had an initializer. Maybe javac could leave an extra attribute there for a "constant field/var load" -- but not sure whether doing so would help the general issue.

We need a general solution to cover user built model

PaulSandoz

Very good.

Let's follow up in another PR with common patterns for constructing and using code transformers.

mabbay · 2026-04-28T15:11:22Z

/integrate

openjdk · 2026-04-28T15:15:27Z

Going to push as commit a7dc835.
Since your change was applied there have been 2 commits pushed to the code-reflection branch:

774621b: [hat] Roll back tensors from the main branch
e12e6dc: Updates to specification and API

Your commit was automatically rebased without conflicts.

openjdk · 2026-04-28T15:15:35Z

@mabbay Pushed as commit a7dc835.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

mabbay added 7 commits March 13, 2026 23:54

Move Interpreter to the location of runtime tests

6fde7bd

Remove methods from jdk.incubator.code.internal.ArithmeticAndConvOpIm…

cf20247

…pls that are not required by op evaluation

Merge branch 'code-reflection' into mv-interpreter

c71e110

# Conflicts: # test/jdk/jdk/incubator/code/TestSwitchExpressionOp.java

Document the issue with interning constant expression of type String

943fa35

Model if a variable is declared final

204fcae

Enhance the check of whether a VarOp models a constant variable

e72bdb3

Merge branch 'code-reflection' into intern-string-constant-expr

3cfd1d8

# Conflicts: # test/jdk/jdk/incubator/code/lib/ArithmeticAndConvOpImpls.java

mabbay requested a review from PaulSandoz March 24, 2026 21:42

mabbay self-assigned this Mar 24, 2026

mabbay changed the title ~~Intern constant expression of type String~~ 8379503: Intern constant expression of type String Mar 24, 2026

mabbay added 11 commits March 25, 2026 19:42

Revert "Enhance the check of whether a VarOp models a constant variable"

b21de78

This reverts commit e72bdb3.

Revert "Model if a variable is declared final"

188e19e

This reverts commit 204fcae.

Broaden the JLS notion of constant variable to include effectively fi…

dfd6325

…nal var

Embed the expected value in the reflect method name

dd21836

Implement constant folding

50d5b82

Encapsulate the code for evaluation in a class

d6b839f

Add a map to class Evaluator for caching the result of evaluation

5796b78

Add test cases

bb86b12

create class ConstantFolder

def145e

Delete TestConstantFolding.java as it contains the same cases as Test…

66f5243

…StringConstantExpressionInterning.java

Use ConstantFolder transformer in BytecodeGenerator and Interpreter

f7b488b

mabbay changed the title ~~8379503: Intern constant expression of type String~~ Implement constant folding in the model Mar 30, 2026

mabbay added 3 commits April 1, 2026 00:21

Forget to commit ConstantFolder class

5465171

Remove constant folding from Interpreter because it causes problems

6cdb7b5

Make RemoveUnusedConstantTransformer internal

e73916c

PaulSandoz reviewed Apr 7, 2026

View reviewed changes

Apply suggestions

6d2ac0b

Merge branch 'code-reflection' into intern-string-constant-expr

fb04923

# Conflicts: # src/jdk.incubator.code/share/classes/jdk/incubator/code/dialect/java/JavaOp.java

mabbay marked this pull request as ready for review April 23, 2026 15:50

openjdk Bot added ready Pull request is ready to be integrated rfr Pull request is ready for review labels Apr 23, 2026

mabbay added 4 commits April 23, 2026 23:20

Merge branch 'code-reflection' into intern-string-constant-expr

60dd65c

Remove empty line

feb5d6d

Derive expected value from the case method

4e74ebb

Add comment

247c773

mabbay changed the title ~~Implement constant folding in the model~~ 8379503: Intern constant expression of type String Apr 23, 2026

mcimadamore reviewed Apr 27, 2026

View reviewed changes

mcimadamore approved these changes Apr 27, 2026

View reviewed changes

PaulSandoz approved these changes Apr 27, 2026

View reviewed changes

Drop comment

e149a14

openjdk Bot added the integrated Pull request has been integrated label Apr 28, 2026

openjdk Bot closed this Apr 28, 2026

openjdk Bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Apr 28, 2026

Conversation

mabbay commented Mar 24, 2026 • edited by openjdk Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper Bot commented Mar 24, 2026

Uh oh!

openjdk Bot commented Mar 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PaulSandoz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mabbay commented Apr 13, 2026

Uh oh!

mabbay commented Apr 14, 2026

Uh oh!

openjdk Bot commented Apr 14, 2026

Uh oh!

mlbridge Bot commented Apr 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

mcimadamore Apr 27, 2026

Choose a reason for hiding this comment

Uh oh!

mabbay Apr 27, 2026

Choose a reason for hiding this comment

Uh oh!

mcimadamore Apr 27, 2026

Choose a reason for hiding this comment

Uh oh!

mcimadamore Apr 27, 2026

Choose a reason for hiding this comment

Uh oh!

mabbay Apr 27, 2026

Choose a reason for hiding this comment

Uh oh!

mcimadamore commented Apr 27, 2026

Uh oh!

mabbay commented Apr 27, 2026

Uh oh!

PaulSandoz left a comment

Choose a reason for hiding this comment

Uh oh!

mabbay commented Apr 28, 2026

Uh oh!

openjdk Bot commented Apr 28, 2026

Uh oh!

openjdk Bot commented Apr 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

mabbay commented Mar 24, 2026 •

edited by openjdk Bot

Loading

openjdk Bot commented Mar 24, 2026 •

edited

Loading

mlbridge Bot commented Apr 23, 2026 •

edited

Loading