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.

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

src: add internal isArrayBufferDetached #45568

Merged

Conversation

anonrig
Copy link
Member

@anonrig anonrig commented Nov 21, 2022

Follow up on #45512

@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 Nov 21, 2022
@anonrig anonrig force-pushed the util/is-array-buffer-detached branch 2 times, most recently from 7d64817 to dcee530 Compare November 21, 2022 23:40
@anonrig anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 21, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 21, 2022
@nodejs-github-bot
Copy link
Collaborator

@anonrig anonrig added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 21, 2022
@anonrig anonrig force-pushed the util/is-array-buffer-detached branch from dcee530 to 4d5d53b Compare November 22, 2022 22:16
@anonrig anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 22, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 22, 2022
@nodejs-github-bot
Copy link
Collaborator

@@ -560,6 +562,14 @@ function SideEffectFreeRegExpPrototypeExec(regex, string) {
return FunctionPrototypeCall(RegExpFromAnotherRealm.prototype.exec, regex, string);
}

function isArrayBufferDetached(value) {
if (ArrayBufferPrototypeGetByteLength(value) === 0) {
return _isArrayBufferDetached(value);
Copy link
Contributor

Choose a reason for hiding this comment

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

Crossing the JS-C++ boundary is gonna be slower than the current implementation although I'm not sure if this call in the zero length case is gonna be the primary bottleneck for any user code.

Did you consider implementing @jasnell's suggestion in #45512 (comment)? Seems like that approach would solve this problem without adding any performance overhead in any case.

One thing we might be able to do is add a hidden symbol to ArrayBuffer instances when we detach them in c++ land. Then we can have an isDetached check in js that checks the length and the presence of that symbol.

Copy link
Member Author

@anonrig anonrig Nov 23, 2022

Choose a reason for hiding this comment

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

IMHO, that is the perfect solution for performance, but I believe it's too hacky to maintain in the long term. I can't think of a good/maintainable way to enforce attaching the hidden symbol as a rule.

If it's the consensus @jasnell thinks is the best way, I'll be happy to implement it regardless.

@nodejs-github-bot
Copy link
Collaborator

Copy link
Contributor

@LiviaMedeiros LiviaMedeiros left a comment

Choose a reason for hiding this comment

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

LGTM with or without a nit.

Adding a test would also be nice.

Comment on lines +566 to +570
if (ArrayBufferPrototypeGetByteLength(value) === 0) {
return _isArrayBufferDetached(value);
}

return false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: the flow might look semantically better like this

Suggested change
if (ArrayBufferPrototypeGetByteLength(value) === 0) {
return _isArrayBufferDetached(value);
}
return false;
if (ArrayBufferPrototypeGetByteLength(value) !== 0) {
return false;
}
return _isArrayBufferDetached(value);

@nodejs-github-bot
Copy link
Collaborator

@anonrig anonrig added the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 27, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 27, 2022
@nodejs-github-bot nodejs-github-bot merged commit 9a8d82b into nodejs:main Nov 27, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in 9a8d82b

ErickWendel pushed a commit to ErickWendel/node that referenced this pull request Nov 30, 2022
PR-URL: nodejs#45568
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: LiviaMedeiros <livia@cirno.name>
targos pushed a commit that referenced this pull request Dec 12, 2022
PR-URL: #45568
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: LiviaMedeiros <livia@cirno.name>
targos pushed a commit that referenced this pull request Dec 13, 2022
PR-URL: #45568
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: LiviaMedeiros <livia@cirno.name>
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
PR-URL: #45568
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: LiviaMedeiros <livia@cirno.name>
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
PR-URL: #45568
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: LiviaMedeiros <livia@cirno.name>
danielleadams pushed a commit that referenced this pull request Jan 3, 2023
PR-URL: #45568
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: LiviaMedeiros <livia@cirno.name>
danielleadams pushed a commit that referenced this pull request Jan 4, 2023
PR-URL: #45568
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: LiviaMedeiros <livia@cirno.name>
danielleadams pushed a commit that referenced this pull request Jan 5, 2023
PR-URL: #45568
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: LiviaMedeiros <livia@cirno.name>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants