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

Added Zoom inside SVG #35

Merged
merged 3 commits into from
Oct 16, 2014
Merged

Added Zoom inside SVG #35

merged 3 commits into from
Oct 16, 2014

Conversation

Saruspete
Copy link

This extends svg script to zoom/unzoom.
This was only tested under chromium, but should not break from standard svg.

You may consider moving the "Reset Zoom" button to a more "clean" position.

@cykl
Copy link

cykl commented Oct 12, 2014

zoom / unzoom seems to work fine on Firefox too.

However I noticed that if you click on an empty box, because it is too narrow, then the zoomed view is empty too. Could this limitation be fixed ? It would be very useful to drill down complex flame graphs.

I uploaded an example of a such flame graph here. You can easily reproduce the issue by clicking on one of the left boxes.

@Saruspete
Copy link
Author

Thanks for testing cykl. Do you mean the zoomed boxes doesn't have the text inside ?
I tried to implement that, but I don't know how to get the "text" box, generated by the "title" to be re-calculated by the renderer.

I'll try to implemented that. I found it also limitating :(

@cykl
Copy link

cykl commented Oct 12, 2014

I just read flamegraph.pl source code. Basically the clipping is done in the Perl script and not in the browser. The only way to achieve what we want is to always put the full string in the text element and find a way to let the browser do the clipping.

Unfortunately, it seems that most browsers do not support the CSS3/SVG2 text-overflow property and SVG 1 does not provide an easy way to automatically manage the overflow. I believe that a rect clipMask could be used but then zoom_child and zoom_reset must be updated to update the mask too ... I can give it a try if you want, but I am definitively not fluent in SVG.

@Saruspete
Copy link
Author

Sorry, I'm late
Indeed, I saw the "text" was generated by flamegraph itself. As I don't want to break compatibility, I'll do it in javascript. Should be only a few lines more without need of css3 or svg2.

@cykl
Copy link

cykl commented Oct 12, 2014

Yup JavaScript does the job. I quickly wrote a proof of concept

The initial clipping should still be done by flamegraph.pl. Otherwise it would break png creation (convert out.svg out.png). I can polish up the patch if needed.

@Saruspete
Copy link
Author

Ok, I added some fixes and parts of your diff without touching a lot to the standard structure.
Also, I'm using the "title" instead of adding another attribute to keep small size when a lot of attributes are generated.

It's still working correctly with convert. Can you test it with firefox ?

@Saruspete Saruspete changed the title Added Zoom inside SVG. Currently tested with Chromium only Added Zoom inside SVG Oct 12, 2014
@Saruspete
Copy link
Author

Tested with Chromium 38 and Firefox 24.1.1 on Windows, also working.

@cykl
Copy link

cykl commented Oct 13, 2014

I can see three issues:

  • Using the title to fill the <text> fine, and it avoids to weight down the SVG file. However it is perfectly valid for $func to contain spaces. You cannot split on the first space. It should be fine to split on the first ( starting from the right.
  • ".." is sometimes displayed in narrow cells. I believe that it's better to leave these cells blank. It does not convey any useful information but clutter up the graph
  • In complex flame graphs, when zoomed in, the text is quite often misaligned.

@Saruspete
Copy link
Author

Thanks for pointing them :)

  • Should be fixed with var txt = e.childNodes[1].textContent.replace(/\([^(]*\)/,"");.
  • Should be fixed also in next commit. There is still some chars that will dissapear when zooming then reset. This is because of the width calculated is using the 2 last chars, which are often bigger than "..". To fix it, I can pre-calculate the width of ".." and use this value (instead of using the size of the 2 last chars that will be replaced by ".."). Do you think it is worth it ?
  • I'm trying to fix the misalignment. It's bacause of the 3px added on the text. this small space takes a huge size when the ratio is high. I'm testing it and try to commit it tonight.

@Saruspete
Copy link
Author

Maybe because of the rouding or float calculation, I wasn't able to fix properly the "+3" pixels. So I aligned the "x" with the parent rect.

Maybe there is too many index relations between the DOM Nodes (2 is rect, 4 is text, etc...). Maybe replacing these numbers with a function may be cleaner, but it'll also add complexity.

@Saruspete
Copy link
Author

@cykl can you provide me a heavy stack (like the one used to generate flamegraph35.svg) ? I'd like to test what a search function for a tagname will imply as performance hit, versus direct index.

brendangregg added a commit that referenced this pull request Oct 16, 2014
@brendangregg brendangregg merged commit b33c391 into brendangregg:master Oct 16, 2014
@brendangregg
Copy link
Owner

This is awesome Saruspete, thanks!

Something minor to do later: when zoomed, I'd still like to see the lower ancestry frames.

@cykl
Copy link

cykl commented Oct 16, 2014

@Saruspete If you want a beta tester or sharing ideas, feel free to contact me via email to stop polluting brendan's inbox :)

The stack straces I use in my unit tests for hprof2flamegraph are quite heavy, you can use them:

git clone https://github.com/cykl/hprof2flamegraph.git
cd hprof2flamegraph/
./hprof2flamegraph.py tests/ref/cpu\=samples\,depth\=100\,interval\=10\,lineno\=y\,thread\=y.hprof.txt 

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.

3 participants