Skip to content

fix(javascript): reserve writer capacity for write paths - #3994

Merged
chaokunyang merged 4 commits into
apache:mainfrom
ayush00git:fix/reserve-writer-buffer
Aug 30, 2026
Merged

fix(javascript): reserve writer capacity for write paths#3994
chaokunyang merged 4 commits into
apache:mainfrom
ayush00git:fix/reserve-writer-buffer

Conversation

@ayush00git

@ayush00git ayush00git commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

Why?

BinaryWriter starts with a ~100KB buffer and, for speed, its individual writeXxx methods do no bounds checking. The contract is that callers call reserve(n) first, which grows the buffer if needed. The generated collection write path honors this (it reserves elementFixedSize * count before the loop), but three paths write per-entry data with no reserve at all: the generated declared-map write, MapAnySerializer.write, and CollectionAnySerializer.write.

What does this PR do?

three write paths now reserve writer capacity, matching the existing generated-collection convention (root reserves fixedSize once; per-element loops reserve fixedSize × count; strings reserve internally)

Related issues

AI Contribution Checklist

  • Substantial AI assistance was used in this PR: yes / no
  • If yes, I included a completed AI Contribution Checklist in this PR description and the required AI Usage Disclosure.
  • If yes, my PR description includes the required ai_review summary and screenshot evidence or equivalent persisted links of the final clean AI review results from both fresh reviewers described in AI_POLICY.md, the Fory-guided reviewer and the independent general reviewer, on the current PR diff or current HEAD after the latest code changes.

Does this PR introduce any user-facing change?

  • Does this PR introduce any public API change?
  • Does this PR introduce any binary protocol compatibility change?

Benchmark

// while the cursor advanced, so dump() returned uninitialized tail bytes.
const fory = new Fory({ compatible: false });
const { serialize, deserialize } = fory.register(Type.list(Type.any()));
const arr = new Array(150000).fill(1);

@chaokunyang chaokunyang Aug 30, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because every element is 1, writeElementsHeader sets isSame = true, so this test exercises only the aggregate reserve. The per-item mixed-type reserves and the writeDeclared reserve can still be removed without this test failing. Please add a list larger than the initial writer buffer with mixed element types; if writeDeclared is part of this fix, cover the unknown-Struct declared-list reserialization path as well.

const { serialize, deserialize } = fory.register(
Type.struct(
{ namespace: "example", typeName: "BigMap" },
{ m: Type.map(Type.int32({ encoding: "fixed" }), Type.int32({ encoding: "fixed" })) },

@chaokunyang chaokunyang Aug 30, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This declared int32 map goes through writeSpecificType, so it does not execute the new reserve in MapAnySerializer.write. Please add a Type.map(Type.any(), Type.any()) case large enough to exceed the initial writer buffer, so removing that reserve makes the regression test fail.

@chaokunyang
chaokunyang merged commit e823e47 into apache:main Aug 30, 2026
64 checks passed
@ayush00git
ayush00git deleted the fix/reserve-writer-buffer branch August 30, 2026 17:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants