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
8294370: Fix allocation bug in java_lang_Thread::async_get_stack_trace() #10424
Conversation
👋 Welcome back pchilanomate! A progress list of the required criteria for merging this PR into |
@pchilano
|
/label add serviceability |
@pchilano |
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.
Good find! Looks good! A couple of queries at this stage.
Thanks.
@@ -2000,6 +2001,11 @@ oop java_lang_Thread::async_get_stack_trace(oop java_thread, TRAPS) { | |||
const int max_depth = MaxJavaStackTraceDepth; | |||
const bool skip_hidden = !ShowHiddenFrames; | |||
|
|||
// Pick some initial length |
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 comment should at least hint at there being some reasonable reason for choosing the value that follows. :)
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.
How about: "Pick minimum length that will cover most cases"?
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.
Sounds good.
int init_length = 64; | ||
_methods = new (ResourceObj::C_HEAP, mtInternal) GrowableArray<Method*>(init_length, mtInternal); | ||
_bcis = new (ResourceObj::C_HEAP, mtInternal) GrowableArray<int>(init_length, mtInternal); | ||
|
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.
Couldn't you just do this in the constructor? I'm not clear if there is a subtle reason for needing lazy-init as well as moving to the C_Heap.
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.
Yes, this could have been done in the constructor too. But since there are additional checks in the closure that could fail I move the allocation here to avoid unnecessary allocation/deallocation. The allocation still needs to be done in the C_Heap in case the target executes the handshake. Otherwise if the target allocates the arrays in its resource area they could be deallocated by the time the requester tries to access them after the handshake.
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.
Good point - only allocate when known to be needed.
Thanks for looking at this David. |
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.
Looks good.
Thanks
@@ -2000,6 +2001,11 @@ oop java_lang_Thread::async_get_stack_trace(oop java_thread, TRAPS) { | |||
const int max_depth = MaxJavaStackTraceDepth; | |||
const bool skip_hidden = !ShowHiddenFrames; | |||
|
|||
// Pick some initial length |
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.
Sounds good.
int init_length = 64; | ||
_methods = new (ResourceObj::C_HEAP, mtInternal) GrowableArray<Method*>(init_length, mtInternal); | ||
_bcis = new (ResourceObj::C_HEAP, mtInternal) GrowableArray<int>(init_length, mtInternal); | ||
|
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.
Good point - only allocate when known to be needed.
@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 |
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.
Yes, it is good find!
The fix looks good.
Thanks,
Serguei
Thanks for the reviews David and Serguei! |
/integrate |
Going to push as commit 5d48da4.
Your commit was automatically rebased without conflicts. |
Please review this small fix in async_get_stack_trace(). The GrowableArrays created to store the bci and Method* of each frame found while traversing the stack are allocated in the resource area of the thread that calls async_get_stack_trace(). But if the handshake is executed by the target and if the number of frames in the stack exceeds the initial size of the GrowableArrays then we will hit an assertion when trying to grow the size of the arrays (see bug description).
Currently we don't see any issues because the initial size of the GrowableArrays is 512 and our tests don't test beyond that (the maximum value of DEPTH in the vmTestbase/nsk/stress/strace/ tests is 500). The issue can be easily reproduced by either decreasing the initial size of the GrowableArrays or by increasing the value of DEPTH in those strace tests.
To fix it I allocated the arrays in the C heap instead. Also I lowered the initial size of the arrays since 512 seemed too much to start with.
Tested it by running all tests in the vmTestbase/nsk/stress/strace/ directory.
Thanks,
Patricio
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/10424/head:pull/10424
$ git checkout pull/10424
Update a local copy of the PR:
$ git checkout pull/10424
$ git pull https://git.openjdk.org/jdk pull/10424/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 10424
View PR using the GUI difftool:
$ git pr show -t 10424
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/10424.diff