On Mon, 7 Oct 2024 08:44:16 GMT, Stefan Karlsson <stef...@openjdk.org> wrote:
>> Roman Kennke has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains 76 commits: >> >> - Merge remote-tracking branch 'rkennke/JDK-8305895-v4' into JDK-8305895-v4 >> - Revert "Disable TestSplitPacks::test4a, failing on aarch64" >> >> This reverts commit 059b1573db26d1d2902ca6dadc8413f445234c2a. >> - Simplify object init code in interpreter >> - Disable some vectorization tests that fail with +UCOH and UseSSE<=3 >> - Fix for CDSPluginTest.java >> - Merge tag 'jdk-24+18' into JDK-8305895-v4 >> >> Added tag jdk-24+18 for changeset 19642bd3 >> - Disable TestSplitPacks::test4a, failing on aarch64 >> - @robcasloz review comments >> - Improve CollectedHeap::is_oop() >> - Allow LM_MONITOR on 32-bit platforms >> - ... and 66 more: https://git.openjdk.org/jdk/compare/19642bd3...8742f3c1 > > src/hotspot/share/oops/markWord.cpp line 35: > >> 33: STATIC_ASSERT(markWord::klass_shift + markWord::klass_bits == 64); >> 34: // The hash (preceding nKlass) shall be a direct neighbor but not >> interleave >> 35: STATIC_ASSERT(markWord::klass_shift == markWord::hash_bits + >> markWord::hash_shift); > > The code is not consistent in it usage of the name for the klass bits. Here > it says `nKlass` in the comment, but the fields are named `klass`. Maybe just > change the comment to says `(preceding klass bits)`. > > Note that the term `nklass` is not prevalent in the code base, but with this > patch its starting to get a foot hold. It might be good to figure out what we > do want to call these in field names and variables to at least a little bit > more consistency in the code base. Currently we have `nklass`, `nKlass` `nk`, > `narrow_klass`. > > In other places we have functions that are named `set_narrow_klass`, but the > field is called `nklass` and other places call it `nk`. It would be good to > stay consistent with the naming. FWIW, nklass has very little precedence in > the code, so cleaning that away might be easiest.thing is to clean out all > usages of nklass, because it isn't a I renamed all occurrences of nklass and nKlass in shared code to something more useful. I left load_nklass* stuff in aarch64 and x86 code alone for now. Let me know if that addresses your concern. https://github.com/openjdk/jdk/pull/20677/commits/1ab207746e4c4baaa6da162d7c1535c75342fa2e ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1790270819
