On Wed, 8 Apr 2026 12:31:03 GMT, Stefan Karlsson <[email protected]> wrote:
> @caspernorrbin Started cleaning up markWord.hpp with > https://github.com/openjdk/valhalla/pull/2238, but while reviewing that patch > I realized that it would be good to make a number of simplifications before > proceeding. I talked that through with Casper and @Arraying. Those changes > have now been done, and I've taken a pass through markWord.hpp to updated the > code in the spirit of what Casper did and some of the things we three talked > about. > > I've also made updates and clarifications to the comments. > > One main thing about this proposal is that I want it to be crystal clear when > we talk about bit and when we talk about a mask. To make sure that the bit > constants only ever refers to one bit I've added helpers to verify that this > is the case. Nice work! This definitely improves the readability of the markWord. I'm very favourable of the "pattern_mask" and "pattern" pattern (pun intended), I think that makes the code so much clearer. Some minor comments and nitpicks attached. src/hotspot/share/oops/markWord.hpp line 75: > 73: // * valhalla reserved: Reserved for future use > 74: // > 75: // Inline types cannot be locked and does not have an identity hash. s/does/do/ src/hotspot/share/oops/markWord.hpp line 122: > 120: > 121: // Number of bits > 122: static const int lock_bits = 2; I know we already discussed this offline and the comment is helpful, but as an alternative if we want to be really clear we could opt for `_n_bits`. I don't feel super strongly about this, it's moreso a comment for your consideration. src/hotspot/share/oops/markWord.hpp line 142: > 140: static const int hash_shift = > valhalla_reserved_shift + valhalla_reserved_bits; > 141: > 142: #define mask_in_place(bits, shift) (right_n_bits(bits) << (shift)) Nit: I skimmed the HotSpot style guide and couldn't find anything regarding case, but I was under the impression we used all uppercase for runtime macros. For example, `#define DISABLE_FLAG_AND_WARN_IF_NOT_DEFAULT(flag)`. src/hotspot/share/oops/markWord.hpp line 156: > 154: static const uintptr_t null_free_array_bit_in_place = > bit_in_place(null_free_array_bits, null_free_array_shift); > 155: static const uintptr_t flat_array_bit_in_place = > bit_in_place(flat_array_bits, flat_array_shift); > 156: static const uintptr_t valhalla_reserved_bit_in_place = > bit_in_place(valhalla_reserved_bits, valhalla_reserved_shift); Nit: alignment. ------------- Changes requested by phubner (Committer). PR Review: https://git.openjdk.org/valhalla/pull/2310#pullrequestreview-4082243420 PR Review Comment: https://git.openjdk.org/valhalla/pull/2310#discussion_r3057768866 PR Review Comment: https://git.openjdk.org/valhalla/pull/2310#discussion_r3057826396 PR Review Comment: https://git.openjdk.org/valhalla/pull/2310#discussion_r3057808621 PR Review Comment: https://git.openjdk.org/valhalla/pull/2310#discussion_r3057831329
