-
-
Notifications
You must be signed in to change notification settings - Fork 38.4k
buffer,fs: support immutable arraybuffers #66379
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -989,6 +989,27 @@ int32_t FastCompare(Local<Value>, | |||||||||||||||||||||
|
|
||||||||||||||||||||||
| static CFunction fast_compare(CFunction::Make(FastCompare)); | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| bool IsImmutableImpl(Local<Value> view) { | ||||||||||||||||||||||
| return view.As<ArrayBufferView>()->Buffer()->IsImmutable(); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
Comment on lines
+992
to
+994
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should likely also at least
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, given how this is used, I think you actually need a tri-state return. For the
Then at the callsites, (that's a bit pedantic tho so I'll leave it up to you... at a minimum the |
||||||||||||||||||||||
|
|
||||||||||||||||||||||
| void SlowIsImmutable(const FunctionCallbackInfo<Value>& args) { | ||||||||||||||||||||||
| CHECK(args[0]->IsArrayBufferView()); | ||||||||||||||||||||||
| args.GetReturnValue().Set(IsImmutableImpl(args[0])); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| bool FastIsImmutable(Local<Value>, | ||||||||||||||||||||||
| Local<Value> view, | ||||||||||||||||||||||
| // NOLINTNEXTLINE(runtime/references) | ||||||||||||||||||||||
| FastApiCallbackOptions& options) { | ||||||||||||||||||||||
| TRACK_V8_FAST_API_CALL("buffer.isImmutable"); | ||||||||||||||||||||||
| HandleScope scope(options.isolate); | ||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this really need a handle scope?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It should not |
||||||||||||||||||||||
|
|
||||||||||||||||||||||
| return IsImmutableImpl(view); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| static CFunction fast_is_immutable(CFunction::Make(FastIsImmutable)); | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| // Computes the offset for starting an indexOf or lastIndexOf search. | ||||||||||||||||||||||
| // Returns either a valid offset in [0...<length - 1>], ie inside the Buffer, | ||||||||||||||||||||||
| // or -1 to signal that there is no possible match. | ||||||||||||||||||||||
|
|
@@ -1927,6 +1948,8 @@ void Initialize(Local<Object> target, | |||||||||||||||||||||
| &fast_byte_length_utf8); | ||||||||||||||||||||||
| SetFastMethod(context, target, "copy", SlowCopy, &fast_copy); | ||||||||||||||||||||||
| SetFastMethodNoSideEffect(context, target, "compare", Compare, &fast_compare); | ||||||||||||||||||||||
| SetFastMethodNoSideEffect( | ||||||||||||||||||||||
| context, target, "isImmutable", SlowIsImmutable, &fast_is_immutable); | ||||||||||||||||||||||
| SetMethodNoSideEffect(context, target, "compareOffset", CompareOffset); | ||||||||||||||||||||||
| SetMethod(context, target, "fill", Fill); | ||||||||||||||||||||||
| SetMethodNoSideEffect(context, target, "indexOfBuffer", IndexOfBuffer); | ||||||||||||||||||||||
|
|
@@ -2014,6 +2037,8 @@ void RegisterExternalReferences(ExternalReferenceRegistry* registry) { | |||||||||||||||||||||
| registry->Register(fast_copy); | ||||||||||||||||||||||
| registry->Register(Compare); | ||||||||||||||||||||||
| registry->Register(fast_compare); | ||||||||||||||||||||||
| registry->Register(SlowIsImmutable); | ||||||||||||||||||||||
| registry->Register(fast_is_immutable); | ||||||||||||||||||||||
| registry->Register(CompareOffset); | ||||||||||||||||||||||
| registry->Register(Fill); | ||||||||||||||||||||||
| registry->Register(IndexOfBuffer); | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,65 @@ | ||
| // Flags: --js-immutable-arraybuffer | ||
| 'use strict'; | ||
|
|
||
| require('../common'); | ||
| const assert = require('assert'); | ||
|
|
||
| function immutable(bytes) { | ||
| const buffer = Uint8Array.from(bytes).buffer.transferToImmutable(); | ||
| assert.strictEqual(buffer.immutable, true); | ||
| return new Uint8Array(buffer); | ||
| } | ||
|
|
||
| function check(actual, expected) { | ||
| assert(Buffer.isBuffer(actual)); | ||
| assert.deepStrictEqual([...actual], expected); | ||
| assert.strictEqual(actual.buffer.immutable, false); | ||
| actual.fill(0); | ||
| } | ||
|
|
||
| { | ||
| const source = immutable([1, 2, 3, 4]); | ||
| check(Buffer.from(source), [1, 2, 3, 4]); | ||
| check(Buffer.from(source.subarray(1, 3)), [2, 3]); | ||
| check(Buffer.from(Buffer.from(source.buffer)), [1, 2, 3, 4]); | ||
| assert.deepStrictEqual([...source], [1, 2, 3, 4]); | ||
| } | ||
|
|
||
| { | ||
| // Larger than half the pool size, so the copy is not pooled. | ||
| const bytes = Array.from({ length: Buffer.poolSize }, (_, i) => i & 0xff); | ||
| const source = immutable(bytes); | ||
| check(Buffer.from(source), bytes); | ||
| check(Buffer.from(source.subarray(1)), bytes.slice(1)); | ||
| } | ||
|
|
||
| { | ||
| const source = immutable([1, 2, 3, 4]); | ||
| check(Buffer.copyBytesFrom(source), [1, 2, 3, 4]); | ||
| check(Buffer.copyBytesFrom(source, 1, 2), [2, 3]); | ||
| const wide = new Uint16Array(Uint16Array.from([0x0102, 0x0304]).buffer.transferToImmutable()); | ||
| check(Buffer.copyBytesFrom(wide, 1), [...new Uint8Array(Uint16Array.from([0x0304]).buffer)]); | ||
| } | ||
|
|
||
| { | ||
| const a = immutable([1, 2]); | ||
| const b = Buffer.from([3, 4]); | ||
| const c = immutable([5, 6]); | ||
| check(Buffer.concat([a, b, c]), [1, 2, 3, 4, 5, 6]); | ||
| check(Buffer.concat([a, b, c], 6), [1, 2, 3, 4, 5, 6]); | ||
| // The last element is cut short. | ||
| check(Buffer.concat([a, b, c], 5), [1, 2, 3, 4, 5]); | ||
| // The result is longer than the sum of the elements and gets zero-filled. | ||
| check(Buffer.concat([a, c], 5), [1, 2, 5, 6, 0]); | ||
| check(Buffer.concat([a.subarray(1), c.subarray(0, 1)]), [2, 5]); | ||
| assert.deepStrictEqual([...a], [1, 2]); | ||
| assert.deepStrictEqual([...c], [5, 6]); | ||
| } | ||
|
|
||
| { | ||
| // The other direction is unchanged: the copy must not write into a view | ||
| // backed by an immutable ArrayBuffer. | ||
| const target = immutable([9, 9, 9, 9]); | ||
| assert.strictEqual(Buffer.from([1, 2, 3, 4]).copy(target), 0); | ||
| assert.deepStrictEqual([...target], [9, 9, 9, 9]); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bikeshed: I'd prefer
validateMutableBufferbut non-blocking.