8372268: [lworld] Keep buffer oop on scalarized calls#2062
8372268: [lworld] Keep buffer oop on scalarized calls#2062rwestrel wants to merge 24 commits intoopenjdk:lworldfrom
Conversation
|
👋 Welcome back roland! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
rwestrel
force-pushed
the
JDK-8372268
branch
from
721fd04 to
3d8010e
Compare
|
|
|
/contributor add @galderz |
|
@rwestrel |
TobiHartmann
left a comment
TobiHartmann
left a comment
There was a problem hiding this comment.
Nice work Roland - thanks for taking care of this! Overall, this looks good to me. Could you resolve the merge conflict?
| } | ||
|
|
||
| MemoryMXBean mem = ManagementFactory.getMemoryMXBean(); | ||
| System.out.println("Heap size: " + mem.getHeapMemoryUsage().getUsed() / (1024*1024) + " MB"); |
There was a problem hiding this comment.
Should we assert some max heap size here?
There was a problem hiding this comment.
I wondered about that. Isn't there a risk that the test becomes fragile? What would you pick?
There was a problem hiding this comment.
I think if the difference of the heap size with and without your fix is large, we can just pick a large enough value for the assert that would then trigger without your fix and still be large enough to account for fluctuations.
There was a problem hiding this comment.
To avoid timeouts, I decreased the number of iterations and the test runs with -Xmx20M. I think it reports 2MB being used with my patch. So if memory usage increases for some reason, it's very likely to cause an OOME. Do you still think an assert is needed?
| // Insert InlineTypeNode::NullMarker field right after T_METADATA delimiter | ||
| _sig_cc->insert_before(last+1, SigEntry(T_BOOLEAN, -1, nullptr, true)); | ||
| _sig_cc_ro->insert_before(last_ro+1, SigEntry(T_BOOLEAN, -1, nullptr, true)); | ||
| if (bt == T_OBJECT) { |
Co-authored-by: Tobias Hartmann <tobias.hartmann@oracle.com>
|
@rwestrel this pull request can not be integrated into git checkout JDK-8372268
git fetch https://git.openjdk.org/valhalla.git lworld
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge lworld"
git push |
openjdk
bot
added
the
merge-conflict
The merge conflict must come from the backout of 8377882. I actually would need to make some tweaks to this one now that JDK-8377882 is no longer there and then once the redo of JDK-8377882 integrates, I would have to tweak it again. So I would rather wait until the redo is integrated to merge. |
|
Sounds good! |
openjdk
bot
removed
the
merge-conflict
2 participants
This implements c2 and runtime support to pass a reference to an existing buffer, if there's one, as part of the scalarized calling convention. The buffer reference and the null marker are both always passed at this point.
Progress
Issue
Contributors
<galder@openjdk.org>Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/valhalla.git pull/2062/head:pull/2062$ git checkout pull/2062Update a local copy of the PR:
$ git checkout pull/2062$ git pull https://git.openjdk.org/valhalla.git pull/2062/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 2062View PR using the GUI difftool:
$ git pr show -t 2062Using diff file
Download this PR as a diff file:
https://git.openjdk.org/valhalla/pull/2062.diff
Using Webrev
Link to Webrev Comment