RFR: 8365256: RelocIterator should use indexes instead of pointers

[Johan Sjölen]

On Thu, 31 Jul 2025 06:17:24 GMT, Johan Sjölen <jsjolen at openjdk.org> wrote:

> Hi,
> 
> This PR replaces the `current` and `end` pointers with a `base` pointer alongside a `current` index and a `len`. This allows us to have `-1` as the initial value for current, while retaining `nullptr` as the 'dead' value for `_mutable_data`.
> 
> Performance testing shows no difference/performance improvements on DaCapo Linux x64. I don't think that these are actual improvements, but at least there are no clear regressions.
> 
> Testing: GHA

@bulasevich , @vnkozlov 

I did this small refactoring of `RelocIterator` to get rid of `blob_end()` as a marker of 'dead' `_mutable_data`. What do you think, should I make a JBS ticket for this and put this into 'ready for review'?

The build failures are all from after [Explicitly assign _mutable_data to nullptr](https://github.com/openjdk/jdk/pull/26569/commits/75a3853b65f264666c470a3ba6b1791dce6c775d), fixing the issues should be trivial.

> Is this change intended to resolve JDK-8361382 (NMT header corruption)? If so, please link it in the PR description and describe how the new logic prevents that corruption.

It's not intended to resolve it, but it does remove one potential source of the issue.

>  However, since relocation iteration is on a performance-critical path, benchmarks should be run to ensure that the added integer field and array indexing introduce no measurable regression.

Yeah, we can check that. Note that we have the same size, as we replaced 1 8-byte field with 2 4-byte fields. I also suspect that the pointer addition (probably a `lea r0,  [ r0 + r1 ]` on x64) won't introduce a performance regression, but nothing wrong with checking.

src/hotspot/share/code/codeBlob.cpp line 213:

> 211:   delete _oop_maps;
> 212:   _oop_maps = nullptr;
> 213: 

`free` and `delete` on null pointers are OK, no need to check.

src/hotspot/share/code/nmethod.cpp line 2156:

> 2154:   os::free(_immutable_data);
> 2155:   _immutable_data = nullptr;
> 2156: 

`free` and `delete` on null pointers are OK, no need to check.

src/hotspot/share/code/relocInfo.cpp line 157:

> 155:   _current = 0;
> 156:   set_has_current(true);
> 157: }

This 'singleton' constructor allows us to remove `set_current()`.

src/hotspot/share/code/relocInfo.cpp line 157:

> 155:   _current = -1;
> 156: }
> 157: 

So that we can remove `set_current()`.

src/hotspot/share/code/relocInfo.hpp line 589:

> 587:     assert(has_current(), "must have current");
> 588:     return current_no_check();
> 589:   }

I had to add the `current_no_check()` as the `print_on` method used to read `_current` directly, without the assert check.

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

PR Comment: https://git.openjdk.org/jdk/pull/26569#issuecomment-3138983480
PR Comment: https://git.openjdk.org/jdk/pull/26569#issuecomment-3141038241
PR Review Comment: https://git.openjdk.org/jdk/pull/26569#discussion_r2244666717
PR Review Comment: https://git.openjdk.org/jdk/pull/26569#discussion_r2244667082
PR Review Comment: https://git.openjdk.org/jdk/pull/26569#discussion_r2244672059
PR Review Comment: https://git.openjdk.org/jdk/pull/26569#discussion_r2244674949
PR Review Comment: https://git.openjdk.org/jdk/pull/26569#discussion_r2244676713