Filtering, Zooming, Detailed Analysis

[thisduck]

Hi Sam,

This is a pull request for the changes I mentioned in #23

Here is a list of changes:

1. Zooming is based on clicking on a node.
2. You can zoom out by one level by pressing the (-) dash/minus button on the keyboard. You reset the zoom entirely by pressing (0) zero on the keyboard (or clicking the reset zoom button).
3. There is a detailed breakdown of the information of a node on hovering.
4. The gems listing has been changed to display both gems and methods.
5. The listings are sorted by "self time," the time the method spends in itself excluding time spent in any children methods.
6. As you zoom in or out, the gem and methods listings display data from the visible areas.
7. Filtering can be done by an include and exclude filter list. The filter searches for any text in the frame. So you can filter by file path, file name, method name, class name, etc.
8. The gems and methods listing changes based on the filtered data (along with the zoomed data).
9. The backtrace modal (triggered by a right click on a node) displays detailed information about the frame, along with a tab that shows any direct method call by the current method.

This is a fairly large rewrite (it was the only way to do it to add filtering and zooming in this way and maintain speed).

I'd love it if you were able to give this a test run on your own code and share the experience.

[thisduck]

I plan on adding a few more features that will make the gem/method listing areas even more useful.

[SamSaffron]

wow, awesome can you do some screenshots

[thisduck]

@SamSaffron I'll do some screenshots soon.

I also want to build in some keyboard based shortcuts and controls, to make investigative navigation easier.

[thisduck]

@SamSaffron I can also do a video tutorial as well. On strategies to approach and navigate performance issues with this interface. Even as I'm using it now, I'm finding that I want features just to make it easier and simpler.

I kinda want this to be really fun and easy to use, that's the drive anyway.

[SamSaffron]

Very cool, very happy you are working on this!

On Sun, Apr 30, 2017 at 10:56 PM, Adnan Ali notifications@github.com wrote:

@SamSaffron https://github.com/SamSaffron I can also do a video tutorial as well. On strategies to approach and navigate performance issues with this interface. Even as I'm using it now, I'm finding that I want features just to make it easier and simpler.

I kinda want this to be really fun and easy to use, that's the drive anyway.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/SamSaffron/flamegraph/pull/24#issuecomment-298278123, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAUXQZcil1lQpPAWevZ0e3KN5bWKqT-ks5r1Un7gaJpZM4NMah8 .

[thisduck]

@SamSaffron Okay, I think this is pretty much ready. I'll get you screenshots and videos soon!

[SamSaffron]

cool, thanks!

On Thu, May 4, 2017 at 10:55 AM, Adnan Ali notifications@github.com wrote:

@SamSaffron https://github.com/SamSaffron Okay, I think this is pretty much ready. I'll get you screenshots and videos soon!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/SamSaffron/flamegraph/pull/24#issuecomment-299210115, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAUXcsgIx4mK2Jx41-Yc2M-bbhBD7aJks5r2eb4gaJpZM4NMah8 .

[thisduck]

@SamSaffron Here is the video of the new flamegraph UI: https://drive.google.com/file/d/0B-8YziZr08bQem05b25PdXlVLXc/view

If you haven't already, I highly recommend trying it out. I am super excited about this, and if I may be cheeky enough to say so, I think if marketed properly it'll change the way performance analysis is done in ruby/rails. =)

[thisduck]

@SamSaffron And of course, I'm open to discussing any of the changes made.

[SamSaffron]

This is looking very polished, I will try it out on Discourse to get a proper feel for it.

There are a few comments I would like to discuss

• I dislike unconditionally losing all the text on the graph. Can we restore that when you zoom in and clearly have space for it. Chrome dev tools has a better text truncation algorithm that we could use.

• There are lots of shortcuts, can you make sure there is a button that display the list of shortcuts in a modal when you click it.

Will provide some more detailed feedback once I try this out "for real"

Thanks so much for all your effort here, its looking really nice.

On Sat, May 6, 2017 at 11:30 PM, Adnan Ali notifications@github.com wrote:

@SamSaffron https://github.com/SamSaffron And of course, I'm open to discussing any of the changes made.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/SamSaffron/flamegraph/pull/24#issuecomment-299680031, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAUXbKxmfqdqAHykAtwVFQeYnvJuaezks5r3TrAgaJpZM4NMah8 .

[thisduck]

@SamSaffron There's already a shortcut definition modal when you press ?, forgot to mention it in the video. I'll add a button as well.

The text on the graph was a large reason for the slowness. I can try and see what I can do to bring it back. Is there anything you'd consider as a substitute for the text in the graph?

[SamSaffron]

I know, the text in the graph is very expensive, longer term we can render with canvas which can make it way cheaper, but for now I want to see the text back especially for cases you are zoomed in and there is clearly room for the text. We can play with the thresholds or even make them configurable.

I would investigate an optimisation perhaps that only renders the text in cells that are in view, or something along those lines.

On Sun, May 7, 2017 at 7:43 AM, Adnan Ali notifications@github.com wrote:

@SamSaffron https://github.com/SamSaffron There's already a shortcut definition modal when you press ?, forgot to mention it in the video. I'll add a button as well.

The text on the graph was a large reason for the slowness. I can try and see what I can do to bring it back. Is there anything you'd consider as a substitute for the text in the graph?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/SamSaffron/flamegraph/pull/24#issuecomment-299700589, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAUXW9rneCcr2Z-lANcJxTj2QsysoFxks5r3a54gaJpZM4NMah8 .

[thisduck]

@SamSaffron I'll see what I can do about the text. Though I'm still trying to understand the benefit/use case. Is it really worth the effort and speed?

Personally, I'd recommend against having text in the graph at all.

[thisduck]

The screenshots above are of the same level of zoom, one is filtered by Rails root and the other is without a filter. In both cases, only some nodes have enough room for the text, but most don't. And this is at a fairly deep level of zoom.

[thisduck]

@SamSaffron I added the text in the graph back. Though I had to covert the rects to divs in order to make this happen while maintaining performance. This has a couple of draw backs, the borders with css aren't as nice. And when there are a lot of frames along the y axis, there are sometimes minor gaps.

No functional issues, however.

Also added a help button.

[SamSaffron]

My biggest blocker here on merging is that I find it super hard to navigate.

Can mousewheel support be added back in? and click and drag graph around support?

[thisduck]

@SamSaffron One of the reasons I changed it to click to zoom is because I had found click and drag harder to navigate, specially on larger datasets. And had similar feedback from others who tried the gem but ended up not using it.

I had a similar experience a few years ago, too. Where speed and navigation prevented from using the gem.

But I respect that existing users have a certain way and comfort of navigating. I do feel like click to zoom is easier overall once you're used to it. Specially to newer users.

[thisduck]

@SamSaffron Also, I changed the nodes to be html based in order to get the text in there, without compromising on speed.

To get click and drag zoom to be nice, I'd have to change back to SVG nodes. But then that would make text nodes slow.

My intent is to improve speed as that impacts usage.

[thisduck]

@SamSaffron Is there something we can do halfway?

Just been thinking about this. I see the benefit, of course, of being able to click and drag. It's just the speed concerns.

Let me process this and see if something comes to mind.

[SamSaffron]

@thisduck any chance we can merge this in as an "option" keep the existing implementation as 1 options and add yours as a second option, they whoever consumes flamegraph chooses the rendering option and you can control and manage (2) completely ?

[thisduck]

@SamSaffron Sounds good. Let me see what I can do in the coming weeks. Thanks!

[thisduck]

Hey @SamSaffron,

One of the reasons I wasn't too keen on retaining the zooming functionality was because with a lot of nodes, it got super slow and hard to maintain. But I've been reading about D3 and Canvas recently and there's ways we can make everyone happy and get all the filtering and analysis goodness. We can potentially have one unified UI this way.

I want to try and refactor the code as well so that it's easier to read and contribute to. How do you feel about a phased approach to getting there? (Something like updating the current code base to ES6, using classes/objects, etc)

I have a fairly decent idea of how to get a bunch of the functionality in there in a performant way since its' already implemented in this PR. But there's a lot we can clean up. And I'll have some time on my hands over December.

(btw, in this PR, I had recently added the ability to track SQL queries through active record, making N+1 detection at the query level clear in the UI!!)

[thisduck]

@SamSaffron

I've started to create smaller PRs on my fork: https://github.com/thisduck/flamegraph/pulls

This way the changes can be small and merged in bit by bit into master.

[SamSaffron]

Awesome, feel free to submit the PRs here, as long as there is toggle between new mode and old mode I am perfectly happy with stuff landing even now.

On Sat, Dec 1, 2018 at 1:49 AM Adnan Ali notifications@github.com wrote:

@SamSaffron https://github.com/SamSaffron

I've started to create smaller PRs on my fork: https://github.com/thisduck/flamegraph/pulls

This way the changes can be small and merged in bit by bit into master.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/SamSaffron/flamegraph/pull/24#issuecomment-443225978, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAUXUVlQd9gvBDUyua4htoyeTxKvzldks5u0UVggaJpZM4NMah8 .

[thisduck]

@SamSaffron Have been plugging away and am very close to something sharable soon.

When you get a moment, can you please send me an example of a flamegraph html that you've been looking at recently? (A few if possible, but one will do great!) Thanks!

[SamSaffron]

Hi @thisduck,

Attached is one I looked at today. I picked this issue up: https://review.discourse.org/t/perf-automatic-upload-size-calculation-not-persisted/868

homepage.zip

[jaydorsey]

I am super late to this, but I just took a look at the branch @thisduck created and overall I'm a fan.

• I love how it's zoomed to 100% of the width of the window
• It's much more responsive to zoom in exactly where I want (once I get used to the controls)
• I had to add newrelic_apm as a dependency in my app (we don't use it)
• This graph, on the same codebase/endpoint, seemed to display the postgres method calls where i expected them to here

I do like the mouse zoom in the old one but it was hit or miss performance wise depending on the graph. Even on a well-equipped Macbook Pro zooming in and what not could be difficult

It would be great to be able to toggle between the two

[thisduck]

Thanks @jaydorsey. I'm going to be without a job in a couple of weeks and can give this attention so that there's either a toggle or some integrated version of the interfaces.

The newrelic_apm dependency was there to display SQL queries and greater N + 1 visibility and that can be made optional. I made it only work with mysql, so it's very limited at the moment.

@SamSaffron Any thoughts if I were to use something like an Ember or React for the interface?

[SamSaffron]

@thisduck sorry to hear you are out of work, wishing you lots of luck with your job hunt. I am fine with ember or react for an interface here.

[thisduck]

So my timeline to work on this wasn't accurate. But am keen on doing something now.

@jaydorsey Would you be able to share the html of some flamegraphs that you were dealing with? Particularly ones that were slow?

[jaydorsey]

@jaydorsey Would you be able to share the html of some flamegraphs that you were dealing with? Particularly ones that were slow?

I wasn't able to find one of the larger graphs since you'd asked, but since then I've moved on to a different role so I won't be able to reproduce it exactly. I think i can fake one though (similar-ish behavior) with a small demo app if that would be useful.