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

[gerard-ziemski]

Please review this enhancement, which eliminates a duplicate and almost identical path we have for malloc() and realloc(), and collapses them down to only one codepath, by taking advantage of fact that malloc() == realloc(NULL)

Tested with MACH5 tier1,2,3,4,5

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8301404: Replace os::malloc with os::realloc, so we only have 1 code path

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/12621/head:pull/12621
$ git checkout pull/12621

Update a local copy of the PR:
$ git checkout pull/12621
$ git pull https://git.openjdk.org/jdk pull/12621/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 12621

View PR using the GUI difftool:
$ git pr show -t 12621

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/12621.diff

[bridgekeeper]

👋 Welcome back gziemski! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@gerard-ziemski this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout JDK-8301404
git fetch https://git.openjdk.org/jdk master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

[openjdk]

@gerard-ziemski The following label will be automatically applied to this pull request:

• hotspot-runtime

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[gerard-ziemski]

I need help with this part. I'm not familiar with C++ enough to make my change work while keeping these fields const

Can someone suggest a better approach here, or is it OK to drop the const modifier off?

[tstuefe]

Its a warning sign that something is off. From first glance, a FreeInfo should be immutable; there should be no reason to modify it once its created.

[mlbridge]

Webrevs

• 00: Full (c8df10f9)

[tstuefe]

Not a big fan, tbh. I find your code harder to decipher than the original version. Malloc and realloc are different things, and having different code paths for them makes more sense than mixing them together.

I am currently revising NMTPreInit (see https://github.com/tstuefe/jdk/tree/make-nmt-preinit-cheaper-for-nmt-off) and plan to continue this work after my vacation in march; it would be nice if we could synchronize work then.

[gerard-ziemski]

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.

Malloc and realloc are different things, and having different code paths for them makes more sense than mixing them together.

I'm concerned here, it's totally possible that I missed something and misunderstood the code, but how are they different (aside from the obvious that they are different APIs)? Why would we want them different?

And if we truly want them different, then why are we currently using malloc() to implement realloc(NULL)?

Isn't this enhancement a proof that they need not be different?

I am currently revising NMTPreInit (see https://github.com/tstuefe/jdk/tree/make-nmt-preinit-cheaper-for-nmt-off) and plan to continue this work after my vacation in march; it would be nice if we could synchronize work then.

Absolutely can and will wait.

[tstuefe]

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

[gerard-ziemski]

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!