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

Include an output format for speedscope #161

Merged
merged 11 commits into from Aug 20, 2018

Conversation

Projects
None yet
3 participants
@jlfwong
Contributor

jlfwong commented Jul 17, 2018

Hi! This is a PR to generate output from rbspy that can be consumed by https://www.speedscope.app/ for fast interactive flamechart visualization & exploration in-browser.

To test this out, generate a profile, then convert it to a speedscope json file like so:

rbspy report -i /Users/jlfwong/.cache/rbspy/records/rbspy-2018-07-12-4h5BlXOheu.raw.gz -f speedscope -o ~/Downloads/foo.speedscope.json

Then drop the resulting .speedscope.json file into https://www.speedscope.app/

This is my first ever PR in Rust, so I expect there to be a lot of changes needed before this can plausibly land.

My eventual plan is to make this integration even more seamless, so you don't even need to find the json file output and drag it into browser. Ideally you'd be able to do something like rbspy view /path/to/profile, and it would open the profile in-browser automatically using a checked-in copy of speedscope. That was the approach I proposed in tmm1/stackprof#100. This PR is just the first step to generate a form that speedscope can consume.

I first asked about potential integration with rbspy in this tweet :) https://twitter.com/jlfwong/status/1015844929027780608

Fixes jlfwong/speedscope#31

@@ -111,6 +111,12 @@ fn get_proc_children() -> Result<Vec<(pid_t, pid_t)>, Error> {
let pids = listpids(ProcType::ProcAllPIDS).map_err(convert_error)?;
// I had to make this modification to fix a cargo build error!

This comment has been minimized.

@jlfwong

jlfwong Jul 17, 2018

Contributor

This is the error I get without this change:

error[E0382]: use of moved value: `convert_error`
   --> src/ui/descendents.rs:120:18
    |
112 |     let pids = listpids(ProcType::ProcAllPIDS).map_err(convert_error)?;
    |                                                        ------------- value moved here
...
120 |         .map_err(convert_error)?;
    |                  ^^^^^^^^^^^^^ value used here after move
    |
    = note: move occurs because `convert_error` has type `[closure@src/ui/descendents.rs:108:25: 110:6]`, which does not implement the `Copy` trait

This comment has been minimized.

@liaden

liaden Jul 17, 2018

Collaborator

Might be due to using a more recent version of rust?

This comment has been minimized.

@jlfwong

jlfwong Jul 17, 2018

Contributor

Is there a specific version of rust I should be targeting?

This comment has been minimized.

@liaden

liaden Jul 17, 2018

Collaborator

Based on the travis.yml, I'd expect latest. Just figured that might be why you are see an error whereas other builds may not see it.

SpeedscopeFile {
// This is always the same
schema: "https://www.speedscope.app/file-format-schema.json".to_string(),

This comment has been minimized.

@liaden

liaden Jul 17, 2018

Collaborator

Could we download the file and vendor it locally?

This comment has been minimized.

@jlfwong

jlfwong Jul 17, 2018

Contributor

This is part of how speedscope detects that the file is a speedscope file. rbspy doesn't actually need to download this file or do anything fancy with it -- it can be effectively treated as an opaque string.

@jlfwong

This comment has been minimized.

Contributor

jlfwong commented Jul 19, 2018

After using this for some visualization of multi-process data at work, I realized that we do store the process ID for every stack trace, and that I'm ignoring that information. That'll be necessary for the time ordered view in speedscope to make any sense, so I'll rework this to avoid mixing the samples from different pids together

@jvns

This comment has been minimized.

Collaborator

jvns commented Jul 24, 2018

let me know when you think this is ready to merge!

@jlfwong

This comment has been minimized.

Contributor

jlfwong commented Aug 15, 2018

@jvns I think this should be ready for a first pass view now!

@jvns jvns merged commit 89d5c2b into rbspy:master Aug 20, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jvns

This comment has been minimized.

Collaborator

jvns commented Aug 20, 2018

Neat! I tried it out and this seems super cool. If you have any changes you'd like to make to the speedscope code (or anything else!) just send another PR :)

jlfwong added a commit to jlfwong/rbspy-docs that referenced this pull request Aug 21, 2018

Update using-rbspy.md to document speedscope format
This is to reflect the format landed in rbspy/rbspy#161
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment