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 Thanks! The patch looks good, except there was one failure observed during testing with the latest patch [1]. It does look related to the latest changes you did in [54050a5](https://github.com/openjdk/jdk/pull/19989/commits/54050a5c2c0aa1d6a9e36d0240c66345765845e3) about `bitmap == SECONDARY_SUPERS_BITMAP_FULL` check. [1] # Internal Error (.../src/hotspot/share/oops/array.hpp:126), pid=1225147, tid=1273512 # assert(i >= 0 && i< _length) failed: oob: 0 <= 63 < 63 V [libjvm.so+0x7f7eab] oopDesc::is_a(Klass*) const+0x21b (array.hpp:126) V [libjvm.so+0xe9805e] java_lang_Throwable::fill_in_stack_trace(Handle, methodHandle const&, JavaThread*)+0x158e (javaClasses.cpp:2677) V [libjvm.so+0xe982cb] java_lang_Throwable::fill_in_stack_trace(Handle, methodHandle const&)+0x6b (javaClasses.cpp:2719) V [libjvm.so+0xfe045c] JVM_FillInStackTrace+0x9c (jvm.cpp:515) .... RBX=0x00000000b446bcf0 is a pointer to class: javasoft.sqe.tests.lang.clss029.clss02902.e67 {0x00000000b446bcf0} ... - name: 'javasoft/sqe/tests/lang/clss029/clss02902/e67' - super: 'javasoft/sqe/tests/lang/clss029/clss02902/e66' - sub: 'javasoft/sqe/tests/lang/clss029/clss02902/e68' ... - secondary supers: Array<T>(0x00007faff68b3058) - hash_slot: 39 - secondary bitmap: 0xffffffffffffffff R12=0x00000000b446ba80 is a pointer to class: javasoft.sqe.tests.lang.clss029.clss02902.e66 {0x00000000b446ba80} ... - name: 'javasoft/sqe/tests/lang/clss029/clss02902/e66' - super: 'javasoft/sqe/tests/lang/clss029/clss02902/e65' - sub: 'javasoft/sqe/tests/lang/clss029/clss02902/e67' ... - secondary supers: Array<T>(0x00007faff68b2c90) - hash_slot: 63 - secondary bitmap: 0xfffffffffffefffd Do we miss ------------- PR Comment: https://git.openjdk.org/jdk/pull/19989#issuecomment-2251567973
