FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

lib,src: use built-in array buffer detach by anonrig · Pull Request #50130 · nodejs/node · GitHub

/ node Public

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

Closed
anonrig wants to merge 2 commits into
nodejs:mainfrom
anonrig:array-buffer-transfer
ConversationCommits (2)ChecksFiles changed
Closed

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

Conversation

anonrig commented Oct 10, 2023

Copy link
Copy Markdown
Member

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

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 force-pushed the array-buffer-transfer branch 4 times, most recently from 63d7adb to 8eaae52 Compare October 10, 2023 21:13
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 Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 11, 2023

nodejs-github-bot commented Oct 11, 2023

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Oct 11, 2023

Copy link
Copy Markdown
Collaborator

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

legendecas commented Oct 11, 2023

Copy link
Copy Markdown
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 requested changes Oct 11, 2023
View reviewed changes
Comment thread lib/internal/webstreams/readablestream.js Outdated

ljharb Oct 11, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

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

Comment thread lib/internal/webstreams/readablestream.js Outdated

ljharb Oct 11, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality
Suggested change
if (ArrayBufferViewGetBuffer(view).detached) {
if (ArrayBufferPrototypeDetached(ArrayBufferViewGetBuffer(view)) {

Comment thread lib/internal/webstreams/readablestream.js Outdated

ljharb Oct 11, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality
Suggested change
const transferredBuffer = buffer.transfer();
const transferredBuffer = ArrayBufferPrototypeTransfer(buffer);

Comment thread lib/internal/webstreams/readablestream.js Outdated

ljharb Oct 11, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality
Suggested change
transferredBuffer = buffer.transfer();
transferredBuffer = ArrayBufferPrototypeTransfer(buffer);

Comment thread lib/internal/webstreams/readablestream.js Outdated

ljharb Oct 11, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality
Suggested change
desc.buffer = desc.buffer.transfer();
desc.buffer = ArrayBufferPrototypeTransfer(desc.buffer);

Comment thread lib/internal/webstreams/readablestream.js Outdated

ljharb Oct 11, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality
Suggested change
const transferredBuffer = buffer.transfer();
const transferredBuffer = ArrayBufferPrototypeTransfer(buffer);

Comment thread lib/internal/webstreams/readablestream.js Outdated

ljharb Oct 11, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality
Suggested change
if (firstPendingPullInto.buffer.detached) {
if (ArrayBufferPrototypeDetached(firstPendingPullInto.buffer)) {

Comment thread lib/internal/webstreams/readablestream.js Outdated

ljharb Oct 11, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality
Suggested change
firstPendingPullInto.buffer = firstPendingPullInto.buffer.transfer();
firstPendingPullInto.buffer = ArrayBufferPrototypeTransfer(firstPendingPullInto.buffer);

Comment thread lib/internal/webstreams/readablestream.js Outdated

ljharb Oct 11, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality
Suggested change
desc.buffer = viewBuffer.transfer();
desc.buffer = ArrayBufferPrototypeTransfer(viewBuffer);

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

anonrig commented Jun 13, 2024

Copy link
Copy Markdown
Member Author

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

legendecas commented Jun 13, 2024

Copy link
Copy Markdown
Member

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

legendecas commented Jun 17, 2024

Copy link
Copy Markdown
Member

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

anonrig commented Jun 18, 2024

Copy link
Copy Markdown
Member Author

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 commented Jun 19, 2024

Copy link
Copy Markdown
Member

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

legendecas mentioned this pull request Jun 20, 2024
Merged
anonrig closed this Sep 7, 2024
anonrig mentioned this pull request Sep 7, 2024
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

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

Back | FazBrowse Home | New Git URL