Library cycle graph loader recursion support #3966
Conversation
PR Health |
davidmorgan
force-pushed
the
parallel-resolve-three
branch
2 times, most recently
from
8c57097 to
b240361
Compare
davidmorgan
force-pushed
the
parallel-resolve-three
branch
from
b240361 to
63461ad
Compare
| _buildCycles(assetDepsLoader.phase); | ||
| return _cycles[id]!; | ||
| } finally { | ||
| _runningAtPhases.removeLast(); |
There was a problem hiding this comment.
I assume this is assumed to be the same as the one added above..? Maybe assert that?
There was a problem hiding this comment.
Good idea, thanks, done.
Removed the try/finally since the linter doesn't like me throwing inside a "finally", the code isn't actually designed to work when there are exceptions anyway.
| final Map<int, Set<AssetId>> _assetDepsToLoadByPhase = {}; | ||
| /// The `key` is the phase to load them at or after. A [SplayTreeMap] is used | ||
| /// to keep the keys sorted so earlier phases are processed first. | ||
| final SplayTreeMap<int, List<AssetId>> _idsToLoad = SplayTreeMap(); |
There was a problem hiding this comment.
Where is the sorted order used? I can't seem to find it.
There was a problem hiding this comment.
It's implicit in _nextIdToLoad, tweaked the comment to mention it.
There was a problem hiding this comment.
Yeah, it does _idsToLoad.entries.where((entry) => entry.key <= upToPhase).first; --- though wouldn't that just be a slow way to do _idsToLoad.entries.first then (well, and a check that i is in fact <= upToPhase)?
For #3811.
Trying to use this on real builds quickly shows that it does not quite work yet, it needs to support reads triggering builds of earlier phase assets.
So, do that :) ... the main trick being to change the local "next IDs" list into a class-level list by phase, so recursive runs at earlier phases can finish the pending work that they need.
In doing so, simplify a bit: remove
_newAssets, use_graphsToComputeAtPhasefor phase 0 instead.And expand test coverage to include recursive loads.