-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Assetgraph propagate symbols #4841
Conversation
|
bad7b73
to
e0758c4
Compare
// TODO isIsolated? | ||
if (incomingDep.value.isEntry || incomingDep.value.isIsolated) { | ||
isEntry = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The toplevel asset is special because it has no incoming dependencies with symbols and so it would simply be deferred. Not sure if isEntry || isIsolated
is correct here (JS entry files, JS script tags)
// TODO ??? | ||
let assetNode = this.getNode(asset.id) ?? nodeFromAsset(asset); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously, assetNode.usedSymbols
was being discarded/overwritten - is this the correct solution?
ak-editor (
|
↪️ Pull Request
Benefits of symbol propagation (e.g. with calling resolveSymbol repeatedly in the bundler):
Benefits of assetgraphbuilder symbol propagation (doing it after building the asset graph):
This PR: Benefits of assetgraph(builder) symbol propagation (as opposed to doing it in the bundle graph):
What this PR implements:
dependencyNode.usedSymbolsDown
) by aligning imports with reexports as best as possible.So for every dependency node,
usedSymbolsDown
is a superset of the actualusedSymbolsUp
(which is exposed in the public BundleGraph )Now comes the nasty part: When a dependency is added/removed (and the asset is resolves to is already in the graph), the dependency node (and part of the subgraph) has to be walked to add/remove symbols, this causes some nodes to be visited multiple times (
{ downVisitedCount: 27699, upVisitedCount: 16001 }
,upVisitedCount
is the true number of nodes).This is what causes a slowdown of 10 seconds when building the asset graph for ak-editor.
usedSymbolsDown
is actually a map ofsymbol -> Set<Incoming Dependencies that caused this symbol request>
to facilitate incremental updatesAt the moment, the build size is not deterministic 😬 (the builds seem to be working however) - I'm guessing some updates to the used symbols are lostCloses #4565
💻 Examples
ak-editor: 500k smaller but
10s18s slower (this is the worst case, because almost everywhere in@atlaskit/*
, named reexports are used)@fluentui/react-northstart
as a react component library with many namespace reexports (#4565)✔️ PR Todo
- Maybe keep the previous defer heuristic and be less aggressive about revisiting nodes ?
- Maybe batch dirty dependencies?