RFR: 8301404: Replace os::malloc with os::realloc, so we only have 1 code path

[Thomas Stuefe]

On Mon, 20 Feb 2023 16:15:58 GMT, Gerard Ziemski <gziemski at openjdk.org> wrote:

> > Not a big fan, tbh. I find your code harder to decipher than the original version.
> 
> The current code implements `realloc(NULL)` in terms of `malloc()`, I just turned it around and implemented `malloc()` on top of `realloc(NULL)`, which allowed us to remove an almost identical 2nd path for allocating the memory.
> 
> This resulted in net benefit of being able to remove 58 lines of code. That is 58 lines of code less for anyone who needs to parse the allocation code.
> 
> I have not written even one new line of code here, just rearranged it. It will definitively look less familiar to those who tend to look/work with this area, like you. But for the others, who are new, it will certainly make it less effort to follow it.
> 

Hi Gerard,

Your simplifications are usually good, but here I'm not convinced. Trimming code is just a way to make code easier digestible, not an end in itself. And there is the "principle of least surprise". Implementing realloc(null) in terms of malloc is traditional and expected; the other way around its surprising. With your patch, we now have a realloc that has two jobs, and you have to think "around the corner" for the memblock==nullptr path - witness the new branch conditions you had to add.

Then there is the question of runtime costs. Folks have been obsessively concerned with that; see e.g. JDK-8299196, which cites runtime costs for NMTPreInit as a reason to remove it. But that cost had been a single load+compare (reading from a very sparse lookup table). Here, you add several branches for ==nullptr. And all libcs I know will redirect realloc(nullptr) to malloc anyway [1][2][3], at the cost of yet another compare-branch, so it will exactly revert your decision to call realloc. And remember that malloc is by far the hotter method of the two; now you pay for those comparisons on every os::malloc(). Better to just call malloc right away.

Cheers Thomas


[1] http://bxr.su/FreeBSD/crypto/heimdal/lib/roken/realloc.c
[2] https://sourceware.org/git/?p=glibc.git;a=blob;f=malloc/malloc.c;h=5829f3fa9b4b3690d732d727a947989c9cd4f056;hb=HEAD#l3394
[3] https://github.com/seL4/musllibc/blob/3d6b939e8f05cb1d2a1a8c8166609bf2e652e975/src/malloc/malloc.c#L396

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

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