fix(client): copy Bytes values out of Node's shared Buffer pool on deserialization - #29701
Conversation
WalkthroughThis change modifies how 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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. Comment |
Merging this PR will degrade performance by 97.28%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ❌ | compile blog post page (uncached baseline) |
3.3 ms | 122.8 ms | -97.28% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing thribhuvan003:fix/bytes-shared-buffer-pool (384f19a) with main (aef8cb5)
Footnotes
-
30 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
aqrln
left a comment
There was a problem hiding this comment.
LGTM. This is unfortunate that we have to do this but safety and correctness come first.
If this causes measurable performance issues for large buffers, we could change this to only clone the buffers up to 4 KB which are actually pooled.
Fixes #29694
Bytes values come back as a Uint8Array that is a view into Node's shared 8 KB Buffer pool, so field.buffer is the whole pool — including bytes from other decoded values — and field.buffer.byteLength is 8192 instead of the value's real length. This changes both decode sites (structured results and $queryRaw) to copy into a standalone buffer instead of keeping the pool view. Added a test in each package asserting the returned array owns its buffer.