Skip to content

lib,src: use built-in array buffer detach#50130

Closed
anonrig wants to merge 2 commits intonodejs:mainfrom
anonrig:array-buffer-transfer
Closed

lib,src: use built-in array buffer detach#50130
anonrig wants to merge 2 commits intonodejs:mainfrom
anonrig:array-buffer-transfer

Conversation

@anonrig
Copy link
Member

@anonrig anonrig commented Oct 10, 2023

This change requires v8 11.8, therefore can't be backported to v18 and v20

@anonrig anonrig added dont-land-on-v18.x dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. labels Oct 10, 2023
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Oct 10, 2023
@anonrig anonrig force-pushed the array-buffer-transfer branch 4 times, most recently from 63d7adb to 8eaae52 Compare October 10, 2023 21:13
@anonrig anonrig added commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. request-ci Add this label to start a Jenkins CI on a PR. labels Oct 10, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 11, 2023
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Oct 11, 2023

CI: https://ci.nodejs.org/job/node-test-pull-request/54688/

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Oct 11, 2023

CI: https://ci.nodejs.org/job/node-test-pull-request/54689/

@legendecas
Copy link
Member

legendecas commented Oct 11, 2023

Note that the v8 flag --harmony-rab-gsab-transfer can still be turned off with --no-harmony-rab-gsab-transfer and the stable Node.js APIs would be broken in such case.

ljharb
ljharb requested changes Oct 11, 2023
View reviewed changes
lib/internal/webstreams/readablestream.js
const viewBufferByteLength = ArrayBufferPrototypeGetByteLength(viewBuffer);

if (isArrayBufferDetached(viewBuffer)) {
if (viewBuffer.detached) {
Copy link
Member

@ljharb ljharb Oct 11, 2023

Choose a reason for hiding this comment

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

this is a mutable property, so it must be a primordial

Suggested change
if (viewBuffer.detached) {
if (ArrayBufferPrototypeDetached(viewBuffer)) {

lib/internal/webstreams/readablestream.js
validateBuffer(view, 'view');

if (isViewedArrayBufferDetached(view)) {
if (ArrayBufferViewGetBuffer(view).detached) {
Copy link
Member

@ljharb ljharb Oct 11, 2023

Choose a reason for hiding this comment

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

Suggested change
if (ArrayBufferViewGetBuffer(view).detached) {
if (ArrayBufferPrototypeDetached(ArrayBufferViewGetBuffer(view)) {

lib/internal/webstreams/readablestream.js
throw new ERR_INVALID_STATE.RangeError('The buffer size is invalid');
assert(!(bytesFilled % elementSize));
const transferredBuffer = transferArrayBuffer(buffer);
const transferredBuffer = buffer.transfer();
Copy link
Member

@ljharb ljharb Oct 11, 2023

Choose a reason for hiding this comment

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

Suggested change
const transferredBuffer = buffer.transfer();
const transferredBuffer = ArrayBufferPrototypeTransfer(buffer);

lib/internal/webstreams/readablestream.js
let transferredBuffer;
try {
transferredBuffer = transferArrayBuffer(buffer);
transferredBuffer = buffer.transfer();
Copy link
Member

@ljharb ljharb Oct 11, 2023

Choose a reason for hiding this comment

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

Suggested change
transferredBuffer = buffer.transfer();
transferredBuffer = ArrayBufferPrototypeTransfer(buffer);

lib/internal/webstreams/readablestream.js
}

desc.buffer = transferArrayBuffer(desc.buffer);
desc.buffer = desc.buffer.transfer();
Copy link
Member

@ljharb ljharb Oct 11, 2023

Choose a reason for hiding this comment

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

Suggested change
desc.buffer = desc.buffer.transfer();
desc.buffer = ArrayBufferPrototypeTransfer(desc.buffer);

lib/internal/webstreams/readablestream.js
return;

const transferredBuffer = transferArrayBuffer(buffer);
const transferredBuffer = buffer.transfer();
Copy link
Member

@ljharb ljharb Oct 11, 2023

Choose a reason for hiding this comment

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

Suggested change
const transferredBuffer = buffer.transfer();
const transferredBuffer = ArrayBufferPrototypeTransfer(buffer);

lib/internal/webstreams/readablestream.js
const firstPendingPullInto = pendingPullIntos[0];

if (isArrayBufferDetached(firstPendingPullInto.buffer)) {
if (firstPendingPullInto.buffer.detached) {
Copy link
Member

@ljharb ljharb Oct 11, 2023

Choose a reason for hiding this comment

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

Suggested change
if (firstPendingPullInto.buffer.detached) {
if (ArrayBufferPrototypeDetached(firstPendingPullInto.buffer)) {

lib/internal/webstreams/readablestream.js
firstPendingPullInto.buffer = transferArrayBuffer(
firstPendingPullInto.buffer,
);
firstPendingPullInto.buffer = firstPendingPullInto.buffer.transfer();
Copy link
Member

@ljharb ljharb Oct 11, 2023

Choose a reason for hiding this comment

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

Suggested change
firstPendingPullInto.buffer = firstPendingPullInto.buffer.transfer();
firstPendingPullInto.buffer = ArrayBufferPrototypeTransfer(firstPendingPullInto.buffer);

lib/internal/webstreams/readablestream.js
throw new ERR_INVALID_ARG_VALUE.RangeError('view', view);

desc.buffer = transferArrayBuffer(viewBuffer);
desc.buffer = viewBuffer.transfer();
Copy link
Member

@ljharb ljharb Oct 11, 2023

Choose a reason for hiding this comment

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

Suggested change
desc.buffer = viewBuffer.transfer();
desc.buffer = ArrayBufferPrototypeTransfer(viewBuffer);

@anonrig anonrig force-pushed the array-buffer-transfer branch from 8eaae52 to 9096c77 Compare October 12, 2023 15:02
@anonrig anonrig added the blocked PRs that are blocked by other issues or PRs. label Oct 12, 2023
@anonrig anonrig mentioned this pull request Dec 27, 2023
Closed
@anonrig
Copy link
Member Author

anonrig commented Jun 13, 2024

@legendecas would you be interested in removing the flag that disables arraybuffer.prototype.detach, from v8?

@legendecas
Copy link
Member

legendecas commented Jun 13, 2024

Sure, the feature has been released since M111 and I believe it is a good time to remove the flag.

@legendecas
Copy link
Member

legendecas commented Jun 17, 2024

v8/v8@9ebca66 the flag was removed since V8 12.6.

@anonrig
Copy link
Member Author

anonrig commented Jun 18, 2024

v8/v8@9ebca66 the flag was removed since V8 12.6.

I couldn't make the git node v8 backport work with this commit. There wasn't anything staged to resolve as a conflict. Does it work for you? @legendecas

⇣ 5% ❯ git node v8 backport 9ebca66 --verbose
[STARTED] Update local V8 clone
[STARTED] Fetch V8
[COMPLETED] Fetch V8
[COMPLETED] Update local V8 clone
[STARTED] V8 commit backport
[STARTED] Get current V8 version
[COMPLETED] Get current V8 version
[STARTED] Generate patches
[COMPLETED] Generate patches
[STARTED] Apply and commit patches to deps/v8
[STARTED] Commit 9ebca66a5740
[STARTED] Apply patch
[PROMPT] ? Resolve merge conflicts and enter 'RESOLVED' ›
[PROMPT] ? Resolve merge conflicts and enter 'RESOLVED' › R
[PROMPT] ? Resolve merge conflicts and enter 'RESOLVED' › RE
[PROMPT] ? Resolve merge conflicts and enter 'RESOLVED' › RES
[PROMPT] ? Resolve merge conflicts and enter 'RESOLVED' › RESO
[PROMPT] ? Resolve merge conflicts and enter 'RESOLVED' › RESOL
[PROMPT] ? Resolve merge conflicts and enter 'RESOLVED' › RESOLV
[PROMPT] ? Resolve merge conflicts and enter 'RESOLVED' › RESOLVE
[PROMPT] ? Resolve merge conflicts and enter 'RESOLVED' › RESOLVED
[PROMPT] ✔ Resolve merge conflicts and enter 'RESOLVED' · RESOLVED
[COMPLETED] Apply patch
[STARTED] Increment embedder version number
[COMPLETED] Increment embedder version number
[STARTED] Commit patch
[COMPLETED] Commit patch
[COMPLETED] Commit 9ebca66a5740
[COMPLETED] Apply and commit patches to deps/v8
[COMPLETED] V8 commit backport

@legendecas
Copy link
Member

legendecas commented Jun 19, 2024

My git node v8 backport 9ebca66a57409dd9441868a9ce429c0c3f61409d doesn't work as well. I will try to cherrypick manually.

@legendecas legendecas mentioned this pull request Jun 20, 2024
Merged
@anonrig anonrig closed this Sep 7, 2024
@anonrig anonrig mentioned this pull request Sep 7, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ljharb ljharb ljharb requested changes

Assignees

No one assigned

Labels

blocked PRs that are blocked by other issues or PRs. c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@anonrig @nodejs-github-bot @legendecas @ljharb