RFR: 8343789: Move mutable nmethod data out of CodeCache [v9]

[Vladimir Kozlov]

On Fri, 24 Jan 2025 20:37:32 GMT, Boris Ulasevich <bulasevich at openjdk.org> wrote:

>> This change relocates mutable data (such as relocations, oops, and metadata) from the nmethod. The change follows the recent PR #18984, which relocated immutable nmethod data from the CodeCache.
>> 
>> The core idea remains the same: use the CodeCache for executable code while moving additional data to the C heap. The primary motivations are improving security and enhancing code density.
>> 
>> Although performance is not the main focus, testing on AArch64 CPUs, where code density plays a significant role, has shown a 1–2% performance improvement in specific scenarios, such as the CodeCacheStress test and the Renaissance Dotty benchmark.
>> 
>> The numbers. Immutable data constitutes **~30%** on the nmehtod. Mutable data constitutes **~8%** of nmethod. Example (statistics collected on the CodeCacheStress benchmark):
>> - nmethod_count:134000, total_compilation_time: 510460ms
>> - total allocation time malloc_mutable/malloc_immutable/CodeCache_alloc: 62ms/114ms/6333ms,
>> - total allocation size (mutable/immutable/nmentod): 64MB/192MB/488MB
>> 
>> Functional testing: jtreg on arm/aarch/x86.
>> Performance testing: renaissance/dacapo/SPECjvm2008 benchmarks.
>> 
>> Alternative solution (see comments): In the future, relocations can be moved to _immutable_data.
>
> Boris Ulasevich has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Force the use of movk in combination with adrp and ldr instructions to address scenarios
>   where os::malloc allocates buffers beyond the typical ±4GB range accessible with adrp

Good work. Here are my comments. And I did not look on AArch64 changes.

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

> 70:   size += align_up(cb->total_content_size(), oopSize);
> 71:   size += align_up(cb->total_oop_size(), oopSize);
> 72:   size += align_up(cb->total_metadata_size(), oopSize);

Please add assert that this is `nmethod` or `cb->total_oop_size()` + `cb->total_metadata_size()` is 0.
To make sure we have them only in `nmethod`.

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

> 86:   _caller_must_gc_arguments(caller_must_gc_arguments),
> 87:   _mutable_data(nullptr),
> 88:   _mutable_data_size(mutable_data_size)

Heave to be moved up for my suggestion in `codeBlob.hpp`

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

> 100: 
> 101:   if (_mutable_data_size > 0) {
> 102:     _mutable_data = (address)os::malloc(_mutable_data_size, mtCode);

Missing `os::free` in `CodeBlob::purge()`. Should be moved from nmethod.

src/hotspot/share/code/codeBlob.hpp line 177:

> 175:   address mutable_data_begin() const          { return _mutable_data; }
> 176:   address mutable_data_end() const            { return _mutable_data + _mutable_data_size; }
> 177:   int mutable_data_size() const               { return _mutable_data_size; }

May be move down after `blob_end()`. And `relocation_*()` after it, to follow new layout.

src/hotspot/share/code/codeBlob.hpp line 181:

> 179:   address    content_end() const              { return (address)    header_begin() + _data_offset; }
> 180:   address    code_begin() const               { return (address)    header_begin() + _code_offset; }
> 181:   // code_end == content_end is true for all types of blobs for now, it is also checked in the constructor

Leave this comment.

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

> 1078: #endif
> 1079: 
> 1080: static int required_mutable_data_space(CodeBuffer* code_buffer,

`space` -> `size`

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

> 2146:   if (_mutable_data != blob_end()) {
> 2147:     os::free(_mutable_data);
> 2148:     _mutable_data = blob_end(); // Valid not null address

This should be done in `CodeBlob::purge();`

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

> 3094:                                              p2i(mutable_data_begin()),
> 3095:                                              p2i(mutable_data_end()),
> 3096:                                              immutable_data_size());

`immutable_data_size()` is incorrect here.

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

> 3140:                                              p2i(mutable_data_begin()),
> 3141:                                              p2i(mutable_data_end()),
> 3142:                                              mutable_data_size());

Duplicated output. Remove.

src/hotspot/share/code/nmethod.hpp line 139:

> 137: //  - header                 (the nmethod structure)
> 138: //  [Relocation]
> 139: //  - relocation information

Whole this comment have to be re-written to reflect new layout and additional data sections outside CodeCache.

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

PR Review: https://git.openjdk.org/jdk/pull/21276#pullrequestreview-2610156853
PR Review Comment: https://git.openjdk.org/jdk/pull/21276#discussion_r1951711125
PR Review Comment: https://git.openjdk.org/jdk/pull/21276#discussion_r1951695961
PR Review Comment: https://git.openjdk.org/jdk/pull/21276#discussion_r1951697949
PR Review Comment: https://git.openjdk.org/jdk/pull/21276#discussion_r1951688083
PR Review Comment: https://git.openjdk.org/jdk/pull/21276#discussion_r1951692337
PR Review Comment: https://git.openjdk.org/jdk/pull/21276#discussion_r1951713931
PR Review Comment: https://git.openjdk.org/jdk/pull/21276#discussion_r1951720695
PR Review Comment: https://git.openjdk.org/jdk/pull/21276#discussion_r1951723676
PR Review Comment: https://git.openjdk.org/jdk/pull/21276#discussion_r1951726173
PR Review Comment: https://git.openjdk.org/jdk/pull/21276#discussion_r1951727564