react-flame-graph icon indicating copy to clipboard operation
react-flame-graph copied to clipboard

Replace SVG elements with DIVs

Open bvaughn opened this issue 5 years ago • 14 comments

Possible alternative fix for issue #1 / PR #2.

  • Replace several SVG elements with a DIV. Hopefully this will improve perf a bit and also enable GPU acceleration for animations.
  • Replaced opacity with brightness filter to prevent overlapping text while still showing an indication of which node is selected.

Demo at: https://react-flame-graph-piamdtgdpj.now.sh

resize

Thoughts, @vincentriemer?

bvaughn avatar Jul 16 '18 17:07 bvaughn

I had tried something similar but didn't get good enough results to include in my previous PR, I'll take a closer look at this when I get home from the office.

vincentriemer avatar Jul 16 '18 18:07 vincentriemer

I had over the weekend too. The key realization this morning was with left-aligning zoomed nodes so we didn't need to animate padding.

Cool, cool. Thanks! 😄

bvaughn avatar Jul 16 '18 18:07 bvaughn

On my MBP this feels a bit choppier than master.

gaearon avatar Jul 16 '18 18:07 gaearon

Hm. I know it's difficult, but any chance you could elaborate?

This is essentially the same animation, but with fewer nodes. I wouldn't expect it to feel choppier. Maybe too fast? Or maybe I'm missing something.

Maybe the GPU acceleration is messing things up somehow? Everything looks really smooth when slowed down to 25% or 10%... 😅I am not very knowledgable here.

bvaughn avatar Jul 16 '18 18:07 bvaughn

It's really hard to show in a gif. I don't think it'll reproduce if you slow it down though. The choppiness happens right at the last few frames, as if it's skipping a few of them. I imagine that if you slow it down, they won't get skipped so it will appear fluid.

gaearon avatar Jul 16 '18 19:07 gaearon

Yeah, that lines up with what I'm seeing too.

bvaughn avatar Jul 16 '18 19:07 bvaughn

Out of morbid curiosity, how does this perform for you guys? https://react-flame-graph-xitkcoyjhr.now.sh/

vincentriemer avatar Jul 17 '18 00:07 vincentriemer

Master still feels smoother to me.

gaearon avatar Jul 17 '18 00:07 gaearon

Yeah I think this may be a case of SVG animations scaling better than divs.

I've anecdotally gotten better results with animating dom nodes instead of SVG but I've never really worked with animating this many elements at the same time 😅

vincentriemer avatar Jul 17 '18 00:07 vincentriemer

It's an interesting problem that I've been wrestling with in rn-dom as well. Theoretically, hardware acceleration would be the key to maximum animation performance, but in practice what ends up happening is it moves the bottleneck from the CPU to the GPU, and with devices like the MBP that default to using their integrated GPU (paired with the fact that Chrome's GPU performance on OSX seems to be very inefficient), it bottlenecks really quickly.

You can see it in the chrome profiler when you compare master:

screen shot 2018-07-17 at 9 18 33 am

to my version (which uses transform & opacity exclusively):

screen shot 2018-07-17 at 9 17 51 am

As further evidence, here's a modified deployment of my version that should perform better (though still not as well as master) because I added a little trick that makes Chrome activate the discrete GPU in MacOS: https://react-flame-graph-qfybxlwodr.now.sh

vincentriemer avatar Jul 17 '18 13:07 vincentriemer

That's interesting. Kind of a bummer, but interesting.

Maybe I'll apply my fill-filter approach to the SVG elements then, so we can at least remove the extra white-filled rect and opacity.

bvaughn avatar Jul 17 '18 14:07 bvaughn

Have you tried to not animate all the css properties? I wouldn’t animate height at least.

Also I had a few ideas which might be worth exploring:

  • render the bars with a transparent background and create and animate a pseudo element to mimic the animated bar effect.
  • Another one could be to avoid to set a width and compute left and right and animate those instead (or translate).
  • Finally did you try translate3d instead of translate (with 0 for the Z axis)?

giuseppeg avatar Jul 17 '18 16:07 giuseppeg

Have you tried to not animate all the css properties? I wouldn’t animate height at least.

Height never changes. But in most of my branches, I've been lazy and scoped transition to "all".

  • Finally did you try translate3d instead of translate (with 0 for the Z axis)?

No, but...I could?

bvaughn avatar Jul 17 '18 17:07 bvaughn

The versions I posted toggle will-change & only animates translate/scale/opacity via the Web Animations API but it's still not as fast as master so translate3d likely won't help 😞

vincentriemer avatar Jul 17 '18 20:07 vincentriemer