Repository navigation
perf(runtime): bound the waits past stale data and in front of a Server Action - #444
Open
soleil-colza wants to merge 5 commits into
Open
soleil-colza wants to merge 5 commits into
soleil-colza wants to merge 5 commits into
Conversation
A read that judges a data value stale asks the host for a later one (`laterIfAny`), and was meant to wait 200 ms at the most for it, 500 ms past an expired value. Each step had a budget of its own instead: the read of the later value (200 or 500 ms), its judgement (200 ms), the read past it where it stood no better than stale (200 or 500 ms) and that one's judgement (200 ms), and then, where a write had overtaken the value, a read of the key again and its judgement with no deadline at all. That came to some 800 ms in front of the render past a stale value and 1.4 s past an expired one, before the first byte of a page with no shell; where the host keeps the later value far off, its read alone can spend the whole 200 ms. The deadline is now set once, as the read asks past the value it judged, and each step is given what is left of it: the read of a later value and its judgement; the read past it, which is not begun once the deadline has gone; and the read of the key again and its judgement, which answer a miss where they do not come in time. A later value the host answers after the runtime stopped waiting is still kept as the data center's copy: the host keeps that going behind the response, and nothing the runtime gives up calls it off. `later-generations.test.ts` covers a later value read in 150 ms whose judgement takes 100 ms (the value read is answered at 200 ms), no read past a later value once the deadline has gone, and a read of the key again that does not come in time (a miss at 200 ms).
…oads A Server Action posted to an App Router page is handed the postponed state of the generation the page was answered from (`actionPostponedFor`): a read of that generation's record, up to 200 ms and as long again for a newer one (`currentGeneration`). It was asked for only once the route's module had been evaluated (`renderedBy`, which evaluates it eagerly, on purpose), though it needs nothing of the module — so the two came to their sum in front of the action's answer. The read is now asked for as the module is evaluated, under the route `renderedBy` comes to, read off the entry tables as it reads them (`routeRenderedBy`): the resolved route where an entrypoint of either runtime has it, else the route its prerender names. Only for an action that is handed state, of a Node.js entrypoint, with a cache; an edge page's action is handed none and reads nothing. It is taken where it was asked for under the route the module renders, and asked for then otherwise; a failure it meets is caught as it is asked, since the module's evaluation can fail first, and is the action's where it is taken, as before. The read is read-only and kept for the requests after it, so one not taken costs nothing more. `action-postponed.test.ts` covers the record read begun before the module is evaluated, the same under the route a member without a module of its own renders under, and no read for an edge page's action.
What a value of the data cache an isolate keeps counts against its memo was its bytes and its answer written out as JSON (`dataBytes`). An answer that carries its value inline carries it as base64, so keeping a value copied all of it into a string of its own only to measure it, on the path of the read that keeps it: 0.6 ms for a value of 512 KiB, 9.7 ms for one of 8 MiB. The answer is now counted from the lengths of its strings — its ids, its handler, each tag with its punctuation, and the value's base64 or the artifact id that stands for it — beside an allowance of 512 characters for its keys, punctuation, kinds and numbers (some 360 with every field set). It counts in the same characters, two bytes each, so a value of the same size takes the same room as before: an answer of ordinary characters is never counted for less than its JSON. `data-memo-bytes.test.ts` covers a value kept inline and one kept as an artifact, each counted with nothing written as JSON.
…s over Bounding everything asked past a stale or expired data value by one deadline left what the isolate holds of the key, read again where a write overtook the later value (`heldNow`), refused outright once the deadline had gone by: a miss, which the render computes again — as a rule more slowly than the write the isolate held in hand. Past the deadline, that read and its judgement are now begun all the same and taken where they settle within the turn, before a timer of none fires: a write still out, tags already brought up. What the host has to answer is still not waited for.
|
Arkor — Preview deployments for arkorlab-upwind Use the checkbox below to choose whether this pull request gets preview deployments.
Checking this builds the latest commit and future pushes. Unchecking stops queued and future builds; a running build can finish and an existing preview stays available. |
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 31 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. CodeRabbit Verify
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
 [Medium impact] The PR appears safe to merge; no blocking issues were found. SummaryThe PR bounds waits after stale or expired data, starts a Server Action’s record read alongside module loading, and avoids copying inline base64 just to size a memo.
DiagramsequenceDiagram
participant R as Runtime
participant C as Cache host
participant M as Route module
participant A as Server Action
par Read saved page state
R->>C: Read current generation
C-->>R: Saved page state
and Load route
R->>M: Evaluate module
M-->>R: Handler
end
R->>A: Invoke handler with saved state
Reviews (2) · Last reviewed commit: "fix(runtime): count what JSON escapes in..." · Reviewed by Greptile |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 7 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/runtime/src/cache/data.ts">
<violation number="1" location="packages/runtime/src/cache/data.ts:122">
P3: The figure "some 1.4 s" does not match the step arithmetic used for the stale case. Four steps at 500 ms each come to 2 s, so either the number or the step count in this comment should be corrected.</violation>
</file>
<file name="packages/runtime/src/cache/held-reads.ts">
<violation number="1" location="packages/runtime/src/cache/held-reads.ts:109">
P2: This bypass can start host-backed `reread` and tag-sync work after the deadline; skip those remote fallbacks and use only isolate-held data in the late path.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
| async function heldNow(weighing: Weighing, until: number): Promise<Judged | undefined> { | ||
| try { | ||
| const now = await weighing.reread(); | ||
| const now = await inTime(until, 'the key read again', () => weighing.reread(), true); |
There was a problem hiding this comment.
P2: This bypass can start host-backed reread and tag-sync work after the deadline; skip those remote fallbacks and use only isolate-held data in the late path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/runtime/src/cache/held-reads.ts, line 109:
<comment>This bypass can start host-backed `reread` and tag-sync work after the deadline; skip those remote fallbacks and use only isolate-held data in the late path.</comment>
<file context>
@@ -51,15 +98,18 @@ export interface Weighing {
+async function heldNow(weighing: Weighing, until: number): Promise<Judged | undefined> {
try {
- const now = await weighing.reread();
+ const now = await inTime(until, 'the key read again', () => weighing.reread(), true);
return now === undefined
? undefined
</file context>
| * it reads from, and a read that crosses to it took 160–360 ms in deploy-tests runs, which the | ||
| * stale read's wait cut short. | ||
| * How long a read waits for a value later than the one it judged expired, past that read, all it | ||
| * asks there together, as `NEWER_DATA_DEADLINE_MS` is past a stale one: some 1.4 s given to each |
There was a problem hiding this comment.
P3: The figure "some 1.4 s" does not match the step arithmetic used for the stale case. Four steps at 500 ms each come to 2 s, so either the number or the step count in this comment should be corrected.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/runtime/src/cache/data.ts, line 122:
<comment>The figure "some 1.4 s" does not match the step arithmetic used for the stale case. Four steps at 500 ms each come to 2 s, so either the number or the step count in this comment should be corrected.</comment>
<file context>
@@ -107,28 +107,37 @@ export async function readData(runtime: CacheRuntime, request: DataReadRequest):
- * it reads from, and a read that crosses to it took 160–360 ms in deploy-tests runs, which the
- * stale read's wait cut short.
+ * How long a read waits for a value later than the one it judged expired, past that read, all it
+ * asks there together, as `NEWER_DATA_DEADLINE_MS` is past a stale one: some 1.4 s given to each
+ * step on its own. It computes the value again otherwise, in front of the render, which as a rule
+ * takes longer; and what it computes is written over the later value another isolate wrote, so that
</file context>
Suggested change
| * asks there together, as `NEWER_DATA_DEADLINE_MS` is past a stale one: some 1.4 s given to each | |
| * asks there together, as `NEWER_DATA_DEADLINE_MS` is past a stale one: some 2 s given to each |
`dataBytes` counted a memo's ids, handler and tags by their lengths, so tags full of quotes, backslashes or control characters — which JSON writes escaped — could come to more than the allowance beside them and leave the memo counted below its JSON (cubic). Each short string is now counted as JSON writes it; the inline base64, which needs no escapes, is still counted by its length alone, never written out. Also from review (cubic): the early route of a Server Action asks the entry tables through `hasEntry`, as `handle.ts` does, rather than a copy of it; the comment on the expired-value deadline spells out the 1.4 s it cites (two reads at 500 ms, two judgements at 200 ms); the artifact-id test now fails where the id goes uncounted. Not changed: past the deadline, the read of what the isolate holds may still start a read on the host, or a tag check, that no render waits for (cubic). It happens only where a write overtook the later value after the whole wait was spent; what it finds lands in the isolate's data memo and tag view for the reads after it; and a memory-only path would split both `readData` and the tag check for that one case.
This branch has not been deployed
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.
Summary
Three bounded waits on the path to the first byte:
Why
JSON.stringify, which copied the whole inline base64: 0.6 ms at 512 KiB, 9.7 ms at 8 MiB.Changes
laterIfAnysets one deadline (200 ms, or 500 ms past expired). The later-value read, its judgement, the probe andheldNoweach get what is left. The probe is not started once the deadline has passed. Past it,heldNowstill takes what the isolate has in hand — the write that overtook the later value, tags already brought up — where it settles within the turn, rather than answering a miss the render would compute again; what the host has to answer is not waited for. Late answers from the authority are still kept as the data center's copy.serveWithBodystarts the record lookup alongside module evaluation, under the routerenderedBywill reach (read from the entry tables), for Node entries with a cache and an action handed state. It uses the early result only when the routes match, and calls it as before otherwise. Edge pages are unaffected.dataBytescounts each short string (ids, handler, tags, artifact id) as JSON writes it, escapes included, and the inline base64 by its length alone, plus a fixed allowance, at two bytes a character, as before.Tests
later-generations.test.ts: a 150 ms read plus a 100 ms judgement answers at 200 ms; no probe once the deadline has passed; a stalledheldNowis a miss at the deadline; past the deadline, the write that overtook the later value is still answered.action-postponed.test.ts: the record read starts before module evaluation finishes; the same under a member's prerender route; no read for an edge page.data-memo-bytes.test.ts: inline and artifact-reference values are sized with nothing written as JSON, and never below their JSON size.cache-reads,cache-mutations, the action-posting suites and thecache-data-copiesworkers suite; eslint, oxfmt,tsc(runtime),check:vocabulary.Notes
Existing per-step data-wait tests, the memo budget tests and the data-copy workers suite all pass unchanged.
Review fixes
hasEntry; the expired-deadline comment spells out its 1.4 s; the artifact-id test fails where the id goes uncounted; a new test covers escape-heavy tags.Not changed
held-reads.ts, the past-deadline read may start host work no render waits for): it happens only where a write overtook the later value after the whole wait was spent; what it finds lands in the isolate's data memo and tag view for the reads after it; a memory-only path would split bothreadDataand the tag check for that one case.