-
Notifications
You must be signed in to change notification settings - Fork 5.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
8335409: Can't allocate and retain memory from resource area in frame::oops_interpreted_do oop closure after 8329665 #20012
base: master
Are you sure you want to change the base?
Conversation
👋 Welcome back pchilanomate! A progress list of the required criteria for merging this PR into |
@pchilano 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 45 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 |
Webrevs
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the detailed explanations. A couple of minor (pre-existing) nits but changes are good.
Thanks
"Should not resource allocate the _bit_mask"); | ||
assert(from->has_valid_mask(), | ||
"Cannot copy entry with an invalid mask"); | ||
// The expectation is that this InterpreterOopMap is a recently created |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/is a recently/is recently/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
#ifdef ASSERT | ||
_resource_allocate_bit_mask = true; | ||
_used = false; | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit pre-existing: use of DEBUG_ONLY would be more consistent with later setting of _used
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
@@ -128,11 +129,12 @@ class InterpreterOopMap: ResourceObj { | |||
|
|||
public: | |||
InterpreterOopMap(); | |||
~InterpreterOopMap(); | |||
|
|||
// Copy the OopMapCacheEntry in parameter "from" into this | |||
// InterpreterOopMap. If the _bit_mask[0] in "from" points to | |||
// allocated space (i.e., the bit mask was to large to hold |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit pre-existing: s/to/too/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
// in-line), allocate the space from a Resource area. | ||
// in-line), allocate the space from the C heap. | ||
void resource_copy(OopMapCacheEntry* from); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name resource_copy
seems somewhat of a misnomer given it may be C heap. Is it worth changing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, this should probably be copy_from, and rename the parameter src. Or something like that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also thought about renaming it but ended up leaving it as is in v1. I changed it to Coleen's suggestion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also a couple of nits, but this looks good. Thanks for tracking down the history and verifying that its an unusual situation that we were optimizing for.
// a resource area for better performance. InterpreterOopMap | ||
// For InterpreterOopMap the bit_mask is allocated in the C heap | ||
// to avoid issues with allocations from the resource area that have | ||
// to live accross the oop closure (see 8335409). InterpreterOopMap |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't usually put bug numbers in the code and after this change nobody will want to move this back to resource area, so putting the bug number as a caution shouldn't be needed. If one wants to know the details, they can git blame this file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed.
// a resource area for better performance. InterpreterOopMap | ||
// For InterpreterOopMap the bit_mask is allocated in the C heap | ||
// to avoid issues with allocations from the resource area that have | ||
// to live accross the oop closure (see 8335409). InterpreterOopMap | ||
// should only be created and deleted during same garbage collection. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add 'the' to "during the same garbage collection."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
// in-line), allocate the space from a Resource area. | ||
// in-line), allocate the space from the C heap. | ||
void resource_copy(OopMapCacheEntry* from); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, this should probably be copy_from, and rename the parameter src. Or something like that.
@@ -89,7 +90,7 @@ class InterpreterOopMap: ResourceObj { | |||
|
|||
protected: | |||
#ifdef ASSERT | |||
bool _resource_allocate_bit_mask; | |||
bool _used; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you make this a DEBUG_ONLY() too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One tiny nit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect, thanks!
The ResourceMark added in 8329665 to address the case of having to allocate extra memory for the _bit_mask, prevents code in the closure from allocating and retaining memory from the resource area across the closure, relying on some ResourceMark in scope further up the stack from frame::oops_interpreted_do(). There is in fact one case today in JFR code where this kind of allocation happens.
The amount of locals and expression stack entries a method can have before having to allocate extra memory for the _bit_mask is 4*64/2 = 128. This is already big enough that we almost never have to allocate. A test run through mach5 tiers1-6 shows only a handful of methods that fall into this case, and most are artificial ones created to trigger this condition. So moving the allocation to the C heap shouldn't have any performance penalty as the comment otherwise says. This comment dates back from 2002 where instead of 128 entries we could have only 32, considering 32 bits cpus as still in main use (see bug for more history details).
The current code in InterpreterOopMap::resource_copy() has a comment expecting the InterpreterOopMap object to be recently created and empty, but it also has an assert in the allocation case path where it considers the entry might be in use already. This assert actually looks wrong since a used InterpreterOopMap object will not necessarily contain a pointer to resource area memory in _bit_mask[0]. I added an example case in the bug details. In any case, since we don't have any such cases in the codebase I added an explicit assert to verify each InterpreterOopMap is only used one.
I tested the patch by running it through mach5 tiers 1-6.
Thanks,
Patricio
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/20012/head:pull/20012
$ git checkout pull/20012
Update a local copy of the PR:
$ git checkout pull/20012
$ git pull https://git.openjdk.org/jdk.git pull/20012/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 20012
View PR using the GUI difftool:
$ git pr show -t 20012
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/20012.diff
Webrev
Link to Webrev Comment