Collapse NoopKvDb into an empty read-only TurboPersistence#93983
Draft
lukesandberg wants to merge 2 commits into
Draft
Collapse NoopKvDb into an empty read-only TurboPersistence#93983lukesandberg wants to merge 2 commits into
lukesandberg wants to merge 2 commits into
Conversation
`noop_backing_storage()` now returns the same concrete type as the real `turbo_backing_storage()` — `KeyValueDatabaseBackingStorage<TurboKeyValueDatabase>` — built on an empty, read-only `TurboPersistence` instance that owns no on-disk state and never touches the filesystem. This eliminates the `Either<TurboBackingStorage, NoopBackingStorage>` wrappers at the napi binding and turbopack-cli call sites, removes one generic instantiation of every kv-backing-storage helper, and replaces the silent-drop write behavior of `NoopKvDb` with a loud bail via the existing `read_only` guard. `TurboPersistence::new` takes a new `empty` parameter that allocates the block caches with zero-sized buckets, avoiding the ~2 MiB of hash-table headroom they would otherwise reserve on the noop instance.
`TurboKeyValueDatabase` is now the only implementor of this trait, so the default impls served no purpose. Making every method required keeps new implementors honest about what they're providing.
Contributor
Author
|
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. |
Contributor
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
Server Middleware
Build DetailsBuild Manifests
📦 WebpackClient Main Bundles
Polyfills
Pages
Server Edge SSR
Middleware
Build DetailsBuild Manifests
Build Cache
🔄 Shared (bundler-independent)Runtimes
📎 Tarball URLCommit: 839abfa |
Contributor
Failing test suitesCommit: 839abfa | About building and testing Next.js
Expand output● yarn PnP › should compile and serve the index page correctly with-eslint
Expand output● yarn PnP › should compile and serve the index page correctly with-eslint
Expand output● yarn PnP › should compile and serve the index page correctly with-eslint
Expand output● yarn PnP › should compile and serve the index page correctly with-eslint
Expand output● yarn PnP › should compile and serve the index page correctly with-eslint
Expand output● yarn PnP › should compile and serve the index page correctly with-eslint Other failing CI jobs |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What?
noop_backing_storage()is now an empty, read-only instance of the realTurboPersistenceinstead of a separateNoopKvDbimpl. Same concrete type asturbo_backing_storage(), so theEither<TurboBackingStorage, NoopBackingStorage>wrappers in napi and turbopack-cli go away.Why?
This is a prefactor for #93910 (devirt of the
TurboTasksApidispatch). That branch needs every caller to produce one concrete handle type so the*const ()→&Concretecast is well-defined; theEithermade that impossible.Tradeoff
We trade one
Either/match in every backing-storage method for oneis_emptyatomic load (RelaxedAtomicBool) onshould_restoreat startup. The atomic is once per startup andshould_restoreisn't on the hot path; theEitherwas on every read/write call. Net win.Also dropped:
NoopKvDb(had no behavior of its own — all trait defaults), theeitherworkspace dep from two crates, and one generic instantiation of everykv_backing_storagehelper. Writes to the noop now bail loudly viaread_onlyinstead of silently dropping.Second commit removes the default impls from
KeyValueDatabasesinceTurboKeyValueDatabaseis now the only implementor.