feat(ai): optimize text accumulation runtime to O(N)#15897
feat(ai): optimize text accumulation runtime to O(N)#15897aayush-kapoor wants to merge 8 commits into
Conversation
|
we would need some benchmarks that show that this is an actual improve (and can be used to check for regressions) |
Validation results + report of a sibling site this PR does not coverValidated this PR (specifically the v6 backport, #15906) against an in-the-wild reproduction of the bug it was filed for. Short version: the PR is correctly written and a real improvement, but the bug still fires on tool-input-heavy workloads because of a third O(N²) site in the same file that this PR does not address. Posting here so you can decide whether to expand scope before merging or land it with a tracking issue. Stack used
Results
Max-step distribution per group (sorted desc):
Every single run, in every config, has at least one step taking 48–120s. In a healthy run, no individual LLM step should take >15s on this model. The signature is the bug still firing — just attenuated. The third site
case 'tool-input-delta': {
const partialToolCall = state.partialToolCalls[chunk.toolCallId];
…
partialToolCall.text += chunk.inputTextDelta; // ← same pattern as text-delta / reasoning-delta
const { value: partialArgs } = await parsePartialJson(
partialToolCall.text, // ← forces flatten on every chunk
);Same Why a naïve
|
| Object.defineProperty(part, '__textAccumulator', { | ||
| configurable: true, | ||
| value: accumulator, | ||
| }); | ||
|
|
||
| Object.defineProperty(part, 'text', { | ||
| configurable: true, | ||
| enumerable: true, | ||
| get() { | ||
| return accumulator.getText(); | ||
| }, | ||
| set(value: string) { | ||
| accumulator.setText(value); | ||
| }, | ||
| }); |
There was a problem hiding this comment.
this is weird why are those functions not methods on the class
| switch (chunk.type) { | ||
| case 'text-start': { | ||
| const textPart: TextUIPart = { | ||
| const textPart = prepareTextAccumulator<TextUIPart>({ |
There was a problem hiding this comment.
the map of part to accumulator should be maintained in this function, not in text-accumulator (which accumulates a single text)
| type TextAccumulatorPart = { | ||
| text: string; | ||
| __textAccumulator?: TextAccumulator; | ||
| }; | ||
|
|
||
| function getTextAccumulator<PART extends { text: string }>(part: PART) { | ||
| return (part as TextAccumulatorPart).__textAccumulator; | ||
| } |
Background
reported in #15670
the old code does this on every delta:
which step by step looks like:
for tiny strings this is fine. for long streams, the copied prefix keeps growing.
It gets worse when something reads
.textbetween writes because that forces the JS engine to materialize/flatten the string before the next append.Summary
the new code keeps chunks internally as
which step by step looks like:
so when someone reads:
textPart.textthe getter does:
Then it caches that:
If
.textis read again before a new delta arrives, it returns the cached string.Manual Verification
na but have asked some providers to verify the fix by applying patch manaully
Checklist
pnpm changesetin the project root)Future Work
investigate to see if similar patterns exist in the codebase
Related Issues
fixes #15670