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

Support alloc event in pprof converter #844

Closed
wants to merge 5 commits into from

Conversation

heart4lor
Copy link

Currently, the pprof converter only supports CPU events. This PR adds support for converting alloc events to pprof format.

src/converter/one/jfr/event/Event.java Outdated Show resolved Hide resolved
src/converter/jfr2pprof.java Outdated Show resolved Hide resolved
src/converter/jfr2pprof.java Show resolved Hide resolved
src/converter/jfr2pprof.java Outdated Show resolved Hide resolved
@apangin
Copy link
Collaborator

apangin commented Nov 24, 2023

Note there is already another PR #713 for the similar functionality.
If you wish, you may contribute to the existing PR instead. I will likely accept one of these, whichever is ready first. Thanks.

@heart4lor
Copy link
Author

@apangin Hi, thanks for advices. I've resolved comments above. For alloc event, I kept the nanosecond field, and put allocated bytes key-value into sample labels. What do you think about it? Please review again, thanks!

src/converter/jfr2pprof.java Outdated Show resolved Hide resolved
src/converter/jfr2pprof.java Show resolved Hide resolved
src/converter/jfr2pprof.java Outdated Show resolved Hide resolved
src/converter/jfr2pprof.java Outdated Show resolved Hide resolved
src/converter/jfr2pprof.java Outdated Show resolved Hide resolved
@heart4lor
Copy link
Author

@apangin All resolved, please review again. Thanks!

profile.field(PROFILE_STRING_TABLE, "allocation_size".getBytes(StandardCharsets.UTF_8));
final int allocationSizeKeyId = stringId++;
profile.field(PROFILE_STRING_TABLE, "bytes".getBytes(StandardCharsets.UTF_8));
final int allocationSizeUnitId = stringId++;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are these strings only need for allocation profile? Does it make sense to put them under if (eventClass == AllocationSample.class)?

final Proto sample = new Proto(1_000).field(SAMPLE_VALUE, nanosSinceLastSample);
sample.field(SAMPLE_VALUE, nanosSinceLastSample);
if (eventClass == AllocationSample.class) {
sample.field(SAMPLE_LABEL, allocationLabel.field(LABEL_NUM, jfrSample.value()));
Copy link
Collaborator

Choose a reason for hiding this comment

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

field method mutates the object; it's not correct to reuse allocationLabel this way.

File dst = new File(args[1]);
Class<? extends Event> eventClass = args.alloc ? AllocationSample.class : ExecutionSample.class;

String output = Optional.ofNullable(args.output).orElse(args.input.replace(".jfr", ".pprof"));
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd rewrite this with a regular if: Optional makes this harder to follow. Also, args.input.replace will be called regardless of the value of output.

@apangin
Copy link
Collaborator

apangin commented Jan 23, 2024

Hi, is this PR still relevant?

@apangin
Copy link
Collaborator

apangin commented Mar 16, 2024

Obsoleted by #905

@apangin apangin closed this Mar 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants