Page MenuHomePhabricator

Performance improvements based on fan-in-out example DAG
ClosedPublic

Authored by bengotow on Thu, Aug 22, 6:22 AM.

Details

Reviewers
prha
Group Reviewers
Restricted Project
Commits
R1:0180115e15bc: Performance improvements based on fan-in-out example DAG
Summary

This diff makes some performance improvements to the Pipeline Explorer because I noticed performance was garbage on the example fan-in-out DAG with a 150+ solids. Here's a quick breakdown:

  • The sidebar now shows at most 20 other invocations of the solid and a "Show All" button expands the rest. This eliminates ~500ms of click latency on fan-in-out caused by 500+ invocations of same solid rendering in the sidebar every time you click a solid.
  • Eliminates another 40ms of latency (50% of remaining) when clicking to a new solid in a large dag by replacing an expensive deep comparison of props in shouldComponentUpdate.
  • Fix a bug that caused the last frame of zoom in / zoom out animations to be dropped, resulting in the SVG being drawn at 0.98812 scale, which turns out is not great for performance.
  • Remove unnecessary <G> tags, unnecessary opacity="1" attributes, and some other things that were bloating the svg.
  • Call directly through to D3's points => svg path method and memoize the results for faster drawing of the connecting lines between solids. I experimented with different ways to do this even faster, but it turns out our current approach (one <path> per connection) is actually fairly ideal because the browser culls offscreen paths when you're zoomed in and only renders the ones partially visible.
  • This one is crazy. I found that when a solid was selected and the sidebar showing solid info was *scrollable*, panning and zooming the DAG was terribly slow. I have no idea why but by poking at this for a while I found that adding position:relative to the sidebar more than doubles pan performance in this specific case where the sidebar can scroll. Might need to file a Chromium issue against this one...

Overall Result:

  • Panning is very very fast when zoomed in
  • Clicking solids is much faster
  • Zoom in / zoom out animation is slightly faster but still sorta garbage on this DAG. The browser is spending almost all the time in the Composite Layers rendering phase and I think that the size of the rasterized svg image is just enormous. We will probably need to rasterize to a canvas and transform ourselves to fix this. (Which would give us more control over the scale that we rasterize the image at, eg not 100%)
Test Plan

Run tests

Diff Detail

Repository
R1 dagster
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

bengotow created this revision.Thu, Aug 22, 6:22 AM
bengotow edited the summary of this revision. (Show Details)Thu, Aug 22, 6:29 AM
bengotow edited the summary of this revision. (Show Details)Thu, Aug 22, 6:32 AM
bengotow updated this revision to Diff 3930.Thu, Aug 22, 6:40 AM
  • Fix up comments
prha accepted this revision.Thu, Aug 22, 8:20 PM
prha added a subscriber: prha.

small nit, but overall this looks great!

js_modules/dagit/src/graph/SolidNode.tsx
122

What do you think about adding if (prevPops.solid.name !== this.props.solid.name) return true;?

I know it's equivalent to the current behavior because we're using solid.name as the key in <PipelineGraph />, but it helps clarify in my head of the render semantics.

This revision is now accepted and ready to land.Thu, Aug 22, 8:20 PM
bengotow added inline comments.Thu, Aug 22, 9:48 PM
js_modules/dagit/src/graph/SolidNode.tsx
122

Ahh that does seem like a good idea, since this is silently taking advantage of those keys higher up. Will add!

bengotow updated this revision to Diff 3955.Thu, Aug 22, 9:50 PM
  • Add prop name to shouldComponentUpdate
This revision was landed with ongoing or failed builds.Thu, Aug 22, 9:51 PM
This revision was automatically updated to reflect the committed changes.