-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
8319220: Pattern matching switch with a lot of cases is unduly slow #16489
Conversation
if (labels.length == 0) { | ||
cb.constantInstruction(0) | ||
.ireturn(); | ||
} |
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.
Isn't this generating a dead code for labels.length == 0 ?
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.
Right, it does (did). Fixed. Thanks!
cases.add(new Element(target, next, currentLabel)); | ||
switchCases.add(SwitchCase.of(idx, target)); | ||
} | ||
cases = cases.reversed(); |
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.
switchCases for tableswitch do not need to be pre-ordered, code builder does the processign
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 guess I would prefer to keep the reverse here, to reduce the cognitive load, as cases
must be processed in original order (and hence reversed here), and it may not be clear why reverse one, and not the other. (Given the List
is an ArrayList
, the reverse operation should not have much impact on anything, if I read how it is implemented correctly.)
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.
src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java
Outdated
Show resolved
Hide resolved
src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java
Outdated
Show resolved
Hide resolved
src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java
Outdated
Show resolved
Hide resolved
src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java
Outdated
Show resolved
Hide resolved
src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java
Outdated
Show resolved
Hide resolved
👋 Welcome back jlahoda! A progress list of the required criteria for merging this PR into |
@lahodaj 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 63 new commits pushed to the
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 |
Webrevs
|
} | ||
cb.iload(1); | ||
Label dflt = cb.newLabel(); | ||
record Element(Label target, Label next, Object label) {} |
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.
'label' is not a Label, is there a better name to make the difference between the switch label and the bytecode label
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.
Changed to caseLabel
.
Element element = cases.get(idx); | ||
Label next = element.next(); | ||
cb.labelBinding(element.target()); | ||
if (element.label() instanceof Class<?> classLabel) { |
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.
It's too bad we can not use a switch on the label here instead of a bunch of instanceof :)
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.
Well, we could, if we tweaked javac so that it would produce a different/simplified code for java.base for pattern switches (i.e. hardcode an if-else cascade). Which we may or may not do at some point.
src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java
Outdated
Show resolved
Hide resolved
Here is a test that uses a hidden class that works with the current implementation. public class SwitchBootstrapTest {
public static void main(String[] args) throws Throwable {
var className = SwitchBootstrapTest.class.getName();
byte[] data;
try(var input = SwitchBootstrapTest.class.getResourceAsStream('/' + className.replace('.', '/') + ".class")) {
data = input.readAllBytes();
}
var lookup = MethodHandles.lookup().defineHiddenClass(data, true, ClassOption.NESTMATE, ClassOption.STRONG);
var hiddenClass = lookup.lookupClass();
var constructor = lookup.findConstructor(hiddenClass, methodType(void.class));
var instance = constructor.invoke();
var methodType = methodType(int.class, Object.class, int.class);
var callSite = SwitchBootstraps.typeSwitch(lookup, "", methodType, hiddenClass, Object.class);
var index = (int) callSite.getTarget().invokeExact(instance, 0);
System.out.println("index " + index);
}
} |
Thanks for all the comments so far - I think I've either reflected them, or wrote a comment to each of them. Please let me know if there's something else, or if I've forgotten something. |
@@ -375,125 +379,128 @@ private static final class EnumMap { | |||
@SuppressWarnings("removal") | |||
private static MethodHandle generateInnerClass(MethodHandles.Lookup caller, Object[] labels) { | |||
List<EnumDesc<?>> enumDescs = new ArrayList<>(); | |||
List<Class<?>> extraClassLabels = new ArrayList<>(); | |||
|
|||
byte[] classBytes = Classfile.of().build(ClassDesc.of(typeSwitchClassName(caller.lookupClass())), clb -> { | |||
clb.withFlags(AccessFlag.FINAL, AccessFlag.SYNTHETIC) |
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.
AccessFlag.SUPER
is missing, this will make this class a value class in the Valhalla repo
src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java
Outdated
Show resolved
Hide resolved
src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java
Outdated
Show resolved
Hide resolved
src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java
Outdated
Show resolved
Hide resolved
You idea to use an extra array is clever. Using an immutable List instead of an array should allow the VM to constant fold the Class.isInstance (see above). |
@@ -71,7 +72,7 @@ private SwitchBootstraps() {} | |||
private static final MethodHandle MAPPED_ENUM_LOOKUP; | |||
|
|||
private static final MethodTypeDesc typesSwitchDescriptor = |
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.
Given that it's a static final, the name should be in uppercase, TYPES_SWITCH_DESCRIPTOR
Looks good to me :) |
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.
lgtm, very interesting!
/integrate |
Going to push as commit 0c9a61c.
Your commit was automatically rebased without conflicts. |
Consider code like:
javac will compile the switch into a switch whose selector is an indy invocation to
SwitchBootstraps.typeSwitch
, with static arguments being the types in the cases.SwitchBootstraps.typeSwitch
will then create a chain ofMethodHandle
s performinginstanceof
checks between the switch's selector and the given case type. The problem is that when the number of cases is high enough, (more than ~40-50), the chain gets too long, and the tests won't inline anymore. This then leads to a very bad performance, when compared to manually written if-instanceof-else-if-instanceof- chain.The proposal herein is to use bytecode (written using the ClassFile API/library) instead of the
MethodHandle
s chain. The overall performance of this seems to be similar to the manually written if-instanceof-else-if-instanceof- chain.Using the benchmark from the bug, and this patch, I am getting:
The most tricky part of this new way to generate the tests is handling of non-type case labels, and in particular cases with enum constant labels. The resolution of enum constants is deferred as much as possible, by using an indirection through the
ResolvedEnumLabels
.Further improvements may be possible, esp. for some specific cases (like all cases having a type, and the type being a final class).
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/16489/head:pull/16489
$ git checkout pull/16489
Update a local copy of the PR:
$ git checkout pull/16489
$ git pull https://git.openjdk.org/jdk.git pull/16489/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 16489
View PR using the GUI difftool:
$ git pr show -t 16489
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/16489.diff
Webrev
Link to Webrev Comment