GeezerVison

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

GeezerVison is showing github.com — deps: cherry-pick 00704f5a from V8 upstream by kvakil · Pull Request #43921 · nodejs/node

Conversation

kvakil commented Jul 21, 2022

Copy link

Copy Markdown

Contributor

Original commit message:

Add more efficient API for accesssing ArrayBuffer raw data

Raw data access is already possible via GetBackingStore()->GetData().
This API exposes a more efficient way for accessing
JSArrayBuffer::backing_store (which, despite the confusing name, is no
the BackingStore but its raw data pointer).

Bug: v8:10343
Change-Id: I695cea91e2c3de75ce6c86bac6e413ce6617958b
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3764341
Reviewed-by: Camillo Bruni <[email protected]>
Commit-Queue: Marja Hölttä <[email protected]>
Cr-Commit-Position: refs/heads/main@{#81745}

Refs: v8/v8@00704f5
Refs: #32226

Sorry, something went wrong.

Uh oh!

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

bricss reacted with thumbs up emoji

All reactions

👍 1 reaction

nodejs-github-bot commented Jul 21, 2022

Copy link

Copy Markdown

Collaborator

Review requested:

@nodejs/v8-update

All reactions

Sorry, something went wrong.

Uh oh!

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

nodejs-github-bot added needs-ci

PRs that need a full CI run.

v8 engine Issues and PRs related to the V8 dependency. labels

Jul 21, 2022

gengjiawen approved these changes

Jul 21, 2022

View reviewed changes

kvakil mentioned this pull request

Jul 21, 2022

src: remove many uses of GetBackingStore #43923

Closed

kvakil added a commit to kvakil/node that referenced this pull request

Jul 21, 2022


          src: remove many uses of GetBackingStore

261134e

This replaces many uses of `GetBackingStore()->Data()` with the newly
backported `Data()`.

I only replaced the "obvious" usages here, where I could easily convince
myself that the lifetime was safe. The less obvious changes will come in
a separate PR.

Refs: nodejs#32226
Refs: nodejs#43921

cjihrig commented Jul 21, 2022

Copy link

Copy Markdown

Contributor

I think this should increment v8_embedder_string in common.gypi.

All reactions

Sorry, something went wrong.

Uh oh!

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

kvakil commented Jul 21, 2022

Copy link

Copy Markdown

Contributor Author

I think this should increment v8_embedder_string in common.gypi.

Thanks, I missed that while skimming. I updated the docs in #43924 with a reminder.

All reactions

Sorry, something went wrong.

Uh oh!

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

kvakil added a commit to kvakil/node that referenced this pull request

Jul 21, 2022


          src: remove unowned usages of GetBackingStore

7973e4e

This removes all usages of GetBackingStore without `CODEOWNERS`. See the
linked issue for an explanation.

Refs: nodejs#32226
Refs: nodejs#43921

kvakil added a commit to kvakil/node that referenced this pull request

Jul 21, 2022


          src: remove usages of GetBackingStore in crypto

c7fade8

This removes all usages of GetBackingStore in `crypto`. See the
linked issue for an explanation.

Note: I am not sure of the lifetime semantics intended by
`ArrayBufferOrViewContents` -- I am pretty sure it is correct based on
a manual audit of the callsites, but please ensure that it is correct.

Refs: nodejs#32226
Refs: nodejs#43921

kvakil added a commit to kvakil/node that referenced this pull request

Jul 21, 2022


          src: remove usages of GetBackingStore in node-api

8e7875f

This removes all usages of GetBackingStore in `node-api`. See the linked
issue for an explanation.

Refs: nodejs#32226
Refs: nodejs#43921

kvakil added a commit to kvakil/node that referenced this pull request

Jul 21, 2022


          src: remove usages of GetBackingStore in modules

5582fcf

This removes all usages of GetBackingStore in modules. See the linked
issue for an explanation.

Refs: nodejs#32226
Refs: nodejs#43921

kvakil added a commit to kvakil/node that referenced this pull request

Jul 21, 2022


          src: remove usages of GetBackingStore in WASI

2ec02bb

This removes all usages of GetBackingStore in WASI. See the linked issue
for an explanation.

Refs: nodejs#32226
Refs: nodejs#43921

kvakil added a commit to kvakil/node that referenced this pull request

Jul 21, 2022


          src: remove usages of GetBackingStore in startup

48287f2

This removes all usages of GetBackingStore in startup. See the linked
issue for an explanation.

Refs: nodejs#32226
Refs: nodejs#43921

kvakil mentioned this pull request

Jul 21, 2022

buffer: ~2x slowdown in master compared to v12.x #32226

Closed

targos approved these changes

Jul 21, 2022

View reviewed changes

tniessen commented Jul 21, 2022

Copy link

Copy Markdown

Member

@kvakil It looks like your git user.email is not associated with your GitHub account. This will not prevent this PR from being merged, however, we still encourage you to either re-commit the changes with an email address that is associated with your GitHub account, or that you add the email address you are using with git to your GitHub account.

All reactions

Sorry, something went wrong.

Uh oh!

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

kvakil commented Jul 21, 2022

Copy link

Copy Markdown

Contributor Author

@kvakil It looks like your git user.email is not associated with your GitHub account. This will not prevent this PR from being merged, however, we still encourage you to either re-commit the changes with an email address that is associated with your GitHub account, or that you add the email address you are using with git to your GitHub account.

Thanks for the flag! I added the email to Github; it appears to have linked back correctly.

tniessen reacted with hooray emoji

All reactions

🎉 1 reaction

Sorry, something went wrong.

Uh oh!

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

kvakil added 2 commits

July 24, 2022 18:23


          deps: cherry-pick 00704f5a from V8 upstream

118138d

Original commit message:

    Add more efficient API for accesssing ArrayBuffer raw data

    Raw data access is already possible via GetBackingStore()->GetData().
    This API exposes a more efficient way for accessing
    JSArrayBuffer::backing_store (which, despite the confusing name, is no
    the BackingStore but its raw data pointer).

    Bug: v8:10343
    Change-Id: I695cea91e2c3de75ce6c86bac6e413ce6617958b
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3764341
    Reviewed-by: Camillo Bruni <[email protected]>
    Commit-Queue: Marja Hölttä <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#81745}

Refs: v8/v8@00704f5
Refs: nodejs#32226


          Increment embedder number.

beed835

kvakil force-pushed the backportFasterAccess branch from edffcbc to beed835 Compare

July 24, 2022 18:24

F3n67u added request-ci

Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review.

author ready PRs with CI started, the required approvals, and no outstanding review comments. labels

Jul 25, 2022

nodejs-github-bot pushed a commit that referenced this pull request

Aug 3, 2022


          src: remove usages of GetBackingStore in WASI

13d93b6

This removes all usages of GetBackingStore in WASI. See the linked issue
for an explanation.

Refs: #32226
Refs: #43921
PR-URL: #44077
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Feng Yu <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>

nodejs-github-bot pushed a commit that referenced this pull request

Aug 3, 2022


          src: remove usages of GetBackingStore in modules

4eaaa17

This removes all usages of GetBackingStore in modules. See the linked
issue for an explanation.

Refs: #32226
Refs: #43921
PR-URL: #44076
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Feng Yu <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>

nodejs-github-bot pushed a commit that referenced this pull request

Aug 3, 2022


          src: remove usages of GetBackingStore in node-api

4dedd81

This removes all usages of GetBackingStore in `node-api`. See the linked
issue for an explanation.

Refs: #32226
Refs: #43921
PR-URL: #44075
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Feng Yu <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>

nodejs-github-bot pushed a commit that referenced this pull request

Aug 3, 2022


          src: remove unowned usages of GetBackingStore

8c35a4e

This removes all usages of GetBackingStore without any entries in the
`CODEOWNERS` file. For the most part this is a pretty straightforward
review; except `SPREAD_BUFFER_ARG` and the changes to `CopyArrayBuffer`.

See the linked issue for an explanation.

Refs: #32226
Refs: #43921
PR-URL: #44080
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>

nodejs-github-bot pushed a commit that referenced this pull request

Aug 4, 2022


          src: remove usages of GetBackingStore in crypto

4c725f4

This removes all usages of GetBackingStore in `crypto`. See the
linked issue for an explanation.

Note: I am not sure of the lifetime semantics intended by
`ArrayBufferOrViewContents` -- I am pretty sure it is correct based on
a manual audit of the callsites, but please ensure that it is correct.

Refs: #32226
Refs: #43921
PR-URL: #44079
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Feng Yu <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>

danielleadams pushed a commit that referenced this pull request

Aug 16, 2022


          deps: cherry-pick 00704f5a from V8 upstream

c900bc1

Original commit message:

    Add more efficient API for accesssing ArrayBuffer raw data

    Raw data access is already possible via GetBackingStore()->GetData().
    This API exposes a more efficient way for accessing
    JSArrayBuffer::backing_store (which, despite the confusing name, is no
    the BackingStore but its raw data pointer).

    Bug: v8:10343
    Change-Id: I695cea91e2c3de75ce6c86bac6e413ce6617958b
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3764341
    Reviewed-by: Camillo Bruni <[email protected]>
    Commit-Queue: Marja Hölttä <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#81745}

Refs: v8/v8@00704f5
Refs: #32226

PR-URL: #43921
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Feng Yu <[email protected]>

danielleadams pushed a commit that referenced this pull request

Aug 16, 2022


          src: remove usages of GetBackingStore in startup

43e8be0

This removes all usages of GetBackingStore in startup. See the linked
issue for an explanation.

Refs: #32226
Refs: #43921
PR-URL: #44078
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Feng Yu <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>

danielleadams pushed a commit that referenced this pull request

Aug 16, 2022


          src: remove usages of GetBackingStore in WASI

ddf9c16

This removes all usages of GetBackingStore in WASI. See the linked issue
for an explanation.

Refs: #32226
Refs: #43921
PR-URL: #44077
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Feng Yu <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>

danielleadams pushed a commit that referenced this pull request

Aug 16, 2022


          src: remove usages of GetBackingStore in modules

34a92d1

This removes all usages of GetBackingStore in modules. See the linked
issue for an explanation.

Refs: #32226
Refs: #43921
PR-URL: #44076
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Feng Yu <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>

danielleadams pushed a commit that referenced this pull request

Aug 16, 2022


          src: remove usages of GetBackingStore in node-api

8a79636

This removes all usages of GetBackingStore in `node-api`. See the linked
issue for an explanation.

Refs: #32226
Refs: #43921
PR-URL: #44075
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Feng Yu <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>

danielleadams pushed a commit that referenced this pull request

Aug 16, 2022


          src: remove unowned usages of GetBackingStore

c7373b7

This removes all usages of GetBackingStore without any entries in the
`CODEOWNERS` file. For the most part this is a pretty straightforward
review; except `SPREAD_BUFFER_ARG` and the changes to `CopyArrayBuffer`.

See the linked issue for an explanation.

Refs: #32226
Refs: #43921
PR-URL: #44080
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>

danielleadams pushed a commit that referenced this pull request

Aug 16, 2022


          src: remove usages of GetBackingStore in crypto

ebff06b

This removes all usages of GetBackingStore in `crypto`. See the
linked issue for an explanation.

Note: I am not sure of the lifetime semantics intended by
`ArrayBufferOrViewContents` -- I am pretty sure it is correct based on
a manual audit of the callsites, but please ensure that it is correct.

Refs: #32226
Refs: #43921
PR-URL: #44079
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Feng Yu <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>

danielleadams mentioned this pull request

Aug 16, 2022

v18.8.0 release proposal #44246

Closed

ruyadorno pushed a commit that referenced this pull request

Aug 23, 2022


          deps: cherry-pick 00704f5a from V8 upstream

a0c5783

Original commit message:

    Add more efficient API for accesssing ArrayBuffer raw data

    Raw data access is already possible via GetBackingStore()->GetData().
    This API exposes a more efficient way for accessing
    JSArrayBuffer::backing_store (which, despite the confusing name, is no
    the BackingStore but its raw data pointer).

    Bug: v8:10343
    Change-Id: I695cea91e2c3de75ce6c86bac6e413ce6617958b
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3764341
    Reviewed-by: Camillo Bruni <[email protected]>
    Commit-Queue: Marja Hölttä <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#81745}

Refs: v8/v8@00704f5
Refs: #32226

PR-URL: #43921
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Feng Yu <[email protected]>

ruyadorno pushed a commit that referenced this pull request

Aug 23, 2022


          src: remove usages of GetBackingStore in startup

38cdb1f

This removes all usages of GetBackingStore in startup. See the linked
issue for an explanation.

Refs: #32226
Refs: #43921
PR-URL: #44078
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Feng Yu <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>

ruyadorno pushed a commit that referenced this pull request

Aug 23, 2022


          src: remove usages of GetBackingStore in WASI

a54e4d4

This removes all usages of GetBackingStore in WASI. See the linked issue
for an explanation.

Refs: #32226
Refs: #43921
PR-URL: #44077
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Feng Yu <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>

ruyadorno pushed a commit that referenced this pull request

Aug 23, 2022


          src: remove usages of GetBackingStore in modules

cddf3ed

This removes all usages of GetBackingStore in modules. See the linked
issue for an explanation.

Refs: #32226
Refs: #43921
PR-URL: #44076
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Feng Yu <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>

ruyadorno pushed a commit that referenced this pull request

Aug 23, 2022


          src: remove usages of GetBackingStore in node-api

8e1b7e2

This removes all usages of GetBackingStore in `node-api`. See the linked
issue for an explanation.

Refs: #32226
Refs: #43921
PR-URL: #44075
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Feng Yu <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>

ruyadorno pushed a commit that referenced this pull request

Aug 23, 2022


          src: remove unowned usages of GetBackingStore

a2022e5

This removes all usages of GetBackingStore without any entries in the
`CODEOWNERS` file. For the most part this is a pretty straightforward
review; except `SPREAD_BUFFER_ARG` and the changes to `CopyArrayBuffer`.

See the linked issue for an explanation.

Refs: #32226
Refs: #43921
PR-URL: #44080
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>

ruyadorno pushed a commit that referenced this pull request

Aug 23, 2022


          src: remove usages of GetBackingStore in crypto

4755ad5

This removes all usages of GetBackingStore in `crypto`. See the
linked issue for an explanation.

Note: I am not sure of the lifetime semantics intended by
`ArrayBufferOrViewContents` -- I am pretty sure it is correct based on
a manual audit of the callsites, but please ensure that it is correct.

Refs: #32226
Refs: #43921
PR-URL: #44079
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Feng Yu <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>

ruyadorno mentioned this pull request

Aug 23, 2022

v18.8.0 release proposal #44353

Merged

targos added the dont-land-on-v16.x label

Sep 5, 2022

JanMarvin mentioned this pull request

Nov 2, 2022

UBSAN error in GetBackingStore() jeroen/V8#152

Closed

Reviewers

gengjiawen

gengjiawen approved these changes

+2 more reviewers

targos

targos approved these changes

F3n67u

F3n67u approved these changes

Reviewers whose approvals may not affect merge requirements

Uh oh!

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

Assignees

No one assigned

Labels

author ready

PRs with CI started, the required approvals, and no outstanding review comments.

commit-queue-squash

PRs the Commit Queue should land as one squashed commit.

needs-ci

PRs that need a full CI run.

v8 engine

Issues and PRs related to the V8 dependency.

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.

9 participants

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.