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

Jump to bottom

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

Closed
wants to merge 2 commits into from
Closed

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

wants to merge 2 commits into from

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 PRs that should not land on the v18.x-staging branch and should not be released in 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

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

@nodejs-github-bot
Copy link
Collaborator

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

@legendecas
Copy link
Member

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 suggested changes Oct 11, 2023
View reviewed changes
lib/internal/webstreams/readablestream.js
@@ -702,7 +699,7 @@ class ReadableStreamBYOBRequest {
const viewBuffer = ArrayBufferViewGetBuffer(view);
const viewBufferByteLength = ArrayBufferPrototypeGetByteLength(viewBuffer);

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

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
@@ -729,7 +726,7 @@ class ReadableStreamBYOBRequest {

validateBuffer(view, 'view');

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

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
@@ -1946,7 +1943,7 @@ function readableByteStreamControllerConvertPullIntoDescriptor(desc) {
if (bytesFilled > byteLength)
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

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
@@ -2615,7 +2612,7 @@ function readableByteStreamControllerPullInto(

let transferredBuffer;
try {
transferredBuffer = transferArrayBuffer(buffer);
transferredBuffer = buffer.transfer();
Copy link
Member

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
@@ -2707,7 +2704,7 @@ function readableByteStreamControllerRespond(controller, bytesWritten) {
throw new ERR_INVALID_ARG_VALUE.RangeError('bytesWritten', bytesWritten);
}

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

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
@@ -2757,22 +2754,20 @@ function readableByteStreamControllerEnqueue(controller, chunk) {
if (closeRequested || stream[kState].state !== 'readable')
return;

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

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

if (pendingPullIntos.length) {
const firstPendingPullInto = pendingPullIntos[0];

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

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

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
@@ -3067,7 +3062,7 @@ function readableByteStreamControllerRespondWithNewView(controller, view) {
if (bufferByteLength !== viewBufferByteLength)
throw new ERR_INVALID_ARG_VALUE.RangeError('view', view);

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

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 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

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

@legendecas
Copy link
Member

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

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-v18.x PRs that should not land on the v18.x-staging branch and should not be released in 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. 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