8370325: G1: Disallow GC for TLAB allocation#4210
8370325: G1: Disallow GC for TLAB allocation#4210GoeLin wants to merge 3 commits intoopenjdk:masterfrom
Conversation
|
👋 Welcome back goetz! A progress list of the required criteria for merging this PR into |
|
@GoeLin This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 30 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
GoeLin
force-pushed
the
goetz_backport_8370325
branch
from
84889b4 to
937ad19
Compare
GoeLin
changed the title
openjdk
Bot
changed the title
|
This backport pull request has now been updated with issue from the original commit. |
openjdk
Bot
added
the
backport
|
@GoeLin This pull request has been inactive for more than 8 weeks and will be automatically closed if another 8 weeks passes without any activity. To avoid this, simply issue a |
GoeLin
force-pushed
the
goetz_backport_8370325
branch
from
937ad19 to
38f9629
Compare
Webrevs
|
| result = _allocator->attempt_allocation_using_new_region(word_size); | ||
| if (result != NULL) { | ||
| return result; | ||
| } |
There was a problem hiding this comment.
Should check allow_gc here and return nullptr if not allowed.
There was a problem hiding this comment.
Thanks for moving the allow_gc before the preventive_collection_required() check. I think that's the clearest solution that prevents gc if not allowed.
| log_trace(gc, alloc)("%s: Successfully scheduled collection returning " PTR_FORMAT, | ||
| Thread::current()->name(), p2i(result)); | ||
| return result; | ||
| } else if (!allow_gc) { |
There was a problem hiding this comment.
This check for allow_gc is misplaced. It's too late because gc is done already at L449. The check should be at L422 above.
There was a problem hiding this comment.
Hi @reinrich, thanks for catching this bad flaw! I pushed a change that should fix this.
|
Hi @reinrich, I fixed my fix as you proposed offline. Thanks! |
reinrich
left a comment
reinrich
left a comment
There was a problem hiding this comment.
Looks good to me.
Cheers, Richard.
| result = _allocator->attempt_allocation_using_new_region(word_size); | ||
| if (result != NULL) { | ||
| return result; | ||
| } |
There was a problem hiding this comment.
Thanks for moving the allow_gc before the preventive_collection_required() check. I think that's the clearest solution that prevents gc if not allowed.
|
|
openjdk
Bot
added
approval
2 participants
I backport this for parity with 17.0.19-oracle.
Based on the push to 25. (which is equal to 21).
Simple resolves needed.
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk17u-dev.git pull/4210/head:pull/4210$ git checkout pull/4210Update a local copy of the PR:
$ git checkout pull/4210$ git pull https://git.openjdk.org/jdk17u-dev.git pull/4210/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 4210View PR using the GUI difftool:
$ git pr show -t 4210Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk17u-dev/pull/4210.diff
Using Webrev
Link to Webrev Comment