GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — lib,src: use built-in array buffer detach by anonrig · Pull Request #50130 · nodejs/node

Conversation

@anonrig

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
gurgunday and bricss reacted with rocket emoji
All reactions
  • 🚀 2 reactions
@anonrig anonrig added dont-land-on-v18.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 involving general changes in the lib/ or src/ directories. 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 PRs the Commit Queue should land as multiple self-contained commits. request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. labels Oct 10, 2023
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Oct 11, 2023
@nodejs-github-bot

nodejs-github-bot commented Oct 11, 2023

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Oct 11, 2023

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@legendecas

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
ljharb
ljharb requested changes Oct 11, 2023
View reviewed changes
Comment thread lib/internal/webstreams/readablestream.js Outdated

@ljharb ljharb Oct 11, 2023

Copy link
Copy Markdown
Member

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.

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread lib/internal/webstreams/readablestream.js Outdated

@ljharb ljharb Oct 11, 2023

Copy link
Copy Markdown
Member

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread lib/internal/webstreams/readablestream.js Outdated

@ljharb ljharb Oct 11, 2023

Copy link
Copy Markdown
Member

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread lib/internal/webstreams/readablestream.js Outdated

@ljharb ljharb Oct 11, 2023

Copy link
Copy Markdown
Member

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread lib/internal/webstreams/readablestream.js Outdated

@ljharb ljharb Oct 11, 2023

Copy link
Copy Markdown
Member

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread lib/internal/webstreams/readablestream.js Outdated

@ljharb ljharb Oct 11, 2023

Copy link
Copy Markdown
Member

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread lib/internal/webstreams/readablestream.js Outdated

@ljharb ljharb Oct 11, 2023

Copy link
Copy Markdown
Member

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread lib/internal/webstreams/readablestream.js Outdated

@ljharb ljharb Oct 11, 2023

Copy link
Copy Markdown
Member

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread lib/internal/webstreams/readablestream.js Outdated

@ljharb ljharb Oct 11, 2023

Copy link
Copy Markdown
Member

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
anonrig added 2 commits October 12, 2023 11:02
@anonrig
lib,src: use built-in array buffer detach
ba7744e
@anonrig
lib: use ArrayBuffer.prototype.transfer
9096c77
@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
lib,src: remove detachArrayBuffer from buffer binding #51297
Closed
@anonrig

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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@legendecas

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.
anonrig reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@legendecas

legendecas commented Jun 17, 2024

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@anonrig

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@legendecas

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@legendecas legendecas mentioned this pull request Jun 20, 2024
deps: V8: cherry-pick 9ebca66a5740 #53522
Merged
@anonrig anonrig closed this Sep 7, 2024
@anonrig anonrig mentioned this pull request Sep 7, 2024
lib,src: use built-in array buffer detach, transfer #54837
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

Uh oh!

There was an error while loading. Please reload this page.

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 PRs the Commit Queue should land as multiple self-contained commits. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. 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.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@anonrig @nodejs-github-bot @legendecas @ljharb
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.