data-store graph walk restricted

[dwsutherland]

closes #5435

Check List

• I have read CONTRIBUTING.md and added my name as a Code Contributor.
• Contains logically grouped changes (else tidy your branch by rebase).
• Does not contain off-topic changes (use other PRs for other changes).
• Applied any dependency changes to both setup.cfg (and conda-environment.yml if present).
• Covered by existing tests.
• CHANGES.md entry included if this is a change that can affect users
• Cylc-Doc pull request opened if required at cylc/cylc-doc/pull/XXXX.
• If this is a bug fix, PR should be raised against the relevant ?.?.x branch.

[oliver-sanders]

Have repeated the reported result, this reduces the number of calls of increment_graph_window from 49021002/7001 to 14002/7001 for the example in #5435 🎉. Doubtless the single biggest complexity reduction for cylc-flow in a single change ever! There are 7001 tasks in the workflow causing the 7001 original calls, each makes two recursive calls giving the 14002 which makes sense.

The revised algorithm seems to be producing the same results, these screenshots come from the same workflow, installed & run under upstream/8.1.x and dwsutherland/graph-walk-efficiency with the same operations performed on both. The graph view displays the same results:

[dwsutherland]

The only down side is; increasing the window size will no longer flesh out the entire workflow graph, it will only show the the direct ancestors/descendants of active nodes.

consider:

a => b => d
c => d

if b is the only active task, expanding the n-window about it will no longer show c
(the walk no longer travels down to d then up to c, only down or only up)

But obviously, no down sides with n=1 .. And all the tests passed, so nothing missing there.

If we're happy with this trade-off, perhaps we should get this one in and work on the no-rewalk change separately.

[oliver-sanders]

Ah, right, this has removed the ability to generate the n>=1 window?

[dwsutherland]

Ah, right, this has removed the ability to generate the n>=1 window?

Nope, n>=1 is still there, but it will no longer travel downstream then upstream, or upstream then downstream.

(you won't see the cousins)

Will update the above comment

[oliver-sanders]

From a bit of experimentation with this pattern:

a => b<N> => c

The number of times increment_graph_window is called (recursive) scales according to:

Before	After
2N^2 + 3N + 1	5N + 1

This change removes the N^2 term which is the nasty one.

The factor of 5 in the new solution is a little odd, not sure where that's coming from. Would be great to reduce it if possible, but linear scaling is the important bit for now.

The lack of cousins in the graph window isn't great, however, this doesn't affect the N=1 window (default) and we don't currently make it easy for users to switch the n-window in the GUI so this shouldn't have any impact right now.

[oliver-sanders]

I think the performance is well worth the caveats, we can always improve later, thanks @dwsutherland!

[oliver-sanders]

(can drop in a changelog later)

[hjoliver]

we don't currently make it easy for users to switch the n-window in the GUI so this shouldn't have any impact right now.

Ha, note cylc/cylc-ui#1281

[hjoliver]

I think the performance is well worth the caveats, we can always improve later, thanks @dwsutherland!

Agreed, fantastic.

The only down side is; increasing the window size will no longer flesh out the entire workflow graph, it will only show the the direct ancestors/descendants of active nodes.

This may be OK for the n-window. For n=cycle we shouldn't need to walk out from active tasks.

[hjoliver]

On my laptop, it took 15s to spawn 7000 tasks on this branch vs 400s on master 🎉

[hjoliver]

Great, merging this now to get it in 8.1.3

@dwsutherland - as a small follow-up could you just add some code comments to explain the current functionality and the "cousins problem"?

[hjoliver]

(I'll push a hot-fix change log entry for this)