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

fix(jfr2pprof): Fix multichunk jfr symbols and location's line numbers #879

Merged
merged 2 commits into from
Jan 17, 2024

Conversation

korniltsev
Copy link
Contributor

  • Do not use one.jfr.JfrReader#readAllEvents(java.lang.Class) as it may trigger reading new chunk in multichunk jfrs, overwriting cp, producing wrong data(I observerd wrong class names). Note: it is still used in netflix code I did not touch.

  • Use Location(method+line) as dedup key to fix line numbers

I've used this JFR file for testing, captured by Intellij IDEA Ultimate on Linux
FastSlow_2024_01_16_180855.jfr.zip

- Do not use one.jfr.JfrReader#readAllEvents(java.lang.Class<E>) as it
  may trigger reading new chunk in multichunk jfrs, overwriting cp,
producing wrong data(I observerd wrong class names)

- Use Location(method+line) as dedup key to fix line numbers
Copy link
Collaborator

@apangin apangin left a comment

Choose a reason for hiding this comment

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

Thank you for the Pull Request. The changes are good except for the minor performance note.


@Override
public int hashCode() {
return Objects.hash(method, line);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Objects.hash is inefficient because of varargs - please rewrite the method without Objects.hash. As a general rule, hashCode should not allocate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Rewritten.

@apangin apangin merged commit 77140be into async-profiler:master Jan 17, 2024
1 check passed
@apangin
Copy link
Collaborator

apangin commented Jan 17, 2024

Merged. Thank you for the contribution.

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.

None yet

2 participants