-
Notifications
You must be signed in to change notification settings - Fork 6
Optimise isAssetReferenced in packager #858
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
Conversation
🦋 Changeset detectedLatest commit: 33cd346 The changes in this PR will be included in the next version bump. This PR includes changesets to release 108 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📊 Type Coverage ReportCoverage Comparison
Files with Most Type Issues (Top 15)
This report was generated by the Type Coverage GitHub Action |
8b00720 to
793231e
Compare
Implements a three-tier optimization strategy for isAssetReferenced calls: 1. **Cache-first lookup** (~0.001ms) - Check expensive computation cache first 2. **Fast checks fallback** (~0.01ms) - Only when cache misses, try quick checks 3. **Expensive computation** (~20-2000ms) - Only when both above fail, with caching **Key Changes:** - Add `isAssetReferencedFastCheck()` method to BundleGraph for fast-only checks - Update ScopeHoistingPackager to use cache-first strategy - Separate fast operations from expensive ones for optimal performance **Technical Details:** - Only expensive bundle traversals get cached results - Cache hit rates: Jira ~10%, Confluence ~95-100% - Reduces cache overhead for operations that are cheaper to compute than cache This resolves performance regressions in Jira while maintaining Confluence optimization benefits through adaptive caching strategy.
fe4be40 to
33cd346
Compare
📊 Benchmark Results🎉 Performance improvements detected! 📊 Benchmark ResultsOverall Performance
🔍 Detailed Phase AnalysisThree.js Real Repository (JS)
Three.js Real Repository (Native)
💾 Unified Memory AnalysisThree.js Real Repository (JS) Memory Statistics
Sample Counts: JS: 6, Native: 271 Three.js Real Repository (Native) Memory Statistics
Sample Counts: JS: 6, Native: 517 🖥️ Environment
|
| asset.meta.shouldWrap || | ||
| this.bundle.env.sourceType === 'script' || | ||
| this.bundleGraph.isAssetReferenced(this.bundle, asset) || | ||
| this.isAssetReferencedInBundle(this.bundle, 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.
Nit: Passing this.bundle is a bit odd. I'd just reference it directly via this in isAssetReferencedInBundle
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.
Ahh I see the bundle changes in some cases. Ignore me 😅
mattcompiles
left a comment
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.
Nice one mate
Motivation
Looking at some product SSR bundling builds shows that a significant amount of time is spent in
bundleGraph.isAssetReferencedduring packaging. This is inefficient as there are overlapping repeated traversals for different assets during the build. In particular this results in a lot of GC, andclear-ing of the visited BitSet used in the graph traversal.This isn't true across all products and all build types though, it seems to be endemic to some builds only. It's still slightly faster for the builds where this wasn't really an issue just not to the same extent (e.g. below - Confluence is 18%±5% faster, Jira is 4%±2% faster)
Also worth noting that for local development the impact will only be seen for SSR, as the ScopeHoistingPackager is not used for FE builds in dev.
Benchmark results:
Confluence SSR:
ENABLE_OPTIMISATION=falseENABLE_OPTIMISATION=trueConfluence FE:
ENABLE_OPTIMISATION=falseENABLE_OPTIMISATION=trueJira SSR:
ENABLE_OPTIMISATION=falseENABLE_OPTIMISATION=trueJira FE:
ENABLE_OPTIMISATION=falseENABLE_OPTIMISATION=trueChanges
Instead of traversing per asset on demand, we take a multi phase approach:
This is behind a new
precomputeReferencedAssetsfeature flag.This approach is faster for all builds that were tested.
Checklist
docs/folder