perf: cut redundant per-turn state serialization - #33
Merged
Conversation
The runtime built the whole actor state image eight times for each committed operation, and nine times for a query. Two are necessary. At the default state limit the repeated work dominated the turn. Pass the image the commit path already holds to the observables guard, and read the committed image for the query mutation check. The guard is unchanged: it still reads the state after observables() returns, because only that read sees a mutation. Both counts are now four. Compute the default state once for each registered class. Every send and every hydration built a throwaway actor and serialized its defaults. The constructor must not depend on external state, so one cached image per validated definition is correct. Each caller parses a detached copy. Stop building the string normalizeJson discards when no byte limit is given, which is every call from actorState, deepCopy, and stableJson. Add warnStateBytes, because maxStateBytes advertised an operating point the runtime does not support. The 5 MB hard default stands, and a commit above the 128 KB soft threshold now reports one instrumentation event with the actor type, actor ID, and byte count, so an operator learns the constraint before an application meets it. Closes #32
Greptile SummaryThe PR reduces redundant actor-state serialization, caches detached default-state images, and adds instrumentation for large committed states.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains; both previously reported issues are fixed at the current head. Important Files Changed
Reviews (2): Last reviewed commit: "fix: report large state only after the c..." | Re-trigger Greptile |
A new configuration option, a new instrumentation event, and a new benchmark match what 0.13.1 shipped as a patch, so this is a patch.
The warning ran before repository.complete, so a turn that lost its activation or failed in a commit action reported a size for state the runtime rolled back and never wrote. Use an early return for the byte limit in normalizeJson.
Owner
Author
|
@greptileai review Both review comments are addressed in b66d23d:
Please re-review at b66d23d. |
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
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.
Closes #32.
What changed
The runtime built the whole actor state image 8 times for each committed
operation, and 9 times for a query. Two are necessary. Both counts are now 4.
1.
normalizeJsonno longer builds a string it discardssrc/serialization.tscalledJSON.stringifyon every value, then used theresult only when
maxByteswas given.actorState,deepCopy, andstableJsonall pass no limit.2.
initialStateForcomputes the default state onceEvery
sendand every hydration constructed a throwaway actor and serializedits full default state. The constructor must not depend on external state, so
one cached image per validated definition is correct. The cache holds the
encoded image, so each caller parses a detached copy and cannot reach the
cache.
3.
readObservablesaccepts the caller's state imageBoth call sites sit next to a place where the runtime already holds the image,
so the image is now passed in. The query mutation check reads the committed
image instead of taking one of its own.
4.
warnStateBytesA soft threshold that defaults to 128 KB. A commit above it reports one
solid_objects.state.largeinstrumentation event with the actor type, theactor ID, the byte count, and the threshold. The event holds no application
state, and the runtime measures the size only when an
instrumentationcallback is configured.
maxStateByteskeeps its 5 MB hard default.Measured
pnpm run benchmark:large-stateis new: one actor, oneincrement(),sequential turns. Apple M5, macOS 26.6, Node.js 24.18.0, SQLite 3.53.1 through
node:sqlite. Median of three runs of 300 operations.One correction to the issue
The issue sets Goal 1 as "8 to 2". Four is the floor while the guard stays
exact. The caller can supply the image from before
observables()runs, butthe image from after it must be a fresh read, or the guard cannot see a
mutation. The issue's own acceptance criterion says the same: "The guard still
compares the state at the moment
observables()runs, not an earlier image."The turn holds only two necessary images, and the operation runs between them,
so neither one can serve as the after-image for either guard. Every acceptance
criterion in the issue is met; only that headline number changes.
Tests
test/state-serialization.test.tsis new and was written first. Againstmainits traversal tests report 8 and 9, which reproduces the issue's evidence
table. It covers:
normalizeJsondoes not encode without a byte limit, and still raisesPayloadTooLargeat one.initialStateForcomputes once per validated definition and returns adetached copy.
actor ID, and byte count, holds no application state, and stays silent below.
The existing observables and query guard tests in
test/correctness.test.tsand
test/realtime.test.tspass unchanged.pnpm run checkpasses.pnpm testreports 1038 passed. Two failures remainin
.claude/worktrees/, which are stale gitignored checkouts that vitestscans; they fail on
maintoo and no file in this branch touches them.One behavior change to note
Building the rollback image before the first
readObservablescall means anobservables()implementation that mutates state now has that mutation rolledback in the cached activation, where before the turn failed without a rollback.
Parity
docs/parity.mdrecords the new warning and points atcardmagic/solid-objects-ruby#57 for the Ruby port, which the issue tracks
separately.
Release
This PR is
0.14.4.package.json,src/version.ts, and the datedCHANGELOG.mdsection are updated, andscripts/release-notes.mjs 0.14.4resolves that section. The
CHANGELOG.mdentry also carries the README anddocs prose that was already sitting in
Unreleased, because it ships in thesame version.
A patch matches this repository's own precedent.
0.13.1shipped a newconfiguration option, a new instrumentation event, and a new benchmark as a
patch, which is the shape of this change. The lockfile does not record the
package's own version, so it needs no refresh.
Follow the rest of
docs/releasing.mdafter merge: run the full releasecommand list, then tag
v0.14.4onmain.