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

src: remove unowned usages of GetBackingStore #44080

Merged
merged 2 commits into from
Aug 3, 2022
Merged

src: remove unowned usages of GetBackingStore #44080

merged 2 commits into from
Aug 3, 2022

Conversation

kvakil
Copy link
Contributor

@kvakil kvakil commented Aug 1, 2022

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

@kvakil
src: remove unowned usages of GetBackingStore
24a94f0 
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: nodejs#32226
Refs: nodejs#43921
@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 Aug 1, 2022
@kvakil kvakil added the buffer Issues and PRs related to the buffer subsystem. label Aug 1, 2022
@kvakil
Copy link
Contributor Author

kvakil commented Aug 1, 2022

cc @nodejs/buffer

There are also the following related PRs, but I'm opting not to CC the group on all of them, since they already have codeowners:

  1. src: remove usages of GetBackingStore in crypto #44079
  2. src: remove usages of GetBackingStore in startup #44078
  3. src: remove usages of GetBackingStore in WASI #44077
  4. src: remove usages of GetBackingStore in modules #44076
  5. src: remove usages of GetBackingStore in node-api #44075

RaisinTen
RaisinTen approved these changes Aug 1, 2022
View reviewed changes
Copy link
Contributor

@RaisinTen RaisinTen left a comment

Choose a reason for hiding this comment

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

LGTM

src/node_buffer.cc Outdated Show resolved Hide resolved
kvakil
kvakil commented Aug 1, 2022
View reviewed changes
src/node_buffer.cc
Comment on lines -1255 to -1256
} else if (args[0]->IsSharedArrayBuffer()) {
source = args[2].As<SharedArrayBuffer>()->GetBackingStore();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nodejs/whatwg-stream: this is a typo: the first args[0] should be args[2]. I don't think it ends up being an issue in practice. The only user of copyArrayBuffer is in lib/internal/webstreams/readablestream.js, and it doesn't seem possible to pass in a SharedArrayBuffer to either of its arguments.

Anyway, the code motion in this diff fixes the issue anyway.

mcollina
mcollina approved these changes Aug 1, 2022
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@kvakil kvakil added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 1, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 1, 2022
@nodejs-github-bot
Copy link
Collaborator

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

@github-actions github-actions bot mentioned this pull request Aug 2, 2022
Open
11 tasks
@kvakil kvakil added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 2, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 2, 2022
@nodejs-github-bot
Copy link
Collaborator

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

@kvakil kvakil added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 2, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 2, 2022
@nodejs-github-bot
Copy link
Collaborator

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

@kvakil kvakil added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 2, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 2, 2022
@nodejs-github-bot
Copy link
Collaborator

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

@kvakil kvakil added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 2, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 2, 2022
@kvakil kvakil added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 2, 2022
@nodejs-github-bot
Copy link
Collaborator

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

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 2, 2022
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@kvakil kvakil added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 3, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 3, 2022
@nodejs-github-bot
Copy link
Collaborator

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

@kvakil kvakil added request-ci Add this label to start a Jenkins CI on a PR. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Aug 3, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 3, 2022
@nodejs-github-bot
Copy link
Collaborator

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

@F3n67u F3n67u added the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 3, 2022
@kvakil kvakil added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Aug 3, 2022
@F3n67u
Copy link
Member

F3n67u commented Aug 3, 2022

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

@kvakil FYI. Jenkins CI run has two modes: fresh build and Resume Build. Adding request-ci label will trigger a fresh build, and everything will start fresh. Resume Build mode will only retry the failed test which is more likely to hit a retry success. But Resume Build mode could only be triggered by a collaborator in Jenkins CI. see https://github.com/nodejs/node/blob/main/doc/contributing/collaborator-guide.md#testing-and-ci for more details.

If Jenkins CI fails often(like in this pr), it would be better to let a collaborator trigger a resume build, instead of restarting a fresh test every time. In long term, we should solve those test flaky or test infra problems. So if you could help to take a look at those problems when ci flaky, it will be great.

@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 3, 2022
@nodejs-github-bot nodejs-github-bot merged commit 8c35a4e into nodejs:main Aug 3, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in 8c35a4e

@kvakil
Copy link
Contributor Author

kvakil commented Aug 3, 2022 via email

This was referenced Aug 5, 2022
Open
Open
Open
Open
danielleadams pushed a commit that referenced this pull request Aug 16, 2022
@kvakil @danielleadams
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 danielleadams mentioned this pull request Aug 16, 2022
Closed
ruyadorno pushed a commit that referenced this pull request Aug 23, 2022
@kvakil @ruyadorno
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 ruyadorno mentioned this pull request Aug 23, 2022
Merged
kvakil added a commit to kvakil/node that referenced this pull request Sep 9, 2022
@kvakil
buffer: revert GetBackingStore optimization in API
8df27df 
In an earlier PR, I replaced a lot of instances of
`GetBackingStore()->Data()` with `Data()`. Apparently these two are not
equivalent in the case of zero-length buffers: the former returns a
"valid" address, while the latter returns `NULL`. At least one library
in the ecosystem (see the referenced issue) abuses zero-length buffers
to wrap arbitrary pointers, which is broken by this difference. It is
unfortunate that every library needs to take a performance hit because
of this edge-case, and somebody should figure out if this is actually a
reasonable contract to uphold long-term.

I have not traced down exactly why this divergence occurs, but I have
verified that reverting this line fixes the referenced issue.

Refs: nodejs#44080
Fixes: nodejs#44554
@kvakil kvakil mentioned this pull request Sep 9, 2022
Open
kvakil added a commit to kvakil/node that referenced this pull request Sep 9, 2022
@kvakil
buffer: revert GetBackingStore optimization in API
5ca238a 
In an earlier PR, I replaced a lot of instances of
`GetBackingStore()->Data()` with `Data()`. Apparently these two are not
equivalent in the case of zero-length buffers: the former returns a
"valid" address, while the latter returns `NULL`. At least one library
in the ecosystem (see the referenced issue) abuses zero-length buffers
to wrap arbitrary pointers, which is broken by this difference. It is
unfortunate that every library needs to take a performance hit because
of this edge-case, and somebody should figure out if this is actually a
reasonable contract to uphold long-term.

I have not traced down exactly why this divergence occurs, but I have
verified that reverting this line fixes the referenced issue.

Refs: nodejs#44080
Fixes: nodejs#44554
tniessen pushed a commit to kvakil/node that referenced this pull request Sep 12, 2022
@kvakil @tniessen
buffer: revert GetBackingStore optimization in API
c947744 
In an earlier PR, I replaced a lot of instances of
`GetBackingStore()->Data()` with `Data()`. Apparently these two are not
equivalent in the case of zero-length buffers: the former returns a
"valid" address, while the latter returns `NULL`. At least one library
in the ecosystem (see the referenced issue) abuses zero-length buffers
to wrap arbitrary pointers, which is broken by this difference. It is
unfortunate that every library needs to take a performance hit because
of this edge-case, and somebody should figure out if this is actually a
reasonable contract to uphold long-term.

I have not traced down exactly why this divergence occurs, but I have
verified that reverting this line fixes the referenced issue.

Refs: nodejs#44080
Fixes: nodejs#44554
Fyko pushed a commit to Fyko/node that referenced this pull request Sep 15, 2022
@kvakil @Fyko
src: remove unowned usages of GetBackingStore
6d9f3ce 
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: nodejs#32226
Refs: nodejs#43921
PR-URL: nodejs#44080
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RaisinTen RaisinTen RaisinTen approved these changes

@addaleax addaleax addaleax approved these changes

@mcollina mcollina mcollina approved these changes

Assignees
No one assigned
Labels
buffer Issues and PRs related to the buffer subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. 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.
7 participants
@kvakil @nodejs-github-bot @F3n67u @mcollina @addaleax @RaisinTen @targos