-
Notifications
You must be signed in to change notification settings - Fork 106
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 support for visualizing CPU Profile #197
Add support for visualizing CPU Profile #197
Conversation
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.
Can you add some docs and how to generate those profiles?
If tests could be added, go for it. This repo is really low on tests.
@mcollina you just go into the example repo I linked to https://github.com/slonka/0x-visualize-v8-profile-example, clone it, run |
Can you add that code in the https://github.com/davidmarkclements/0x/tree/master/examples and add a section about it in the README? |
Yeah sure, I thought that you had problems running the example. |
3621d22
to
6afc6ed
Compare
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.
LGTM
How about merged vs unmerged? How they differ / is there an algorithm to convert them? How about optimized vs unoptimized? |
I think you can’t provide that info through that output. We can detect that by parsing the isolate.log file. The buttons can either do nothing, or we can grey them out in this case. |
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.
can you please either
- add merge/unmerge functionality if it's possible with the data
OR - disable merge button
Sure, can you help me with understanding the merging algorithm? |
6afc6ed
to
a2252e8
Compare
hey @slonka it might not apply here - I'll described and see if it fits v8-profile visualizing: with the current method, (using The merge button removes the optimized/unoptimized information, but groups both function states into the JS function which creates a simplified graph. Does this make sense? |
Yes, sure. How can I disable the "merge" button? |
if the v8-profiler doesn't differentiate between optimized/unoptimized we'll also need to disable the Optimized Unoptimized buttons as well Additionally the filter buttons – possibly many of those don't apply? They should also be disabled The buttons are HTML buttons so can have a disabled attribute set, this is passed down to a basic function component here: and they're called here: https://github.com/davidmarkclements/0x/blob/master/visualizer/cmp/controls.js#L19-L33 you might need to adjust/add to the state (https://github.com/davidmarkclements/0x/blob/master/visualizer/state.js) to determine whether a button should be disabled, the state is initialized in the visualizer here: https://github.com/davidmarkclements/0x/blob/master/visualizer/index.js#L52 and options can be passed to the visualizer from here: https://github.com/davidmarkclements/0x/blob/master/lib/render.js#L23 |
@davidmarkclements - I disabled the buttons when flamegraph is generated from v8 profiler output. |
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.
LGTM
BTW, this repo needs more tests definitely because the previous version broke the tool and I only noticed when I wanted to use it with different switches. I'll open a new PR for that. |
@slonka some tests have been added in the last week. You might want to rebase. |
14d5767
to
01fdc75
Compare
I did the rebase, any chance to get this merged? @mcollina @davidmarkclements |
amazing PR @slonka sorry it took so long, I'm just going to rename it to --visualize-cpu-profile – especially in light of nodejs/node#27147 |
renaming commit here: |
Thanks for merging this 🎊 |
Are you sure the format of --cpu-prof matches the one provided here? |
No, however I expect it to be very similar to the extent that a tweak or two will make it work for both scenarios |
...gathered by CpuProfiler::StartProfling.
As suggested in the comment: #196 (comment) here is an example https://github.com/slonka/0x-visualize-v8-profile-example of how to generate v8 profile at runtime and a flamegraph from it.
Example flamegraph looks like this:
There is a couple of things that this PR is missing:
But the basic functionality is there. Would love help improving this.