On Thu, 25 Jul 2024 16:05:49 GMT, Andrew Haley <a...@openjdk.org> wrote:
>> This patch expands the use of a hash table for secondary superclasses >> to the interpreter, C1, and runtime. It also adds a C2 implementation >> of hashed lookup in cases where the superclass isn't known at compile >> time. >> >> HotSpot shared runtime >> ---------------------- >> >> Building hashed secondary tables is now unconditional. It takes very >> little time, and now that the shared runtime always has the tables, it >> might as well take advantage of them. The shared code is easier to >> follow now, I think. >> >> There might be a performance issue with x86-64 in that we build >> HotSpot for a default x86-64 target that does not support popcount. >> This means that HotSpot C++ runtime on x86 always uses a software >> emulation for popcount, even though the vast majority of machines made >> for the past 20 years can do popcount in a single instruction. It >> wouldn't be terribly hard to do something about that. >> >> Having said that, the software popcount is really not bad. >> >> x86 >> --- >> >> x86 is rather tricky, because we still support >> `-XX:-UseSecondarySupersTable` and `-XX:+UseSecondarySupersCache`, as >> well as 32- and 64-bit ports. There's some further complication in >> that only `RCX` can be used as a shift count, so there's some register >> shuffling to do. All of this makes the logic in macroAssembler_x86.cpp >> rather gnarly, with multiple levels of conditionals at compile time >> and runtime. >> >> AArch64 >> ------- >> >> AArch64 is considerably more straightforward. We always have a >> popcount instruction and (thankfully) no 32-bit code to worry about. >> >> Generally >> --------- >> >> I would dearly love simply to rip out the "old" secondary supers cache >> support, but I've left it in just in case someone has a performance >> regression. >> >> The versions of `MacroAssembler::lookup_secondary_supers_table` that >> work with variable superclasses don't take a fixed set of temp >> registers, and neither do they call out to to a slow path subroutine. >> Instead, the slow patch is expanded inline. >> >> I don't think this is necessarily bad. Apart from the very rare cases >> where C2 can't determine the superclass to search for at compile time, >> this code is only used for generating stubs, and it seemed to me >> ridiculous to have stubs calling other stubs. >> >> I've followed the guidance from @iwanowww not to obsess too much about >> the performance of C1-compiled secondary supers lookups, and to prefer >> simplicity over absolute performance. Nonetheless, this i... > > Andrew Haley has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 40 commits: > > - Merge branch 'clean' into JDK-8331658-work > - Cleanup > - temp > - Merge branch 'JDK-8331658-work' of https://github.com/theRealAph/jdk into > JDK-8331658-work > - Review comments > - Review comments > - Review comments > - Review comments > - Review feedback > - Review feedback > - ... and 30 more: https://git.openjdk.org/jdk/compare/34ee06f5...248f44dc src/hotspot/share/oops/klass.inline.hpp line 82: > 80: // subtype check: true if is_subclass_of, or if k is interface and > receiver implements it > 81: inline bool Klass::is_subtype_of(Klass* k) const { > 82: guarantee(secondary_supers() != nullptr, "must be"); Minor point: considering libjvm contains hundreds of copies, does it make sense to turn it into an assert instead? For example, on AArch64 the check costs 36 bytes [1] in product build. [1] libjvm.dylib[0x1306d4] <+28>: ldr x9, [x8, #0x28] libjvm.dylib[0x1306d8] <+32>: cbz x9, 0x1307dc ; <+292> [inlined] Klass::is_subtype_of(Klass*) const at klass.inline.hpp:82:3 ... libjvm.dylib[0x1307dc] <+292>: adrp x0, 3200 libjvm.dylib[0x1307e0] <+296>: add x0, x0, #0x663 ; "open/src/hotspot/share/oops/klass.inline.hpp" libjvm.dylib[0x1307e4] <+300>: adrp x2, 3200 libjvm.dylib[0x1307e8] <+304>: add x2, x2, #0x690 ; "guarantee(secondary_supers() != nullptr) failed" libjvm.dylib[0x1307ec] <+308>: adrp x3, 3200 libjvm.dylib[0x1307f0] <+312>: add x3, x3, #0x6c0 ; "must be" libjvm.dylib[0x1307f4] <+316>: mov w1, #0x52 libjvm.dylib[0x1307f8] <+320>: bl 0x54f870 ; report_vm_error at debug.cpp:181 ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/19989#discussion_r1692288943
