Skip to content

Avoid a second graph build when getting assets#1511

Closed
tido64 wants to merge 8 commits into
react:mainfrom
tido64:tido/avoid-second-traversal
Closed

Avoid a second graph build when getting assets#1511
tido64 wants to merge 8 commits into
react:mainfrom
tido64:tido/avoid-second-traversal

Conversation

@tido64

@tido64 tido64 commented Jun 3, 2025

Copy link
Copy Markdown
Contributor

Summary

@dannyvv recently discovered that Metro calls buildSubgraph() twice during bundling. Once during build(): https://github.com/facebook/metro/blob/15fef8ebcf5ae0a13e7f0925a22d4211dde95e02/packages/metro/src/index.flow.js#L431

And again, shortly after, in getAssets(): https://github.com/facebook/metro/blob/15fef8ebcf5ae0a13e7f0925a22d4211dde95e02/packages/metro/src/index.flow.js#L435

I think the second build should be mostly cached, but it would be better if we can avoid it altogether if possible.

Digging deeper down, it looks like we have the full graph inside build(), but throw away the work when returning. I've changed things so that it also returns the graph and made it possible to pass it to getAssets() to avoid the second graph build. I don't know if this is the best way to solve this, but it seems to work on my machine™️.

Changelog: [Performance] Avoid a second graph build when getting assets

Test plan

I've added console.log inside buildSubgraph to make sure it only gets called once per unique input.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jun 3, 2025
@tido64 tido64 force-pushed the tido/avoid-second-traversal branch from 44e064c to 08e7f76 Compare June 3, 2025 08:06
@facebook-github-bot facebook-github-bot added the Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. label Jun 3, 2025
@robhogan

Copy link
Copy Markdown
Contributor

Hi @tido64 - thanks for spotting this. Indeed the transform results would normally be cached (where a cache is configured), but resolutions wouldn't, so this is a useful optimisation.

Is it particularly the use of the runBuild API that you're looking at? The reason I ask - I'd prefer to avoid leaking the whole Graph from the shared/output/bundle, but I think we can tweak things so that it can return assets directly (and also benefit from parallelising serialisation and asset enumeration). I'd add a withAssets: boolean to RequestOptions and encapsulate building assets within Server.build.

Does it still work for you if I pull this in and make those changes?

@tido64

tido64 commented Jun 10, 2025

Copy link
Copy Markdown
Contributor Author

Does it still work for you if I pull this in and make those changes?

Yes, please! As long as we implement the optimization, please feel free to make any changes.

@facebook-github-bot

Copy link
Copy Markdown
Contributor

@robhogan has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot

Copy link
Copy Markdown
Contributor

@robhogan has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot

Copy link
Copy Markdown
Contributor

@robhogan merged this pull request in 261b8f3.

@tido64 tido64 deleted the tido/avoid-second-traversal branch June 18, 2025 11:44
jbroma pushed a commit to callstack/metro that referenced this pull request Jul 1, 2025
Summary:
dannyvv recently discovered that Metro calls [`buildSubgraph()`](https://github.com/facebook/metro/blob/15fef8ebcf5ae0a13e7f0925a22d4211dde95e02/packages/metro/src/DeltaBundler/buildSubgraph.js#L105) twice during bundling. Once during `build()`: https://github.com/facebook/metro/blob/15fef8ebcf5ae0a13e7f0925a22d4211dde95e02/packages/metro/src/index.flow.js#L431

And again, shortly after, in `getAssets()`: https://github.com/facebook/metro/blob/15fef8ebcf5ae0a13e7f0925a22d4211dde95e02/packages/metro/src/index.flow.js#L435

I *think* the second build should be mostly cached, but it would be better if we can avoid it altogether if possible.

Digging deeper down, it looks like we have the full graph inside `build()`, but throw away the work when returning. I've changed things so that it also returns the graph and made it possible to pass it to `getAssets()` to avoid the second graph build. I don't know if this is the best way to solve this, but it seems to work on my machine™️.

Changelog: [Performance] `runBuild`: Avoid a second graph build when getting assets

Pull Request resolved: react#1511

Test Plan: I've added `console.log` inside `buildSubgraph` to make sure it only gets called once per unique input.

Reviewed By: huntie

Differential Revision: D76342685

Pulled By: robhogan

fbshipit-source-id: d66b6dbe73b4130c140b0a78fd4ed7e53d7a18c3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants