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 > are now done, and I've taken a pass through markWord.hpp to follow 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. src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp line 527: > 525: // Try to unlock. Transition lock bits 0b00 => 0b01 > 526: movptr(reg_rax, mark); > 527: andptr(reg_rax, ~(int32_t)markWord::lock_mask_in_place); Upstream is using the wrong constant. (Both have the same value, so there's no functional bug here) src/hotspot/share/oops/markWord.hpp line 50: > 48: // klass:22 hash:31 valhalla:4 age:4 self-fwd:1 lock:2 > 49: // > 50: // - hash - contains the identity hash value: largest value is 31 bits, > see Sections below have been moved around to follow the order that we have in the bits above. It might be better to move it the other way around because that's how the constants are listed. And then I can add klass bits as well. src/hotspot/share/oops/markWord.hpp line 145: > 143: static_assert((bits) == 1 NOT_LP64(|| (bits) == 0)); \ > 144: return mask_in_place((bits), (shift)); \ > 145: }() I want to be crystal clear when we're talking about one bit. So, `bit_in_place` is used when we have only one bit, and `mask_in_place` is used when have a consecutive set of bits. This disambiguates usage sites, IMHO. src/hotspot/share/oops/markWord.hpp line 151: > 149: static const uintptr_t age_mask_in_place = age_mask << > age_shift; > 150: static const uintptr_t valhalla_reserved_mask = > right_n_bits(valhalla_reserved_bits); > 151: static const uintptr_t valhalla_reserved_mask_in_place = > right_n_bits(valhalla_reserved_mask) << valhalla_reserved_shift; This was actually wrong. It should have been `valhalla_reserved_mask << valhalla_reserved_shift`. There's no functional bug because the result is the same. src/hotspot/share/oops/markWord.hpp line 365: > 363: > 364: // Recover address of oop from encoded form used in mark > 365: inline void* decode_pointer() const { return > (void*)clear_lock_bits().value(); } Restore compared to upstream. ------------- PR Review Comment: https://git.openjdk.org/valhalla/pull/2310#discussion_r3051318602 PR Review Comment: https://git.openjdk.org/valhalla/pull/2310#discussion_r3051362549 PR Review Comment: https://git.openjdk.org/valhalla/pull/2310#discussion_r3051344081 PR Review Comment: https://git.openjdk.org/valhalla/pull/2310#discussion_r3051326309 PR Review Comment: https://git.openjdk.org/valhalla/pull/2310#discussion_r3051320998
