-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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.
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
8295302: Do not use ArrayList when LambdaForm has a single ClassData #10706
8295302: Do not use ArrayList when LambdaForm has a single ClassData #10706
Conversation
👋 Welcome back iklam! A progress list of the required criteria for merging this PR into |
Webrevs
|
Class<?> invokerClass = LOOKUP.makeHiddenClassDefiner(className, classFile, Set.of()) | ||
.defineClass(true, classDataValues()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just put it in classDataValues()
instead?
Object classDataValues() {
final List<ClassData> cd = classData;
return switch(cd.size()) {
case 0 -> null; // flatten for zero case
case 1 -> cd.get(0).value; // flatten for single value case
case 2 -> List.of(cd.get(0).value, cd.get(1).value);
...
And it also covers zero case for free.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fixed it as you suggested. I also changed the method to private to make sure no one else is using this method.
emitIconstInsn(mv, index++); | ||
mv.visitMethodInsn(Opcodes.INVOKEINTERFACE, "java/util/List", | ||
"get", "(I)Ljava/lang/Object;", true); | ||
if (classData.size() == 1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (classData.size() < 2) {
It works for zero case, because checkcast
always succeeds on nulls.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually the zero case is already handled here:
static void clinit(ClassWriter cw, String className, List<ClassData> classData) {
if (classData.isEmpty())
return;
No <clinit>
method is generated because we have no static field to initialize.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
@iklam 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:
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 no new commits pushed to the ➡️ To integrate this PR with the above commit message to the |
final List<ClassData> cd = classData; | ||
return switch(cd.size()) { | ||
case 0 -> List.of(); | ||
case 1 -> List.of(cd.get(0).value); | ||
case 0 -> null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
List.of()
always returns the same singleton instance and does not cause any object allocation. I prefer to keep the classDataValues()
to return List<Object>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I now see why you have to change the signature because of the single element case. I made my comment too quickly.
A couple of suggestions:
Please add a javadoc for this method to describe what this method returns for different cases. It's better to move this method close to clinit
as they are tightly coupled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved the method and added in javadoc. Could you check if the wording is OK?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, assuming there are tests calling in with no class data to check that defineClass
is fine with null
inputs?
Yes, we have two existing test cases that has no class data:
I found them by adding this to
If I remove the above hack but keep my patch, the tests passed. So I think this case is covered. |
@@ -360,6 +342,29 @@ private void methodEpilogue() { | |||
mv.visitEnd(); | |||
} | |||
|
|||
/** | |||
* Returns an object to pass this.classData to the <clinit> method of the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about:
* Returns an object to pass this.classData to the <clinit> method of the | |
* Returns the class data object that will be passed to `Lookup.defineHiddenClass`. | |
* The classData is loaded in the <clinit> method of the generated class. | |
* If the class data contains only one single object, this method returns that single object. | |
* If the class data contains more than one objects, this method returns a List. | |
* | |
* This method returns null if no class data. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, the classData is passed here:
private MemberName loadMethod(byte[] classFile) {
Class<?> invokerClass = LOOKUP.makeHiddenClassDefiner(className, classFile, Set.of())
.defineClass(true, classDataValues());
return resolveInvokerMember(invokerClass, invokerName, invokerType);
}
So it doesn't go through Lookup.defineHiddenClass
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, it's an internal version that defines a hidden class, equivalent to calling Lookup::defineHiddenClassWithClassData
(typo in my suggested wording), but it will bypass the security permission check and also returns the Class instead of Lookup (saving not to allocate a Lookup object).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update. I made the suggested wording. You can push once it's updated.
*/ | ||
private Object classDataValues() { | ||
final List<ClassData> cd = classData; | ||
return switch(cd.size()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return switch(cd.size()) { | |
return switch (cd.size()) { |
Thanks @cl4es @iwanowww @mlchung for the review, and @turbanoff for the suggestion. |
Going to push as commit 8d4c077.
Your commit was automatically rebased without conflicts. |
Please review this small optimization. As shown in the JBS issue, most of the generated LambdaForm classes have a single ClassData, so we can get a small footprint/speed improvement.
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/10706/head:pull/10706
$ git checkout pull/10706
Update a local copy of the PR:
$ git checkout pull/10706
$ git pull https://git.openjdk.org/jdk pull/10706/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 10706
View PR using the GUI difftool:
$ git pr show -t 10706
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/10706.diff