Skip to content
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.

Sign up for GitHub

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

src: add internal isArrayBufferDetached #45568

Merged
merged 1 commit into from
Nov 27, 2022
Merged

src: add internal isArrayBufferDetached #45568

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions lib/internal/util.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
'use strict';

const {
ArrayBufferPrototypeGetByteLength,
ArrayFrom,
ArrayIsArray,
ArrayPrototypePush,
Expand Down Expand Up @@ -42,6 +43,7 @@ const {
} = require('internal/errors');
const { signals } = internalBinding('constants').os;
const {
isArrayBufferDetached: _isArrayBufferDetached,
privateSymbols: {
arrow_message_private_symbol,
decorated_private_symbol,
Expand Down Expand Up @@ -560,6 +562,14 @@ function SideEffectFreeRegExpPrototypeExec(regex, string) {
return FunctionPrototypeCall(RegExpFromAnotherRealm.prototype.exec, regex, string);
}

function isArrayBufferDetached(value) {
if (ArrayBufferPrototypeGetByteLength(value) === 0) {
return _isArrayBufferDetached(value);
Copy link
Contributor

Choose a reason for hiding this comment

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

Crossing the JS-C++ boundary is gonna be slower than the current implementation although I'm not sure if this call in the zero length case is gonna be the primary bottleneck for any user code.

Did you consider implementing @jasnell's suggestion in #45512 (comment)? Seems like that approach would solve this problem without adding any performance overhead in any case.

One thing we might be able to do is add a hidden symbol to ArrayBuffer instances when we detach them in c++ land. Then we can have an isDetached check in js that checks the length and the presence of that symbol.

Copy link
Member Author

@anonrig anonrig Nov 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

IMHO, that is the perfect solution for performance, but I believe it's too hacky to maintain in the long term. I can't think of a good/maintainable way to enforce attaching the hidden symbol as a rule.

If it's the consensus @jasnell thinks is the best way, I'll be happy to implement it regardless.

}

return false;
Comment on lines +566 to +570
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: the flow might look semantically better like this

Suggested change
if (ArrayBufferPrototypeGetByteLength(value) === 0) {
return _isArrayBufferDetached(value);
}
return false;
if (ArrayBufferPrototypeGetByteLength(value) !== 0) {
return false;
}
return _isArrayBufferDetached(value);

}

module.exports = {
assertCrypto,
cachedResult,
Expand All @@ -577,6 +587,7 @@ module.exports = {
getInternalGlobal,
getSystemErrorMap,
getSystemErrorName,
isArrayBufferDetached,
isError,
isInsideNodeModules,
join,
Expand Down
6 changes: 3 additions & 3 deletions lib/internal/webstreams/readablestream.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ const {
const {
createDeferredPromise,
customInspectSymbol: kInspect,
isArrayBufferDetached,
kEmptyObject,
kEnumerableProperty,
} = require('internal/util');
Expand Down Expand Up @@ -104,7 +105,6 @@ const {
extractHighWaterMark,
extractSizeAlgorithm,
lazyTransfer,
isDetachedBuffer,
isViewedArrayBufferDetached,
isBrandCheck,
resetQueue,
Expand Down Expand Up @@ -669,7 +669,7 @@ class ReadableStreamBYOBRequest {
const viewBuffer = ArrayBufferViewGetBuffer(view);
const viewBufferByteLength = ArrayBufferPrototypeGetByteLength(viewBuffer);

if (isDetachedBuffer(viewBuffer)) {
if (isArrayBufferDetached(viewBuffer)) {
throw new ERR_INVALID_STATE.TypeError('Viewed ArrayBuffer is detached');
}

Expand Down Expand Up @@ -2643,7 +2643,7 @@ function readableByteStreamControllerEnqueue(controller, chunk) {
if (pendingPullIntos.length) {
const firstPendingPullInto = pendingPullIntos[0];

if (isDetachedBuffer(firstPendingPullInto.buffer)) {
if (isArrayBufferDetached(firstPendingPullInto.buffer)) {
throw new ERR_INVALID_STATE.TypeError(
'Destination ArrayBuffer is detached',
);
Expand Down
18 changes: 2 additions & 16 deletions lib/internal/webstreams/util.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
'use strict';

const {
ArrayBufferPrototypeGetByteLength,
ArrayBufferPrototypeSlice,
ArrayPrototypePush,
ArrayPrototypeShift,
Expand Down Expand Up @@ -45,6 +44,7 @@ const {
} = internalBinding('util');

const assert = require('internal/assert');
const { isArrayBufferDetached } = require('internal/util');

const kState = Symbol('kState');
const kType = Symbol('kType');
Expand Down Expand Up @@ -135,23 +135,10 @@ function transferArrayBuffer(buffer) {
return res;
}

function isDetachedBuffer(buffer) {
if (ArrayBufferPrototypeGetByteLength(buffer) === 0) {
// TODO(daeyeon): Consider using C++ builtin to improve performance.
try {
new Uint8Array(buffer);
} catch (error) {
assert(error.name === 'TypeError');
return true;
}
}
return false;
}

function isViewedArrayBufferDetached(view) {
return (
ArrayBufferViewGetByteLength(view) === 0 &&
isDetachedBuffer(ArrayBufferViewGetBuffer(view))
isArrayBufferDetached(ArrayBufferViewGetBuffer(view))
);
}

Expand Down Expand Up @@ -251,7 +238,6 @@ module.exports = {
extractSizeAlgorithm,
lazyTransfer,
isBrandCheck,
isDetachedBuffer,
isPromisePending,
isViewedArrayBufferDetached,
peekQueueValue,
Expand Down
12 changes: 12 additions & 0 deletions src/node_util.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,15 @@ static void GetProxyDetails(const FunctionCallbackInfo<Value>& args) {
}
}

static void IsArrayBufferDetached(const FunctionCallbackInfo<Value>& args) {
if (args[0]->IsArrayBuffer()) {
auto buffer = args[0].As<v8::ArrayBuffer>();
args.GetReturnValue().Set(buffer->WasDetached());
return;
}
args.GetReturnValue().Set(false);
}

static void PreviewEntries(const FunctionCallbackInfo<Value>& args) {
if (!args[0]->IsObject())
return;
Expand Down Expand Up @@ -342,6 +351,7 @@ static void ToUSVString(const FunctionCallbackInfo<Value>& args) {
void RegisterExternalReferences(ExternalReferenceRegistry* registry) {
registry->Register(GetPromiseDetails);
registry->Register(GetProxyDetails);
registry->Register(IsArrayBufferDetached);
registry->Register(PreviewEntries);
registry->Register(GetOwnNonIndexProperties);
registry->Register(GetConstructorName);
Expand Down Expand Up @@ -403,6 +413,8 @@ void Initialize(Local<Object> target,
SetMethodNoSideEffect(
context, target, "getPromiseDetails", GetPromiseDetails);
SetMethodNoSideEffect(context, target, "getProxyDetails", GetProxyDetails);
SetMethodNoSideEffect(
context, target, "isArrayBufferDetached", IsArrayBufferDetached);
SetMethodNoSideEffect(context, target, "previewEntries", PreviewEntries);
SetMethodNoSideEffect(
context, target, "getOwnNonIndexProperties", GetOwnNonIndexProperties);
Expand Down