RFR: 8301404: Factor out os::malloc with os::realloc common code, so that we only have 1 code path [v8]

[Coleen Phillimore]

On Tue, 1 Apr 2025 12:51:03 GMT, Gerard Ziemski <gziemski at openjdk.org> wrote:

>> This is the 2nd time I am proposing this (controversial?) change, but this time I do have performance numbers, which indicate no change in speed (using NMTBenchmark from #23786):
>> 
>> proposed:
>> 
>> time:72,642,827[ns]
>> [samples:807,804] [NMT headers:382,064]
>> [malloc#:588,703] [realloc#:12,462] [free#:206,639]
>> memory requested:57,274,288 bytes, allocated:69,004,800 bytes
>> malloc overhead:4,853,360 bytes [8.47%], NMT headers overhead:6,877,152 bytes [12.01%]
>> existing code:
>> 
>> time:73,085,446[ns]
>> [samples:807,804] [NMT headers:382,064]
>> [malloc#:588,703] [realloc#:12,462] [free#:206,639]
>> memory requested:57,274,288 bytes, allocated:69,004,800 bytes
>> malloc overhead:4,853,360 bytes [8.47%], NMT headers overhead:6,877,152 bytes [12.01%]
>> Note: the NMTBenchmark reports realloc(nullptr) as mallocs(), which is why both versions show the same count for mallocs/reallocs.
>> 
>> The performance is virtually the same where I sampled each test 30 times and took the best (the shortest).
>> 
>> This proposed change factors out the common code and simplifies both os::malloc and os::realloc. We were able to reduce malloc from 44 lines down to 8 (saving of 36 lines) and realloc from 84 to 55 (29 lines).
>> 
>> To me the most important part here is that we reduce the number of times that NMT has to interact with the native allocation code.
>
> Gerard Ziemski has updated the pull request incrementally with one additional commit since the last revision:
> 
>   revert last checkin - wrong issue

This looks really reasonable.  The new names for the refactoring are good.

Also I think I saw that you ran tier1 on this change.  You should run tier1-4 at least.

src/hotspot/share/nmt/nmtPreInit.hpp line 270:

> 268:   // *rc contains the return address.
> 269:   static bool handle_malloc(void** rc, size_t size) {
> 270:     size = MAX2((size_t)1, size);         // malloc(0)

Not this change, but why are these functions in the hpp file? They seem more appropriate in the cpp file.

src/hotspot/share/runtime/os.cpp line 632:

> 630:   // Special handling for NMT preinit phase before arguments are parsed
> 631:   void* rc = nullptr;
> 632:   if (NMTPreInit::handle_malloc(&rc, size)) {

So it looks like this was the only call to handle_malloc.  Is it now unused since malloc and realloc() call this function?

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

PR Review: https://git.openjdk.org/jdk/pull/24189#pullrequestreview-2732615079
PR Comment: https://git.openjdk.org/jdk/pull/24189#issuecomment-2769294583
PR Review Comment: https://git.openjdk.org/jdk/pull/24189#discussion_r2022814336
PR Review Comment: https://git.openjdk.org/jdk/pull/24189#discussion_r2022811525