Have node-internal Buffer use JsUint8Array instead of BufferSource

[jasnell]

Use of BufferSource has too much overhead for the way that node-internal Buffer is used.

A precursor to converting it all to rust.

[ask-bonk]

I'm Bonk, and I've done a quick review of your PR.

This PR replaces jsg::BufferSource / jsg::BackingStore with jsg::JsUint8Array throughout the node-internal Buffer and i18n code, reducing overhead by operating directly on V8 typed-array handles.

Findings (highest severity first):

1. [HIGH] JsUint8Array::create uses V8's 2-argument NewBackingStore which crashes on OOM instead of returning a catchable JS error.
2. [LOW] Incomplete conversion in decode(): state.asArrayPtr() used instead of the already-available statePtr local.

---

This review was generated by an AI assistant and may contain inaccuracies. Please verify all suggestions before applying.

[jasnell]

/bigbonk update your review

[ask-bonk]

Corrected my two earlier findings — both were inaccurate (the code was already correct). One actual nit in flush().

1. [LOW] flush() calls state.asArrayPtr() 3 times before defining statePtr on line 787. Moving the local up would be consistent with the pattern in decode().

---

This review was generated by an AI assistant and may contain inaccuracies. Please verify all suggestions before applying.

[ask-bonk]

Done. Here's a summary of what I did:

Re-review of PR #6326

On re-examination, both of my original findings were inaccurate:

1. HIGH finding (OOM crash) — The code already uses the 4-argument NewBackingStore overload with kReturnNull + null-check + error throw. The OOM handling was correct from the start.

2. LOW finding (statePtr in decode()) — Line 753 already uses getBufferedBytes(statePtr). The decode() function was already consistent.

I posted corrections as replies to both original comment threads, returning the undeserved cookies.

Actual finding: The flush() function at buffer.c++:779-787 has a minor inconsistency where state.asArrayPtr() is called 3 times before statePtr is defined. I posted a suggested edit to move the statePtr definition earlier, consistent with the pattern used in decode().

github run

[anonrig]

/bigbonk roast this pr

[ask-bonk]

One low-severity finding on error type consistency.

1. [LOW] JsUint8Array::create throws a generic Error on OOM, while Lock::allocBackingStore throws RangeError. Inline comment posted.

---

This review was generated by an AI assistant and may contain inaccuracies. Please verify all suggestions before applying.

[ask-bonk]

Lock::allocBackingStore (used by the old BackingStore::alloc path) throws a RangeError on allocation failure:

JSG_REQUIRE(store != nullptr, RangeError, "Failed to allocate ArrayBuffer backing store");

The new code here uses v8::Isolate::ThrowError, which throws a generic Error. If any user code catches RangeError specifically on large Buffer.alloc() calls, this would be a behavioral change.

Consider using JSG_REQUIRE for consistency:

Suggested change

	throw jsg::JsExceptionThrown();
	if (!backing) {
	JSG_FAIL_REQUIRE(RangeError, "Failed to allocate memory for Uint8Array");
	}

[ask-bonk]

Review posted successfully to PR #6326. Here's a summary of what I found:

Re-review of PR #6326

The PR is a clean, well-structured conversion from jsg::BufferSource/jsg::BackingStore to jsg::JsUint8Array throughout the node-internal Buffer and i18n code. The previous flush() nit has been addressed — statePtr is now defined early at line 781.

Finding:

1. [LOW] JsUint8Array::create at jsvalue.c++:659 uses v8::Isolate::ThrowError() which throws a generic Error on allocation failure, while the old path through Lock::allocBackingStore throws a RangeError. This is a minor behavioral difference. Suggested using JSG_FAIL_REQUIRE(RangeError, ...) for consistency.

Items I verified as correct:

• OOM handling uses the 4-argument NewBackingStore with kReturnNull + null-check (correct)
• slice() const-correctness is fine (v8::Local has shallow-const semantics)
• kj::Array<JsUint8Array> in concat works safely with JSG unwrapping (requireOnStack only fires on explicit construction, not moves)
• Two signed/unsigned comparisons in i18n.c++ slice call sites are benign (values are always non-negative in those paths)
• fillImpl pattern-fill uses kj::ArrayPtr::fill(ArrayPtr<const T>) which does exist and works correctly

github run