Skip to content
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

Closed
wants to merge 19 commits into from
Closed

Conversation

mischnic
Copy link
Member

@mischnic mischnic commented Jul 3, 2020

↪️ Pull Request

Benefits of symbol propagation (e.g. with calling resolveSymbol repeatedly in the bundler):

  1. Tree shaking across (shared) bundles
  2. Better/faster tree shaking with many namespace reexports (e.g. react component libraries)
  3. Not implemented in this PR: Tree shaking dynamic imports
  4. Not implemented in this PR: Somehow operate on symbols cross assets (e.g. skip TS type reexports)
  5. Not implemented in this PR (not sure how slower it would be): actually do the symbol resolution in the graph. Could catch conflicting reexports (spec compliant)

Benefits of assetgraphbuilder symbol propagation (doing it after building the asset graph):

  1. Exclude a subgraph of assets during bundling (e.g. to also exclude CSS imports of unused&side-effect free assets or not creating async bundles that are imported from dead assets)

This PR: Benefits of assetgraph(builder) symbol propagation (as opposed to doing it in the bundle graph):

  1. Incremental ("faster")
  2. Deferring is always correct (except with namespace reexports - chained named reexports do work)
  3. Way way slower...

What this PR implements:

  1. Propagate the actually used symbols down the asset graph (dependencyNode.usedSymbolsDown) by aligning imports with reexports as best as possible.
  2. To handle namespace reexports correctly (where the imports have to be distributed to every reexport), there is another pass that walks the finished asset graph in post-order and propagates the used & available symbols back up. (This is actually very fast - 80ms for the ak-editor. Could also expanded later to solve Parcel 2 fails to bundle TypeScript code that re-exports types #4796).
    So for every dependency node, usedSymbolsDown is a superset of the actual usedSymbolsUp (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 of symbol -> Set<Incoming Dependencies that caused this symbol request> to facilitate incremental updates

At 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 lost

Closes #4565

💻 Examples

ak-editor: 500k smaller but 10s 18s slower (this is the worst case, because almost everywhere in @atlaskit/*, named reexports are used)

this:
 build . --no-cache --no-source-maps --no-minify --log-level warn  288.47s user 42.57s system 383% cpu 1:26.36 total
 16015360	dist

v2:
 build . --no-cache --no-source-maps --no-minify --log-level warn  212.17s user 27.54s system 350% cpu 1:08.31 totaldist/  16527360 dist


building only the asset graph:
this:
 PARCEL_WORKERS=0  build . --no-cache --no-source-maps --no-minify --profile    151.91s user 33.03s system 152% cpu 2:01.12 total
v2:
 PARCEL_WORKERS=0  build . --no-cache --no-source-maps --no-minify --profile    93.44s user 19.82s system 152% cpu 1:14.16 total

@fluentui/react-northstart as a react component library with many namespace reexports (#4565)

$ time parcel2 build src/index.html --no-cache
✨ Built in 17.37s

dist/index.html               245 B    4.51s
dist/index.173315dd.js    232.84 KB    7.41s
 build src/index.html --no-cache  52.73s user 6.22s system 314% cpu 18.718 total
$ time parcel2 build src/index.html --no-cache
✨ Built in 28.39s

dist/index.html               245 B     1.35s
dist/index.b261fe70.js    1012.9 KB    14.82s
 build src/index.html --no-cache  78.04s user 9.98s system 286% cpu 30.757 total

✔️ PR Todo

  • Build size is non-deterministic !?!?
  • Is deleting a dependency, which causes a cascade (orphans), handled correctly?
  • Faster? - visit fewer nodes multiple times
    - Maybe keep the previous defer heuristic and be less aggressive about revisiting nodes ?
    - Maybe batch dirty dependencies?
  • Remove debug comments

@height
Copy link

height bot commented Jul 3, 2020

Link Height tasks by mentioning a task ID in the pull request title or description, commit messages, or comments.

💡Tip: You can also use "Close T-X" to automatically close a task when the pull request is merged.

Comment on lines +336 to +338
// TODO isIsolated?
if (incomingDep.value.isEntry || incomingDep.value.isIsolated) {
isEntry = true;
Copy link
Member Author

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)

Comment on lines +562 to +563
// TODO ???
let assetNode = this.getNode(asset.id) ?? nodeFromAsset(asset);
Copy link
Member Author

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?

@mischnic
Copy link
Member Author

mischnic commented Jul 5, 2020

ak-editor (--no-minify):

  • this (propagate during assetgraph building, meaning aggressive deferring):
    286.06s user 42.96s system 382% cpu 1:26.07 total
    16015360 bytes

  • alternative (old deferring heuristic, propagate after building assetgraph):
    211.91s user 28.20s system 366% cpu 1:05.57 total
    16117760 bytes
    (I have no idea how to make the bundler skip unused subgraphs, so it is likely possible to make it smaller)
    https://github.com/parcel-bundler/parcel/tree/assetgraph-propagate-symbols-fast

  • v2:
    219.03s user 28.07s system 340% cpu 1:12.59 total
    16527360 bytes

@mischnic mischnic mentioned this pull request Jul 8, 2020
3 tasks
@mischnic mischnic closed this Jul 8, 2020
@DeMoorJasper DeMoorJasper deleted the assetgraph-propagate-symbols branch November 12, 2020 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Parcel2 v. Webpack - parcel bundle size is significantly larger
1 participant