8292914: Introduce a system property that enables stable names for lambda proxy classes

[sstanoje]

This PR introduces a system property that creates stable names for the lambda classes in the JDK. Instead of using an atomic counter in the lambda name, we can use a 32-bit hash after $$Lambda$. Thus, the name becomes lambdaCapturingClass$$Lambda$hashValue. Parameters used to create a stable part of the name (hash value) are a superset of the parameters used for lambda class archiving when the CDS dumping option is enabled. During the stable name creation process, all the common parameters are in the same form as in the low-level implementation (C part of the code) of the archiving process. We concatenate all of those parameters in one string hashData. We calculate the long hash value for hashData in the same manner as the java.lang.StringUTF16#hashCode does, and then we hash that value using Long.toString(longHashValue, Character.MAX_RADIX). The desired length for this hash is equal to the length of the Long.toString(Long.MAX_VALUE, Character.MAX_RADIX). Sometimes, the calculated hash value is shorter than the desired length, so we pad it with the character # to hit it. Appending # only affects the hash length, but not its stability.

Link to the related issue: https://bugs.openjdk.org/browse/JDK-8292914

---

Progress

• [ ] Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• [x] Change must not contain extraneous whitespace
• [x] Commit message must refer to an issue

Issue

• JDK-8292914: Introduce a system property that enables stable names for lambda proxy classes

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 10024

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/10024.diff

[bridgekeeper[bot]]

Hi @sstanoje, welcome to this OpenJDK project and thanks for contributing!

We do not recognize you as Contributor and need to ensure you have signed the Oracle Contributor Agreement (OCA). If you have not signed the OCA, please follow the instructions. Please fill in your GitHub username in the "Username" field of the application. Once you have signed the OCA, please let us know by writing /signed in a comment in this pull request.

If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please use "Add GitHub user sstanoje" as summary for the issue.

If you are contributing this work on behalf of your employer and your employer has signed the OCA, please let us know by writing /covered in a comment in this pull request.

[openjdk[bot]]

@sstanoje The following label will be automatically applied to this pull request:

• core-libs

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[sstanoje]

/signed

[bridgekeeper[bot]]

Thank you! Please allow for up to two weeks to process your OCA, although it is usually done within one to two business days. Also, please note that pull requests that are pending an OCA check will not usually be evaluated, so your patience is appreciated!

[mlbridge[bot]]

Webrevs

• 06: Full - Incremental (275e23f2)
• 05: Full - Incremental (f5feb430)
• 04: Full - Incremental (6179915a)
• 03: Full - Incremental (dd8e592d)
• 02: Full - Incremental (6a8f9128)
• 01: Full - Incremental (dd268934)
• 00: Full (1f551d1d)

[iklam]

Have you tested with method references? Two references to the same method will result in a single JVM_CONSTANT_InvokeDynamic constant pool entry in the classfile, but it's invoked by two callsites. As a result, two different lambda proxy classes will be generated, as the JVMS requires the invokedynamic resolution to be per callsite, not per constantpool entry.

public class ShareBSM {
  public static void main(String args[]) {
    doit1(ShareBSM::func);
    doit2(ShareBSM::func);
  }
  static void func() {  Thread.dumpStack();  }
  static void doit1(Runnable r) {  r.run();  }
  static void doit2(Runnable r) {  r.run();  }
}

Here's the output:

$ java -cp . -XX:+UnlockDiagnosticVMOptions -XX:+ShowHiddenFrames ShareBSM
java.lang.Exception: Stack trace
	at java.base/java.lang.Thread.dumpStack(Thread.java:1380)
	at ShareBSM.func(ShareBSM.java:8)
	at ShareBSM$$Lambda$1/0x0000000800c009f0.run(Unknown Source)
	at ShareBSM.doit1(ShareBSM.java:12)
	at ShareBSM.main(ShareBSM.java:3)
java.lang.Exception: Stack trace
	at java.base/java.lang.Thread.dumpStack(Thread.java:1380)
	at ShareBSM.func(ShareBSM.java:8)
	at ShareBSM$$Lambda$2/0x0000000800c00bf8.run(Unknown Source)
	at ShareBSM.doit2(ShareBSM.java:15)
	at ShareBSM.main(ShareBSM.java:4)

Will you patch generate the same name for both callsites? Does this matter for your use case?

[openjdk-notifier[bot]]

@sstanoje Please do not rebase or force-push to an active PR as it invalidates existing review comments. All changes will be squashed into a single commit automatically when integrating. See OpenJDK Developers’ Guide for more information.

[sstanoje]

Have you tested with method references? Two references to the same method will result in a single JVM_CONSTANT_InvokeDynamic constant pool entry in the classfile, but it's invoked by two callsites. As a result, two different lambda proxy classes will be generated, as the JVMS requires the invokedynamic resolution to be per callsite, not per constantpool entry.

public class ShareBSM {
  public static void main(String args[]) {
    doit1(ShareBSM::func);
    doit2(ShareBSM::func);
  }
  static void func() {  Thread.dumpStack();  }
  static void doit1(Runnable r) {  r.run();  }
  static void doit2(Runnable r) {  r.run();  }
}

Here's the output:

$ java -cp . -XX:+UnlockDiagnosticVMOptions -XX:+ShowHiddenFrames ShareBSM
java.lang.Exception: Stack trace
	at java.base/java.lang.Thread.dumpStack(Thread.java:1380)
	at ShareBSM.func(ShareBSM.java:8)
	at ShareBSM$$Lambda$1/0x0000000800c009f0.run(Unknown Source)
	at ShareBSM.doit1(ShareBSM.java:12)
	at ShareBSM.main(ShareBSM.java:3)
java.lang.Exception: Stack trace
	at java.base/java.lang.Thread.dumpStack(Thread.java:1380)
	at ShareBSM.func(ShareBSM.java:8)
	at ShareBSM$$Lambda$2/0x0000000800c00bf8.run(Unknown Source)
	at ShareBSM.doit2(ShareBSM.java:15)
	at ShareBSM.main(ShareBSM.java:4)

Will you patch generate the same name for both callsites? Does this matter for your use case?

@iklam Yes, I tested it against method references too. The patch produces the same names. That is okay with our use-case.

[bridgekeeper[bot]]

@sstanoje This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[sstanoje]

Hi @iklam, is there any additional work I can do on this PR?

[bridgekeeper[bot]]

@sstanoje This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[openjdk-notifier[bot]]

@sstanoje Please do not rebase or force-push to an active PR as it invalidates existing review comments. All changes will be squashed into a single commit automatically when integrating. See OpenJDK Developers’ Guide for more information.

[bridgekeeper[bot]]

@sstanoje This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[sstanoje]

Dummy comment to keep the PR alive.

[bridgekeeper[bot]]

@sstanoje This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[dmlloyd]

Looking at the code for InnerClassLambdaMetafactory, I don't think that the numerical suffix is necessary at all. These lambda classes are defined as hidden classes, in which case the class is already created with a unique "name" (in the form of a suffix comprising / and some unique identifying digits).

From my own experiments, I simply removed the counter altogether and everything continues to work with the classes simply being named xxx/xxx/Xxx$$Lambda. So perhaps the entire mechanism (which consists of + counter.incrementAndGet() within InnerClassLambdaMetafactory#lambdaClassName) is unneeded?

I'll also add this comment on to the original issue as well.

[briangoetz]

David Llloyd's proposed solution is dramatically less intrusive, so it should be preferred over the one here.

[thomaswue]

Having a stable numeric suffix for the lambda class name improves debuggability of the system compared to a solution where all lambdas have the same class name. The stable hash function can probably be simplified given that collisions are less problematic because of #12579.

[dmlloyd]

What about other cases like the JLI MH, DMH, InjectedInvoker, lambda form etc. classes? Or the FFI specialized upcall proxies, or the many cases where users define hidden classes?

Wouldn't it be better to make the suffix of a hidden class be repeatable, thus solving all of these cases at once, rather than chasing down every place where a hidden class is defined?

[bridgekeeper[bot]]

@sstanoje This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[koutheir]

Hi @koutheir, thanks for making a comment in an OpenJDK project!

All comments and discussions in the OpenJDK Community must be made available under the OpenJDK Terms of Use. If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please Use "Add GitHub user koutheir for the summary.

If you are not an OpenJDK Author, Committer or Reviewer, simply check the box below to accept the OpenJDK Terms of Use for your comments.

• [ ] I agree to the OpenJDK Terms of Use for all comments I make in a project in the OpenJDK GitHub organization.

Your comment will be automatically restored once you have accepted the OpenJDK Terms of Use.

[openjdk[bot]]

@sstanoje this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout sstanoje/JDK-8292914/stable-lambda-class-names
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

[bridgekeeper[bot]]

@sstanoje This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[thomaswue]

@briangoetz

[bridgekeeper[bot]]

@sstanoje This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[bridgekeeper[bot]]

@sstanoje This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.