Page MenuHomePhabricator

Add web worker to calculate dagre layout for large graphs
ClosedPublic

Authored by prha on Dec 13 2019, 9:39 PM.

Details

Summary

Hooks based approaches use useEffect, which only runs side effects *after* the first render call

Test Plan

Rendered multiple pipelines in Explore

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

prha created this revision.Dec 13 2019, 9:39 PM
prha edited the summary of this revision. (Show Details)Dec 13 2019, 9:40 PM
alangenfeld requested changes to this revision.Dec 13 2019, 9:48 PM

something odd is happening on big dags - I patched this and tested it against prezi and it kept going back to the loading spinner as i panned around

This revision now requires changes to proceed.Dec 13 2019, 9:48 PM
prha updated this revision to Diff 7710.Dec 13 2019, 11:15 PM

add back memoization for the async dagre layout call

prha planned changes to this revision.Dec 13 2019, 11:51 PM

Need to mock out jest config to support web workers

prha updated this revision to Diff 7728.Dec 14 2019, 2:03 AM

rebased, added jest mocks

prha updated this revision to Diff 7754.Dec 15 2019, 12:54 AM

fix unit tests, handle serving worker js in production mode

prha updated this revision to Diff 7755.Dec 15 2019, 1:00 AM

rebase

bengotow accepted this revision.EditedDec 15 2019, 9:38 PM

I'd defer to @alangenfeld but this looks great! Approved but with a couple inline comments - would definitely be good to double check and make sure the caching is working the way we intend.

js_modules/dagit/src/graph/PipelineGraphContainer.tsx
50

Since this method is defined (and redefined) on each render and the useEffect re-runs whenever solids / parentSolid changes, I don't think it's necessary for this method to have arguments - it could just pull the values from the parent scope? I'd lean toward removing them because having the args implies there might be code paths that pass other values.

54

I wonder if this function should start with a setLoading(true) type of thing?

58

I think the ASYNC_LAYOUT_SOLID_COUNT probably solves this, but if we run into scenarios where the UI "flickers" into the loading state briefly and it looks bad, we could also force the loading state for 400ms before calling setLayout / setLoading here. (Eg: track how much time elapsed during the work and set a timer for the remaining milliseconds). Even though that technically makes it slower, it might feel better than the flickering. ASYNC_LAYOUT_SOLID_COUNT should work I think, but the right value might vary a lot based on the machine.

js_modules/dagit/src/graph/getFullSolidLayout.ts
25

I think this caching is probably broken currently because the callsite passes a short lived object as the first param. The weakmap actually relies on the object identity, so:

const var = {a: 1};
cache.set(var, "associated-value");
cache.has(var) // works

cache.set({a: 1}, "associated-value");
cache.has({a: 1}) // doesn't work - second {a: 1} is a different object

It's definitely subtle, but in the original version we were passing the graphql response handles / solids in ( I think...), so the cached value would be re-used until the query was remade.

js_modules/dagit/src/workers/dagre_layout.worker.ts
7

Oh hey that's pretty straighforward!

js_modules/dagit/yarn.lock
3756

Ahh thank you for adding this I've been missing flat ๐Ÿ˜…

prha added inline comments.Dec 16 2019, 1:14 AM
js_modules/dagit/src/graph/PipelineGraphContainer.tsx
50

yep, that sounds good.

54

oh yeah... good catch!

I think previously I had this in a componentWillReceiveProps and only set loading when the pipeline changed (to avoid the startling loading flicker), but did a bad conversion to hooks.

js_modules/dagit/src/graph/getFullSolidLayout.ts
25

ahhh, you're right... adding the level of indirection / processing the graphql handles skips this memoization (and the checks in the useEffect call).

changing this to use memoization via normal LRU cache instead of a WeakMap and using a custom hashing function.

prha updated this revision to Diff 7777.Dec 16 2019, 1:36 AM

address caching errors

alangenfeld accepted this revision.Dec 16 2019, 5:48 PM

thumbsup

js_modules/dagit/src/graph/getFullSolidLayout.ts
3

you should probably put a comment here tho

This revision is now accepted and ready to land.Dec 16 2019, 5:48 PM
prha updated this revision to Diff 7808.Dec 16 2019, 5:55 PM

add comment about webpack loader syntax

This revision was automatically updated to reflect the committed changes.