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

Rewrite flamegraph viewer using canvas #74

Merged
merged 14 commits into from
Aug 25, 2017
Merged

Conversation

aroben
Copy link
Collaborator

@aroben aroben commented Oct 19, 2016

Large profiles were generating so many SVG nodes that the browser was
bogging down. Using canvas keeps the DOM very simple, and repainting it
seems to be pretty fast. It also lets us drop the D3 and jQuery
dependencies for a smaller page load.

The viewer has been flipped vertically: the small overview graph is on
the top, and stacks grow top down instead of bottom up. This matches
Chrome's devtools timeline and many other timeline/flamegraph tools I've
seen.

Controls are:

  • drag: pan/scroll
  • scrollwheel: pan/scroll
  • Shift-scrollwheel: zoom

Click-and-drag to select a region in the small overview graph isn't
currently supported, nor is dragging the selected region to pan/scroll.
That would be nice to add back eventually, though I've found that
zoom/scroll performance is good enough that I haven't really missed
those features.

This should be compatible with IE 10 and newer, though I've only tested
in Safari, Firefox, and Chrome.

/cc #73 @waiting-for-dev @tmm1 @kdaigle

Large profiles were generating so many SVG nodes that the browser was
bogging down. Using canvas keeps the DOM very simple, and repainting it
seems to be pretty fast. It also lets us drop the D3 and jQuery
dependencies for a smaller page load.

The viewer has been flipped vertically: the small overview graph is on
the top, and stacks grow top down instead of bottom up. This matches
Chrome's devtools timeline and many other timeline/flamegraph tools I've
seen.

Controls are:

 - drag: pan/scroll
 - scrollwheel: pan/scroll
 - Shift-scrollwheel: zoom

Click-and-drag to select a region in the small overview graph isn't
currently supported, nor is dragging the selected region to pan/scroll.
That would be nice to add back eventually, though I've found that
zoom/scroll performance is good enough that I haven't really missed
those features.

This should be compatible with IE 10 and newer, though I've only tested
in Safari, Firefox, and Chrome.
@tmm1
Copy link
Owner

tmm1 commented Oct 19, 2016

Looks great! Definitely many times more performant.

cc @SamSaffron

@SamSaffron
Copy link

It is very cool, some bits of feedback...

  • I would be nice to be able to select range from the top part like chrome does with flamechart
  • frame info is missing, which is very useful (click on frame)
  • would love if this is instead added to https://github.com/SamSaffron/flamegraph and then stackprof consumes that instead (cause rack-mini-profiler uses flamegraph gem and it is a standalone thing)

@waiting-for-dev
Copy link

Hey, great job! Just two considerations:

  • For a lot of data it gets pretty quirk. Following screenshot if with 88M:

screenshot-flamegraph

  • Scrolling to the bottom seems to be quite slow

Many, many thanks!

@aroben
Copy link
Collaborator Author

aroben commented Oct 20, 2016

@SamSaffron @waiting-for-dev Thanks for trying it out.

I would be nice to be able to select range from the top part like chrome does with flamechart

This was mentioned as a limitation in the PR body, and I agree it would be good to add. Since you mentioned it I'll try to get it implemented in this initial version.

frame info is missing, which is very useful (click on frame)

Is this different from the info that is shown when you hover over a frame?

screen shot 2016-10-20 at 9 13 30 am

would love if this is instead added to https://github.com/SamSaffron/flamegraph and then stackprof consumes that instead (cause rack-mini-profiler uses flamegraph gem and it is a standalone thing)

@tmm1 what do you think about this?

For a lot of data it gets pretty quirk. Following screenshot if with 88M:

@waiting-for-dev I don't see anything obviously wrong with that screenshot. To me it just looks like you need to zoom in (Shift-mousewheel) so you can see what's going on in those lower regions. Have you tried that?

Scrolling to the bottom seems to be quite slow

If you can provide me with your stackprof profile I can try to figure out how to speed it up.

@aroben
Copy link
Collaborator Author

aroben commented Oct 20, 2016

I've noticed that scrolling performance is much better in Safari than in Chrome. I guess Safari's canvas is more optimized for this case. I'll see if I can figure out how to make it faster in Chrome. (It's still far faster than the SVG implementation.)

@aroben
Copy link
Collaborator Author

aroben commented Oct 20, 2016

Looks like CanvasRenderingContext2D.prototype.measureText is much slower in Chrome than in Safari, which accounts for almost all of the performance difference. I'll see if I can minimize calls to that method.

CanvasRenderingContext2D.prototype.measureText is quite slow in Google
Chrome. Removing calls to it during painting speeds up painting a
large-ish profile (3MB) from 9.9fps to 38.5fps in Chrome, and from
41.7fps to 50fps in Safari.
We now first draw all frame backgrounds, then draw all frame labels. And
when drawing the backgrounds, we group them by color. This lets us set
the fillStyle a minimum number of times: once for each color, and once
before drawing all the labels.

This speeds up rendering of a 3MB profile from 38.5fps to 50fps in
Chrome, and from 50fps to 62.5fps in Safari.
@aroben
Copy link
Collaborator Author

aroben commented Oct 20, 2016

I just pushed up some optimizations. Rendering time for a 3MB profile increased from 9.9fps to 50fps in Chrome, and from 41.7fps to 62.5fps in Safari. Check it out at http://roben.org/stackprof-flamegraph/3381fd86cf0e01918f4f5786481e724e4682fd58/viewer.html?data=/stackprof-flamegraph/rubocop-spec-flamegraph.txt

@aroben
Copy link
Collaborator Author

aroben commented Oct 20, 2016

I would be nice to be able to select range from the top part like chrome does with flamechart

I've pushed up some support for this.

@aroben
Copy link
Collaborator Author

aroben commented Oct 20, 2016

@aroben
Copy link
Collaborator Author

aroben commented Oct 20, 2016

I'd like to implement capturing of mouse events while dragging. Right now you can quickly move the mouse off of the overlay while you're dragging and it will stop responding to you.

@SamSaffron
Copy link

Getting lots of artifacts on the latest example:

image

Regarding frame info was talking about this:

image

Long term I want us to ship an optional middleware that allows you to view a syntax highlighted source file, that way when you see a frame is playing up you can simply click and view source.

This also highlight the problem of having a fork of flamegraph, everyone end up losers this way.

@tmm1 I think it is correct to keep it in flamegraph gem, that way @headius can eventually make this thing work in JRuby as well :)

Happy to give you and @aroben ownership on flamegraph gem, in fact I did so now.

@aroben
Copy link
Collaborator Author

aroben commented Oct 21, 2016

Getting lots of artifacts on the latest example:

I seem to have really screwed things up on Retina displays. I was working on a non-Retina display today. This shouldn't be too hard to fix. I'll take a look tomorrow.

Regarding frame info was talking about this:

Ah, interesting. That does seem nice. The existing implementation in this repo doesn't have that view, so I think this PR could probably land without it. But if we move this into the flamegraph gem we'd of course want to keep that working.

Long term I want us to ship an optional middleware that allows you to view a syntax highlighted source file, that way when you see a frame is playing up you can simply click and view source.

Yes, having a quick way to get from the flamegraph to the source code is something I've been wanting as well.

@waiting-for-dev
Copy link

Thanks for your work on this @aroben .

@waiting-for-dev I don't see anything obviously wrong with that screenshot. To me it just looks like you need to zoom in (Shift-mousewheel) so you can see what's going on in those lower regions. Have you tried that?

You are right. I can zoom in. I had tried with Firefox and, there, now I see the zoom works in the opposite way I feel natural: I need to move the mousewheel backwards. In Chrome it works "well"; forwards. I don't know if it is just some browser default behavior or something.

If you can provide me with your stackprof profile I can try to figure out how to speed it up.

Now I see that, in Chrome, scrolling and zooming have an acceptable speed. However, in Firefox is quite slow, making it not very handy to work with large files. In a minute, I'm going to send to your private email account (the one in your Github profile) a link to download a real example.

This was totally wrong. Changing the canvas's dimensions resets its
transform (along with a bunch of other state).

This reverts commit 92fc987.
@aroben
Copy link
Collaborator Author

aroben commented Oct 21, 2016

You are right. I can zoom in. I had tried with Firefox and, there, now I see the zoom works in the opposite way I feel natural: I need to move the mousewheel backwards. In Chrome it works "well"; forwards. I don't know if it is just some browser default behavior or something.

What OS and browser versions are you using? If you're on OS X, do you have "Scroll direction: natural" enabled in your mouse settings?

Now I see that, in Chrome, scrolling and zooming have an acceptable speed. However, in Firefox is quite slow, making it not very handy to work with large files. In a minute, I'm going to send to your private email account (the one in your Github profile) a link to download a real example.

I'll take a look. Thanks!

@waiting-for-dev
Copy link

What OS and browser versions are you using? If you're on OS X, do you have "Scroll direction: natural" enabled in your mouse settings?

I'm on Firefox 48.0.2 in debian 8 (in fact a VM on QubesOS 3.2, but this should be irrelevant).

There are two changes here:

First, we now correctly respect OS X's "Scroll direction: Natural"
setting in Safari. I.e., if you turn that setting off, sliding your
fingers up on your trackpad will still result in a zoom-in gesture.
(Firefox and Chrome don't support the APIs needed for this, so if you
turn that setting off then in those browsers you'll have to slide your
fingers down to zoom in.)

Second, the zoom direction has been fixed on Windows/Linux. Scrolling
your mousewheel up now results in zooming in, just like it does on OS X.
@aroben
Copy link
Collaborator Author

aroben commented Oct 21, 2016

I'm on Firefox 48.0.2 in debian 8 (in fact a VM on QubesOS 3.2, but this should be irrelevant).

Were you using Chrome on a Mac, or also on Linux? If Chrome and Firefox were both on Linux I'd be surprised if there zoom behavior were different.

http://roben.org/stackprof-flamegraph/df8afd35e9c7f85a92532a7323682a05d55c4a35/viewer.html?data=/stackprof-flamegraph/rubocop-spec-flamegraph.txt should have the zoom direction fixed on Windows/Linux. Let me know how it works for you.

@aroben
Copy link
Collaborator Author

aroben commented Oct 21, 2016

Now I see that, in Chrome, scrolling and zooming have an acceptable speed. However, in Firefox is quite slow, making it not very handy to work with large files. In a minute, I'm going to send to your private email account (the one in your Github profile) a link to download a real example.

I'll take a look. Thanks!

Firefox and Chrome perform about the same for me with that profile. I'm on macOS 10.12 and don't have a Linux machine handy for testing. You could try capturing a JavaScript profile in Firefox's devtools and see where the time is being spent.

@waiting-for-dev
Copy link

Hey! I'll be able to look at this on Monday. This weekend I'm far from the computer :) Thanks!

@waiting-for-dev
Copy link

Were you using Chrome on a Mac, or also on Linux? If Chrome and Firefox were both on Linux I'd be surprised if there zoom behavior were different.

Yes, it was in the same Linux machine. It also seemed strange to me...

http://roben.org/stackprof-flamegraph/df8afd35e9c7f85a92532a7323682a05d55c4a35/viewer.html?data=/stackprof-flamegraph/rubocop-spec-flamegraph.txt should have the zoom direction fixed on Windows/Linux. Let me know how it works for you.

Great! Now in both Firefox and Chrome I have to move the mousewheel forwards.

Firefox and Chrome perform about the same for me with that profile. I'm on macOS 10.12 and don't have a Linux machine handy for testing. You could try capturing a JavaScript profile in Firefox's devtools and see where the time is being spent.

It is not that it performs bad in Firefox. Simply it has an unsuitable "scale". I mean, for the same mousewheel movement I zoom in much more in Chrome than in Firefox. Anyway, here you have a javascript profile just zooming in in Firefox:

https://gist.github.com/waiting-for-dev/4b7b241cc330ee33c68b8af670192a88

Thanks

@waiting-for-dev
Copy link

waiting-for-dev commented Oct 25, 2016 via email

Firefox does line-based scrolling when a physical mousewheel (as opposed
to a trackpad) is used. We were interpreting the wheel deltas as pixels
in all cases, but now we handle line-based scrolling correctly.
@aroben
Copy link
Collaborator Author

aroben commented Oct 26, 2016

@waiting-for-dev
Copy link

waiting-for-dev commented Oct 26, 2016

@aroben awesome! Now it works perfectly well in Firefox :)

@parkr
Copy link

parkr commented Aug 22, 2017

Hey! I was just trying to view a flamegraph from stackprof and got quite stuck. My browser locked up completely. I was looking for one of two things:

  1. A more performant alternative to the viewer (like this PR!)
  2. Output to a flamegraph.pl-compatible input format.

@aroben It sounds like you have addressed all comments above, and this would greatly benefit folks using this gem. Are there any other tests you'd like to run? Perhaps this could be merged into a v0.3.0 of stackprof?

@aroben
Copy link
Collaborator Author

aroben commented Aug 22, 2017

I don't have any objections to merging this (and as far as I know it's been working well internally at GitHub) but I don't have permission to merge it myself.

@aroben aroben merged commit 6b3adc2 into tmm1:master Aug 25, 2017
@aroben aroben deleted the canvas-viewer branch August 25, 2017 16:32
@aroben
Copy link
Collaborator Author

aroben commented Aug 25, 2017

@tmm1 are you able to make a new release?

@parkr
Copy link

parkr commented Aug 25, 2017

Thanks @aroben and @tmm1! There are a lot of really excellent projects under your account, @tmm1! Thank you for sharing them 😄

@brianmario
Copy link

@aroben @parkr just FYI, I think he's out of town for at least another week I think.

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.

6 participants