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 support for JS Self-Profiling data #388

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

nornagon
Copy link

This adds support for importing data in the format produced by the JS Self-Profiling API.

I'm not too familiar with the internals of this project, so I went the cheap route: there's a fairly straightforward mapping from the Self-Profiling data format to the Speedscope SAMPLED file format, so I've done that mapping and delegated to the Speedscope importer to do the rest of the work.

@jlfwong
Copy link
Owner

jlfwong commented May 29, 2022

Hi @nornagon!

Thanks for adding this -- I didn't know this API existed, but it's so cool that it does!

The code as-is certainly isn't the best way of doing it. You probably want to use StackListProfileBuilder, like we do here: https://github.com/jlfwong/speedscope/blob/main/src/import/bg-flamegraph.ts

Before this can merge, this would also need tests. You can see example tests here: https://github.com/jlfwong/speedscope/blob/main/src/import/bg-flamegraph.test.ts

Given that this is also a new profile type, this would also ideally include the files necessary to generate this kind of profile locally, in case other features are added to the profile filetype. You can see example programs e.g. in https://github.com/jlfwong/speedscope/tree/main/sample/programs/javascript.

Which brings me to a question: I tried generating one myself locally to understand this, and ran into errors that say something like [Violation] Document policy violation: js-profiling is not allowed in this document. It seems like this is fixable via setting headers from the server. How have you been generating profiles for this?

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