feat(perf): Avoid unnecessary .slice() in Utils.toArrayBuffer()#1038
Conversation
|
@wh201906 is attempting to deploy a commit to the Margelo Team on Vercel. A member of the Team first needs to authorize it. |
boorad
left a comment
There was a problem hiding this comment.
Nice optimization — the fast-path condition (byteOffset === 0 && byteLength === buffer.byteLength) is exactly the case where .slice() produces a byte-identical copy of the backing buffer, so skipping is safe. Also cleans up the effectively-dead if (buf?.buffer?.slice) branch (all real ArrayBuffers have .slice, so the old else was unreachable). Consistent with bufferLikeToArrayBuffer (which already returns ArrayBuffer inputs directly) and abvToArrayBuffer.
Two minor things worth considering before merge:
1. Updated docstring on toArrayBuffer (line 112). A few small issues:
- Missing terminal period.
- "internal buffer" is ambiguous — "underlying ArrayBuffer" is the more standard term.
- Doesn't mention that the returned buffer may now be aliased with the caller's input.
Suggested wording:
Note this copies data only when the supplied view represents a subrange of the underlying ArrayBuffer; otherwise the backing buffer is returned directly (aliased — do not mutate after passing).
2. Behavioral change worth calling out in the PR description. The previous code always returned a fresh copy from the buffer/view branch. The new code may alias the original Buffer's backing storage when the view covers it entirely. All current callers (hash, cipher, ec, dh, etc.) treat these as read-only inputs to native crypto, and bufferLikeToArrayBuffer/abvToArrayBuffer already alias for ArrayBuffer inputs — so the practical risk is low. But it's a subtle semantic change that's worth a sentence in the PR body for future readers / changelog.
|
I've applied the review suggestions to the code and PR description. |
In the current implementation,
if (buf?.buffer?.slice)condition inUtils.toArrayBuffer()is alwaystrueforCraftzdogBufferandArrayBufferView, so the underlying buffer is always copied via.slice().Considering that
toArrayBuffer()is for protecting data outside the view's region, and the existingreturn buf.buffer as ArrayBuffer;branch already implies that returning the original buffer directly is acceptable, we can call.slice()only if the view represents a subrange of the underlying buffer, and remove the unnecessaryif (buf?.buffer?.slice)condition.In summary, the new code may alias the original Buffer's backing storage when the view covers it entirely. Callers should not modify the result.