Skip to content
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

[JFR] Fix object profiling stacktrace problem on aarch64 platform #190

Merged
merged 1 commit into from
Dec 1, 2021
Merged

[JFR] Fix object profiling stacktrace problem on aarch64 platform #190

merged 1 commit into from
Dec 1, 2021

Conversation

D-D-H
Copy link
Collaborator

@D-D-H D-D-H commented Nov 29, 2021

Hi,
Please help review this patch that fixes the stack trace problem of object profiling on the aarch64 platform.

The root cause of this problem is that the stack layout of aarch64 is different from x86_64 so that we cannot use vframeStream(thread, os::current_frame()) to get the java call stack traces.

This fix uses CallStaticJavaNode to invoke the jfr_fast_object_alloc_C so that the _last_Java_pc is set properly by opto stub.

Issue: #189
Denghui

@CLAassistant
Copy link

CLAassistant commented Nov 29, 2021

CLA assistant check
All committers have signed the CLA.

@D-D-H D-D-H requested a review from sanhong November 29, 2021 07:49
@D-D-H D-D-H self-assigned this Nov 29, 2021
@D-D-H D-D-H added the bug Something isn't working label Nov 29, 2021
@D-D-H D-D-H requested a review from zhengxiaolinX November 30, 2021 07:15
Copy link
Contributor

@zhengxiaolinX zhengxiaolinX left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm.

@D-D-H
Copy link
Collaborator Author

D-D-H commented Nov 30, 2021

@yfxhust
Hi Fangxi,
Please help review this fix when you have time.
This patch used CallStaticJavaNode to replace CallLeafNode, so the _cached_top_frame_bci and stack walk mode could be removed. Plz help confirm this logic is correct.

@yfxhust
Copy link

yfxhust commented Nov 30, 2021

@yfxhust Hi Fangxi, Please help review this fix when you have time. This patch used CallStaticJavaNode to replace CallLeafNode, so the _cached_top_frame_bci and stack walk mode could be removed. Plz help confirm this logic is correct.

  // Jfr callstack collection relies on vframeStream.
  // But the bci of top frame can not be determined by vframeStream in some scenarios.
  // For example, in the opto CallLeafNode runtime call of
  // OptoRuntime::jfr_fast_object_alloc_C, the top frame bci
  // returned by vframeStream is always invalid. This is largely due to the oopmap that
  // is not correctly granted ( refer to PhaseMacroExpand::expand_allocate_common to get more details ).
  // The opto fast path object allocation tracing occurs in the opto CallLeafNode,
  // which has been broken by invalid top frame bci.
  // To fix this, we get the top frame bci in opto compilation phase
  // and pass it as parameter to runtime call. Our implementation will replace the invalid top
  // frame bci with cached_top_frame_bci.

This is why _cached_top_frame_bci. I am not sure whether it still stands in current static Java call implementation.

@yfxhust
Copy link

yfxhust commented Nov 30, 2021

LGTM

@yfxhust yfxhust closed this Nov 30, 2021
@yfxhust yfxhust reopened this Nov 30, 2021
@D-D-H
Copy link
Collaborator Author

D-D-H commented Nov 30, 2021

@yfxhust Hi Fangxi, Please help review this fix when you have time. This patch used CallStaticJavaNode to replace CallLeafNode, so the _cached_top_frame_bci and stack walk mode could be removed. Plz help confirm this logic is correct.

  // Jfr callstack collection relies on vframeStream.
  // But the bci of top frame can not be determined by vframeStream in some scenarios.
  // For example, in the opto CallLeafNode runtime call of
  // OptoRuntime::jfr_fast_object_alloc_C, the top frame bci
  // returned by vframeStream is always invalid. This is largely due to the oopmap that
  // is not correctly granted ( refer to PhaseMacroExpand::expand_allocate_common to get more details ).
  // The opto fast path object allocation tracing occurs in the opto CallLeafNode,
  // which has been broken by invalid top frame bci.
  // To fix this, we get the top frame bci in opto compilation phase
  // and pass it as parameter to runtime call. Our implementation will replace the invalid top
  // frame bci with cached_top_frame_bci.

This is why _cached_top_frame_bci. I am not sure whether it still stands in current static Java call implementation.

JVMState includes the BCI info, and OptoStub can set _last_java_sp properly, hence, vframeStream could get the correct context information.

@D-D-H
Copy link
Collaborator Author

D-D-H commented Nov 30, 2021

LGTM

Thank you!

Summary:
1. Use CallStaticJavaNode to invoke jfr_fast_object_alloc_C so that
last_java_sp is set properly
2. Remove StackWalkMode and _cached_top_frame_bci

Test Plan: test/jdk/jfr/event/objectsprofiling/TestOptoObjectAllocationsSampling.java

Reviewed-by: kelthuzadx, kuaiwei, sandlerwang, zhengxiaolinX

Issue: #189
Copy link
Collaborator

@kuaiwei kuaiwei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@D-D-H
Copy link
Collaborator Author

D-D-H commented Dec 1, 2021

Thanks again for the review!

@D-D-H D-D-H merged commit 0dc5812 into dragonwell-project:master Dec 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants