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

[Gerard Ziemski]

On Thu, 23 Feb 2023 07:29:30 GMT, Thomas Stuefe <stuefe 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,

Thank you again for a thoughtful and insightful comment.

> 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.

I just like this pattern:


void* ptr = NULL;
ptr = realloc(ptr, size); // at some init time
ptr = realloc(ptr, new_size); // some time later


over this:


void* ptr = NULL;
ptr = malloc(size); // at some init time
ptr = realloc(ptr, new_size); // some time later



Moreover it can collapse to just:


ptr = realloc(ptr, size); // at any time we figure it need resizing


if the code updates the size as needed. Can't get simpler than that.


I've seen the pattern using realloc(NULL) in hotspot VM in a few places and I personally like it better, but it looks like a personal choice, and it also seems like I'm in the minority here, so I will withdraw it.


> 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.

I assumed that what libc does is just an implementation detail, and assumed that the cost of VM_client_malloc(size) -> os::realloc(NULL, size) -> libc_malloc(size) is not an issue.

I would need to run some benchmarks and get real numbers, before being able to comment on this, which at this point seems unnecessary.

Thank you for your feedback!

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

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