RFR: 8269736: Optimize CDS PatchEmbeddedPointers::do_bit()

[Ashutosh Mehra]

On Tue, 25 Oct 2022 20:56:10 GMT, Matias Saavedra Silva <matsaave at openjdk.org> wrote:

> The current implementation reads the narrow oop, decodes it using the "dump time" encoding, encodes it using "runtime" encoding, and then stores it back into the heap. In the cases where the dumptime and runtime shifts are equal, there is an opportunity to calculate the fixed distance between oops so they can be patched more optimally. Verified with tier 1, 2, and 4 tests.

src/hotspot/share/cds/archiveHeapLoader.cpp line 81:

> 79: }
> 80: 
> 81: void ArchiveHeapLoader::init_narrow_oop_decoding(address dumptime_base, int shift) {

AFAIU the first parameter is not really a dumptime base. It has different meaning depending on whether it is called for loaded or mapped heap regions. For loaded regions, it is dumptime base address, but for mapped regions, it represents the runtime base to use for decoding. So I would probably refrain from renaming it.

src/hotspot/share/cds/archiveHeapLoader.cpp line 135:

> 133:     assert(!CompressedOops::is_null(v), "null oops should have been filtered out at dump time");
> 134:     narrowOop new_v;
> 135:     new_v = static_cast<narrowOop>(static_cast<uint32_t>(v) + _delta);

I think it is better to use existing APIs for casting to and from `narrowOop`. This statement can be rewritten as:
`narrowOop new_v = CompressedOops::narrow_oop_cast(CompressedOops::narrow_oop_value(v) + _delta)`

src/hotspot/share/cds/archiveHeapLoader.cpp line 174:

> 172:     uint32_t quick_delta = (uint32_t)rt_encoded_bottom - (uint32_t)dt_encoded_bottom;
> 173: 
> 174:     log_info(cds)("patching heap embedded pointers: narrowOop 0x%8x -> 0x%8x",

I think it makes sense to display this message whenever patching is required. Right now this message is missing in the else block where PatchCompressedEmbeddedPointers is used. Also it can be avoided when quick_delta==0.

src/hotspot/share/cds/archiveHeapLoader.cpp line 185:

> 183:     }
> 184:   } else {
> 185:     log_info(cds)("CDS heap data relocation not possible");

This message can be confusing because we can do the relocation, just that it wouldn't be as simple as adding a delta.

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

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