oracle / graal

GraalVM compiles Java applications into native executables that start instantly, scale fast, and use fewer compute resources 🚀
https://www.graalvm.org
Other
20.44k stars 1.64k forks source link

[GR-37191] Record equals/hashCode use reflection #4348

Closed TheMode closed 7 months ago

TheMode commented 2 years ago

Describe the issue Record equals (and I assume hashCode, even if untested) currently use reflection, and effectively run much slower (about x10-20 in my case compared to my specialized method comparing the fields directly)

Describe GraalVM and your environment:

More details I have noticed the issue due to a hot path in my code being weirdly expensive: https://github.com/Minestom/Minestom/blob/2c0b026e46f271a88cd9824e00f211c25980a7f1/src/main/java/net/minestom/server/listener/PlayerPositionListener.java#L36 https://github.com/Minestom/Minestom/blob/2c0b026e46f271a88cd9824e00f211c25980a7f1/src/main/java/net/minestom/server/entity/Entity.java#L1239 (notice the Pos#equals calls)

Flamegraph obtained with perf: flamegraph

TheMode commented 2 years ago

Is there any plan to "fix" it? It does make native-image unsuitable for any jdk16+ project, and seem to be, I assume, easily fixable.

oubidar-Abderrahim commented 2 years ago

Hi, Thank you for reporting this, We will take a look into it and get back to you.

oubidar-Abderrahim commented 2 years ago

Tracked internally on GR--37191

loicottet commented 2 years ago

This is due to our current method handle implementation, which uses reflection internally and has quite poor performance as a result. We are planning on changing this implementation in the near future, which should make this code run faster.

TheMode commented 2 years ago

What about specializing those record methods to do not use method handles? I assume that it would give you more implementation freedom.

loicottet commented 2 years ago

The problem is that these record methods do not simply use method handles, they are method handles. This makes it more difficult to substitute them out, since we would have to generate alternative methods for every record class in the image. We also want to introduce substitutions only when strictly necessary, and will therefore primarily focus on improving the performance of our method handle implementation.

TheMode commented 2 years ago

My issue in this case is that improving the performance of method handles may not be enough, as equals/hashCode are likely to be present in a lot of hot path. I would consider it a bug for my hand-coded methods to perform better.

I can however understand that exceptions to the rule are not always desirable.

loicottet commented 2 years ago

Our goal for this reimplementation would be to use JDK-generated, optimized bytecode method handle invokers when the handle is known at build time, which is the case for record methods. We expect to see a performance similar to a hand-written method in that case.

kkriske commented 1 year ago

What is the current state of this? Did the mentioned rewrite happen or is there possibly an indication of a timeline when it will happen?

loicottet commented 1 year ago

This is still on the to-do list with a fairly high priority, but there is no fixed timeline for this change yet.

kkriske commented 1 year ago

@loicottet This pr https://github.com/oracle/graal/pull/7196 and more specifically the change-log entry below, added in this pr https://github.com/oracle/graal/pull/7262 vaguely mention method handle intrinsification of records: https://github.com/oracle/graal/blob/35634fae3f3eb851ac9a8b72ae755889790b067a/substratevm/CHANGELOG.md?plain=1#L23

Does that address this issue as well?

loicottet commented 1 year ago

@peter-hofer can you confirm this?

christianwimmer commented 10 months ago

The performance problem of equals, hashCode, and toString methods for records will be fixed by #8109.

The plan is to integrate the fix into the upcoming GraalVM for JDK 22, and also backport to GraalVM for JDK 21 in the April CPU release.

luneo7 commented 8 months ago

Can this be backported to release/graal-vm/23.0 (JDK 17) as well?

christianwimmer commented 7 months ago

Can this be backported to release/graal-vm/23.0 (JDK 17) as well?

This is not planned, since it would require backporting a lot of feature code into JDK 17.

kristofdho commented 7 months ago

and also backport to GraalVM for JDK 21 in the April CPU

@christianwimmer I'm thorought confused, there still is no 21.0.3 available, and it's not on the release calendar either. Is the release still in the works but delayed? I can't find any information on this. I know this is not really the place to ask but I've tried asking this on slack as well but I've not been able to reach anyone.

luneo7 commented 7 months ago

There are LibericaNIK builds with the fix https://github.com/bell-sw/LibericaNIK/releases/tag/23.1.3%2B1-21.0.3%2B10

christianwimmer commented 7 months ago

All the current GraalVM for JDK 21 downloads on graalvm.org are for the latest CPU release, so downloading GraalVM for JDK 21 gives you the CPU release 21.0.3.

kkriske commented 7 months ago

Those are Oracle GraalVM downloads which we cannot use due to the license. The latest CE release is 21.0.2 which does not contain this fix: https://github.com/graalvm/graalvm-ce-builds/releases/ So is the community edition for GraalVM for JDK 21 already unmaintained?

christianwimmer commented 7 months ago

Those are Oracle GraalVM downloads which we cannot use due to the license.

Oracle GraalVM 21.0.3 is free to use, for all commercial purposes.

kkriske commented 7 months ago

That's what Oracle keeps repeating, but unless the license changed, it's not allowed to distibute binaries to paying customers, ergo not usable for us.

Oracle grants to You, as a recipient of this Program, subject to the conditions stated herein, a nonexclusive, nontransferable, limited license to: [...] (b) redistribute the unmodified Program and Program Documentation, under the terms of this License, provided that You do not charge Your licensees any fees associated with such distribution or use of the Program, including, without limitation, fees for products that include or are bundled with a copy of the Program or for services that involve the use of the distributed Program.

https://www.oracle.com/downloads/licenses/graal-free-license.html

luneo7 commented 7 months ago

Yup, that's what we've read from the Graal free license as well... if we use it... as we do charge a "service fee" for our stuff in production, if we use Oracle GraalVM we would be breaking license... GraalVM CE should be publishing versions though... as there is no new LTS version out there...

luneo7 commented 7 months ago

LibericaNIK in the other hand right now is free to use.... it is mainly GraalVM CE + ParallelGC (which is seems to be not production ready), and sometimes some backport... it is the default native image for Spring now... BellSW charges for support (seems like RedHat), not for usage of their native image built version...