-
Notifications
You must be signed in to change notification settings - Fork 30.4k
Remove task snapshots from persistence #89412
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
base: optimized_change_tracking
Are you sure you want to change the base?
Remove task snapshots from persistence #89412
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
turbopack/crates/turbo-tasks-macros/src/derive/task_storage_macro.rs
Outdated
Show resolved
Hide resolved
CodSpeed Performance ReportMerging this PR will not alter performanceComparing Summary
Footnotes
|
f71fd0f to
c3343d0
Compare
Stats from current PR✅ No significant changes detected📊 All Metrics📖 Metrics GlossaryDev Server Metrics:
Build Metrics:
Change Thresholds:
⚡ Dev Server
📦 Dev Server (Webpack) (Legacy)📦 Dev Server (Webpack)
⚡ Production Builds
📦 Production Builds (Webpack) (Legacy)📦 Production Builds (Webpack)
📦 Bundle SizesBundle Sizes⚡ TurbopackClient Main Bundles: **434 kB** → **435 kB**
|
| Canary | PR | Change | |
|---|---|---|---|
| middleware-b..fest.js gzip | 763 B | 764 B | ✓ |
| Total | 763 B | 764 B |
Build Details
Build Manifests
| Canary | PR | Change | |
|---|---|---|---|
| _buildManifest.js gzip | 449 B | 451 B | ✓ |
| Total | 449 B | 451 B |
📦 Webpack
Client
Main Bundles
| Canary | PR | Change | |
|---|---|---|---|
| 5528-HASH.js gzip | 5.47 kB | N/A | - |
| 6280-HASH.js gzip | 54.5 kB | N/A | - |
| 6335.HASH.js gzip | 169 B | N/A | - |
| 912-HASH.js gzip | 4.53 kB | N/A | - |
| e8aec2e4-HASH.js gzip | 62.5 kB | N/A | - |
| framework-HASH.js gzip | 59.7 kB | 59.7 kB | ✓ |
| main-app-HASH.js gzip | 254 B | 253 B | ✓ |
| main-HASH.js gzip | 39 kB | 39.1 kB | ✓ |
| webpack-HASH.js gzip | 1.68 kB | 1.68 kB | ✓ |
| 262-HASH.js gzip | N/A | 4.52 kB | - |
| 2889.HASH.js gzip | N/A | 169 B | - |
| 5602-HASH.js gzip | N/A | 5.48 kB | - |
| 6948ada0-HASH.js gzip | N/A | 62.5 kB | - |
| 9544-HASH.js gzip | N/A | 55.2 kB | - |
| Total | 228 kB | 229 kB |
Polyfills
| Canary | PR | Change | |
|---|---|---|---|
| polyfills-HASH.js gzip | 39.4 kB | 39.4 kB | ✓ |
| Total | 39.4 kB | 39.4 kB | ✓ |
Pages
| Canary | PR | Change | |
|---|---|---|---|
| _app-HASH.js gzip | 194 B | 194 B | ✓ |
| _error-HASH.js gzip | 183 B | 180 B | 🟢 3 B (-2%) |
| css-HASH.js gzip | 331 B | 330 B | ✓ |
| dynamic-HASH.js gzip | 1.81 kB | 1.81 kB | ✓ |
| edge-ssr-HASH.js gzip | 256 B | 256 B | ✓ |
| head-HASH.js gzip | 351 B | 352 B | ✓ |
| hooks-HASH.js gzip | 384 B | 383 B | ✓ |
| image-HASH.js gzip | 580 B | 581 B | ✓ |
| index-HASH.js gzip | 260 B | 260 B | ✓ |
| link-HASH.js gzip | 2.49 kB | 2.49 kB | ✓ |
| routerDirect..HASH.js gzip | 320 B | 319 B | ✓ |
| script-HASH.js gzip | 386 B | 386 B | ✓ |
| withRouter-HASH.js gzip | 315 B | 315 B | ✓ |
| 1afbb74e6ecf..834.css gzip | 106 B | 106 B | ✓ |
| Total | 7.97 kB | 7.97 kB | ✅ -1 B |
Server
Edge SSR
| Canary | PR | Change | |
|---|---|---|---|
| edge-ssr.js gzip | 126 kB | 126 kB | ✓ |
| page.js gzip | 249 kB | 249 kB | ✓ |
| Total | 375 kB | 375 kB |
Middleware
| Canary | PR | Change | |
|---|---|---|---|
| middleware-b..fest.js gzip | 616 B | 613 B | ✓ |
| middleware-r..fest.js gzip | 156 B | 155 B | ✓ |
| middleware.js gzip | 33.1 kB | 33.1 kB | ✓ |
| edge-runtime..pack.js gzip | 842 B | 842 B | ✓ |
| Total | 34.7 kB | 34.8 kB |
Build Details
Build Manifests
| Canary | PR | Change | |
|---|---|---|---|
| _buildManifest.js gzip | 732 B | 736 B | ✓ |
| Total | 732 B | 736 B |
Build Cache
| Canary | PR | Change | |
|---|---|---|---|
| 0.pack gzip | 3.8 MB | 3.81 MB | 🔴 +14.4 kB (+0%) |
| index.pack gzip | 102 kB | 104 kB | 🔴 +1.87 kB (+2%) |
| index.pack.old gzip | 102 kB | 102 kB | ✓ |
| Total | 4 MB | 4.02 MB |
🔄 Shared (bundler-independent)
Runtimes
| Canary | PR | Change | |
|---|---|---|---|
| app-page-exp...dev.js gzip | 311 kB | 311 kB | ✓ |
| app-page-exp..prod.js gzip | 166 kB | 166 kB | ✓ |
| app-page-tur...dev.js gzip | 311 kB | 311 kB | ✓ |
| app-page-tur..prod.js gzip | 166 kB | 166 kB | ✓ |
| app-page-tur...dev.js gzip | 308 kB | 308 kB | ✓ |
| app-page-tur..prod.js gzip | 164 kB | 164 kB | ✓ |
| app-page.run...dev.js gzip | 308 kB | 308 kB | ✓ |
| app-page.run..prod.js gzip | 164 kB | 164 kB | ✓ |
| app-route-ex...dev.js gzip | 70.4 kB | 70.5 kB | ✓ |
| app-route-ex..prod.js gzip | 48.9 kB | 49 kB | ✓ |
| app-route-tu...dev.js gzip | 70.4 kB | 70.5 kB | ✓ |
| app-route-tu..prod.js gzip | 49 kB | 49 kB | ✓ |
| app-route-tu...dev.js gzip | 70 kB | 70.1 kB | ✓ |
| app-route-tu..prod.js gzip | 48.7 kB | 48.8 kB | ✓ |
| app-route.ru...dev.js gzip | 70 kB | 70.1 kB | ✓ |
| app-route.ru..prod.js gzip | 48.7 kB | 48.7 kB | ✓ |
| dist_client_...dev.js gzip | 324 B | 324 B | ✓ |
| dist_client_...dev.js gzip | 326 B | 326 B | ✓ |
| dist_client_...dev.js gzip | 318 B | 318 B | ✓ |
| dist_client_...dev.js gzip | 317 B | 317 B | ✓ |
| pages-api-tu...dev.js gzip | 43.1 kB | 43.2 kB | ✓ |
| pages-api-tu..prod.js gzip | 32.9 kB | 32.9 kB | ✓ |
| pages-api.ru...dev.js gzip | 43.1 kB | 43.2 kB | ✓ |
| pages-api.ru..prod.js gzip | 32.8 kB | 32.9 kB | ✓ |
| pages-turbo....dev.js gzip | 52.4 kB | 52.5 kB | ✓ |
| pages-turbo...prod.js gzip | 39.4 kB | 39.4 kB | ✓ |
| pages.runtim...dev.js gzip | 52.4 kB | 52.5 kB | ✓ |
| pages.runtim..prod.js gzip | 39.3 kB | 39.4 kB | ✓ |
| server.runti..prod.js gzip | 62.6 kB | 62.6 kB | ✓ |
| Total | 2.77 MB | 2.78 MB |
📝 Changed Files (25 files)
Files with changes:
app-page-exp..ntime.dev.jsapp-page-exp..time.prod.jsapp-page-tur..ntime.dev.jsapp-page-tur..time.prod.jsapp-page-tur..ntime.dev.jsapp-page-tur..time.prod.jsapp-page.runtime.dev.jsapp-page.runtime.prod.jsapp-route-ex..ntime.dev.jsapp-route-ex..time.prod.jsapp-route-tu..ntime.dev.jsapp-route-tu..time.prod.jsapp-route-tu..ntime.dev.jsapp-route-tu..time.prod.jsapp-route.runtime.dev.jsapp-route.ru..time.prod.jspages-api-tu..ntime.dev.jspages-api-tu..time.prod.jspages-api.runtime.dev.jspages-api.ru..time.prod.js- ... and 5 more
View diffs
app-page-exp..ntime.dev.js
failed to diffapp-page-exp..time.prod.js
Diff too large to display
app-page-tur..ntime.dev.js
failed to diffapp-page-tur..time.prod.js
Diff too large to display
app-page-tur..ntime.dev.js
failed to diffapp-page-tur..time.prod.js
Diff too large to display
app-page.runtime.dev.js
failed to diffapp-page.runtime.prod.js
Diff too large to display
app-route-ex..ntime.dev.js
Diff too large to display
app-route-ex..time.prod.js
Diff too large to display
app-route-tu..ntime.dev.js
Diff too large to display
app-route-tu..time.prod.js
Diff too large to display
app-route-tu..ntime.dev.js
Diff too large to display
app-route-tu..time.prod.js
Diff too large to display
app-route.runtime.dev.js
Diff too large to display
app-route.ru..time.prod.js
Diff too large to display
pages-api-tu..ntime.dev.js
Diff too large to display
pages-api-tu..time.prod.js
Diff too large to display
pages-api.runtime.dev.js
Diff too large to display
pages-api.ru..time.prod.js
Diff too large to display
pages-turbo...ntime.dev.js
Diff too large to display
pages-turbo...time.prod.js
Diff too large to display
pages.runtime.dev.js
Diff too large to display
pages.runtime.prod.js
Diff too large to display
server.runtime.prod.js
Diff too large to display
Tests Passed |
We snapshot tasks to ensure that we only hold the the dashmap locks for a short amount of time. However, profiling has revealed that we spend ~22% of the time in `save_snapshot` calling these clone methods, and 28% of the time encoding task storage values. So by dropping the clones we save a lot of time and end up holding locks a little bit longer. These are read locks on the dashmap so we will block contending writers (which would be about every turbo task execution that races with persistence in dev). This of course was already the case but now we do it for slightly longer.
3f34bea to
5e371ae
Compare

Don't clone TaskStorage objects when snapshotting the database
What
Instead of cloning TaskStorage objects when preparing to snapshot we directly encode the reference in the backend storage map.
Why?
From a performance profile of save_snapshot i observed that ~22% of the time was spent in
clone_meta_snapshotorclone_data_snapshot, while ~28% of the time was spent inencode_task_data. The whole reason we are cloning is so we can drop the lock on the dashmap shard while encoding. In principle this makes sense, however:track_modification. So slowing them down isn't too risky.For a site like vercel-site there are ~8M tasks and 65K shards, so each shard has ~128 items. So the worst case would be waiting for them all to flush and 128 isn't so many that the task would be blocked for a very long time.
So this should be a pure win, reduced allocations in the encoding path.