RFR: 8299125: UnifiedOopRef in JFR leakprofiler should treat thread local oops correctly [v2]

[Axel Boldt-Christmas]

On Tue, 3 Jan 2023 13:32:12 GMT, Markus Grönlund <mgronlun at openjdk.org> wrote:

>> Axel Boldt-Christmas has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Use more suitible nomenclature: raw
>
> src/hotspot/share/jfr/leakprofiler/chains/rootSetClosure.cpp line 51:
> 
>> 49:   assert(ref != NULL, "invariant");
>> 50:   assert(is_aligned(ref, HeapWordSize), "invariant");
>> 51:   if (NativeAccess<>::oop_load(ref) != nullptr) {
> 
> Why is this needed now?

The GC requires that load barriers to access oops in native storage. Currently the raw load works on all GCs as it only checks for null and all GCs encode null as NULL. But it is semantically wrong, and will not work with generational ZGC where there exist colored null which is not encoded as NULL.

> src/hotspot/share/jfr/leakprofiler/utilities/unifiedOopRef.hpp line 34:
> 
>> 32:   static const uintptr_t tag_mask   = LP64_ONLY(0b111) NOT_LP64(0b011);
>> 33:   static const uintptr_t native_tag = 0b001;
>> 34:   static const uintptr_t raw_tag    = 0b010;
> 
> What is "raw", compared to "native"?

Native is native storage (not in java heap) which requires GC barriers. Raw is raw storage which does not require GC barriers. This is thread local storage, such as the handle area, stack, etc.

> src/hotspot/share/jfr/leakprofiler/utilities/unifiedOopRef.inline.hpp line 45:
> 
>> 43: template<>
>> 44: inline uintptr_t UnifiedOopRef::addr<uintptr_t>() const {
>> 45:   return (_value & ~tag_mask) LP64_ONLY(>> 1);
> 
> Why a shift now as well?

I do not remember exactly. But I think there was some ref which on 64 bit which had the third bit set. That is this assert failed:
`assert((reinterpret_cast<uintptr_t>(ref) & UnifiedOopRef::tag_mask) == 0, "Unexpected low-order bits");`
I have to remove the shift and test again. I think it is strange given that `ObjectAlignmentInBytes >= 8` but we added the shift for some reason. Or maybe I am just missing something obvious.

> src/hotspot/share/jfr/leakprofiler/utilities/unifiedOopRef.inline.hpp line 70:
> 
>> 68:   LP64_ONLY(assert(((reinterpret_cast<uintptr_t>(ref)) & (1ull << 63)) == 0, "Unexpected high-order bit"));
>> 69:   UnifiedOopRef result = { (reinterpret_cast<uintptr_t>(ref) LP64_ONLY(<< 1)) | tag };
>> 70:   assert(result.addr<T>() == ref, "sanity");
> 
> This is crazy hard to read.

Yeah. Refactoring out `reinterpret_cast<uintptr_t>(ref)` into a variable might help a bit at least. The assert just want to check that the lowest two bits are not set (the tagging does not destroy information) and that on 64 bit platforms the highest bit is not set (the shift does not destroy information).

Not sure if there is a better way to express those two invariants.

> src/hotspot/share/oops/accessBackend.hpp line 1260:
> 
>> 1258:     }
>> 1259: 
>> 1260:     inline bool operator ==(std::nullptr_t) const {
> 
> why std::nullptr_t?

This change is only here to facilitate being able to compare the OopLoadProxy object directly to nullptr instead of having to create and use an `oop()` / `oop(NULL)`.

-------------

PR: https://git.openjdk.org/jdk/pull/11741