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

Add option to explicitly annotate Java frames with _[j]/_[i] #62

Merged
merged 1 commit into from
Jan 31, 2018

Conversation

nitsanw
Copy link
Member

@nitsanw nitsanw commented Oct 9, 2017

This is not a well broken down PR, sorry.
I lumped together some code cleanup/changes here:

  • resolving many warnings pointed out by CLion
  • refactoring JVMTI deallocation
  • splitting out method name cleanup and '.' vs '/' conversion
  • annotating inlined vs real frames explicitly. This is more accurate than than the approach taken in FlameGraph/stackcollapse-perf.pl which relies on package names.
    If this is too much I can split the commit into separate PRs.

With the new options I can have PMA generate this format, which let's BCC tools generate a usable collapased stack directly:

address size package/classname::methodname_[j];package/classname::methodname_[i]

One could argue that the right solution is a map file post processing script rather than further elaboration of the tool itself. Up for discussion.

@nitsanw
Copy link
Member Author

nitsanw commented Oct 12, 2017

@jrudolph any chance you can take a look?

@nitsanw
Copy link
Member Author

nitsanw commented Oct 24, 2017

ping?

@jrudolph
Copy link
Member

@nitsanw sorry for the silence... busy months. I'll try to have a look soon.

This is done to natively support frame disambiguation. This is
more accurate than than the approach taken in
FlameGraph/stackcollapse-perf.pl which relies on package names.
I did lump together some code cleanup here, resolving many
warnings pointed out by CLion, refactoring deallocate etc. If
this is too much I can split the commit into separate PRs.
@nitsanw
Copy link
Member Author

nitsanw commented Jan 31, 2018

@jrudolph well... at least now we know it builds!!! :-)

Copy link
Member

@jrudolph jrudolph left a comment

Choose a reason for hiding this comment

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

Sorry, it took me so long. ;)

LGTM, can you add the new options to the README?

@@ -103,9 +114,9 @@ static void sig_string(jvmtiEnv *jvmti, jmethodID method, char *output, size_t n
if(entrycount > 0) lineno = lines[0].line_number;
snprintf(source_info, sizeof(source_info), "(%s:%d)", sourcefile, lineno);

if (lines != NULL) (*jvmti)->Deallocate(jvmti, (unsigned char *) lines);
deallocate(jvmti, lines);
Copy link
Member

Choose a reason for hiding this comment

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

Good cleanup 👍

bool clean_class_names = false;
bool dotted_class_names = false;
Copy link
Member

Choose a reason for hiding this comment

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

Yes, makes sense to add further options. Can your document the new ones?

@nitsanw nitsanw merged commit 3e1e292 into jvm-profiling-tools:master Jan 31, 2018
@nitsanw
Copy link
Member Author

nitsanw commented Jan 31, 2018

Will update the README shortly

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants