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: migrate to newer V8 ArrayBuffer APIs #30339

Closed
wants to merge 2 commits into from

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Nov 8, 2019

src: migrate off ArrayBuffer::GetContents

V8 deprecates GetContents() in favour of GetBackingStore().
Update our code to reflect that.
V8 also deprecates Externalize() and IsExternal(); we should
be able to remove all usage of this once V8 8.0 is there.

Refs: v8/v8@bfe3d6b

src: drop CallbackInfo for Buffer lifetime tracking

This has been rendered unnecessary by upstream V8 changes,
which now allow user-provided deleter functions out of the box.

(Sadly, that function signature mismatches our deleter function
signature, so we still need a heap allocation to track our deleter
data.)

Refs: v8/v8@bba5f1f

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

V8 deprecates `GetContents()` in favour of `GetBackingStore()`.
Update our code to reflect that.
V8 also deprecates `Externalize()` and `IsExternal()`; we should
be able to remove all usage of this once V8 8.0 is there.

Refs: v8/v8@bfe3d6b
@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. labels Nov 8, 2019
@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@devsnek devsnek left a comment

Choose a reason for hiding this comment

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

🎉

Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

LGTM, and thanks for the heads up in the WASI PR.

@addaleax
Copy link
Member Author

I’ve dropped the second commit (be7e6e6ecf829fa859dab7a37f3732f021fb1b57) for now, and restored the IsExternal() condition in node_messaging.cc.

The reason for the CI failures was that V8’s BackingStoreDeleterCallback was invoked directly by GC, and not in the GC epilogue like weak callbacks. This meant that the callback could be invoked by any thread, and we’d have to emulate the previous behaviour of running it on the main thread somehow.

Two possible solutions that I’ve discarded for now:

  1. Use the platform’s GetForegroundTaskRunner() and schedule a foreground task ourselves. This doesn’t work well because it won’t work for embedders that don’t provide a platform implementation to Node.js (only to V8).
  2. Use an uv_async_t on Environment or similar. This doesn’t work well because ArrayBuffers can outlive Environments (and even Isolates).

Ultimately, I think the best solution is sadly an API migration/change to something closer to what V8 does. The fact that ArrayBuffers can be transferred between Isolates means that, in an ideal situation, the free callback should not rely what on what thread it is invoked on; we currently only work around this by copying external ArrayBuffers rather than transferring them.

That this design flaw has made it’s way into N-API is unfortunate. Maybe we can eventually tag ArrayBuffers created through these APIs (the current Buffer::New() with a callback and napi_create_external_{array,}buffer) as non-transferable in some way, and handle all other ArrayBuffers properly.

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 10, 2019
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@danbev
Copy link
Contributor

danbev commented Nov 12, 2019

Landed in e66a2ac.

@danbev danbev closed this Nov 12, 2019
danbev pushed a commit that referenced this pull request Nov 12, 2019
V8 deprecates `GetContents()` in favour of `GetBackingStore()`.
Update our code to reflect that.
V8 also deprecates `Externalize()` and `IsExternal()`; we should
be able to remove all usage of this once V8 8.0 is there.

PR-URL: #30339
Refs: v8/v8@bfe3d6b
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: David Carlier <[email protected]>
@addaleax addaleax deleted the no-get-contents branch November 12, 2019 09:37
addaleax added a commit to addaleax/node that referenced this pull request Nov 13, 2019
More precisely, make them untransferable if they were created through
*our* APIs, because those do not follow the improved free callback
mechanism that V8 uses now. All other ArrayBuffers can be transferred
between threads now, the assumption being that they were created in a
clean way that follows the V8 API on this.

This addresses a TODO comment.

Refs: nodejs#30339 (comment)
@MylesBorins
Copy link
Contributor

If we backport 7.9 to 13.x we might want to include this. Thoughts @addaleax?

addaleax added a commit that referenced this pull request Nov 19, 2019
More precisely, make them untransferable if they were created through
*our* APIs, because those do not follow the improved free callback
mechanism that V8 uses now. All other ArrayBuffers can be transferred
between threads now, the assumption being that they were created in a
clean way that follows the V8 API on this.

This addresses a TODO comment.

Refs: #30339 (comment)

PR-URL: #30475
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: David Carlier <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 21, 2019
V8 deprecates `GetContents()` in favour of `GetBackingStore()`.
Update our code to reflect that.
V8 also deprecates `Externalize()` and `IsExternal()`; we should
be able to remove all usage of this once V8 8.0 is there.

PR-URL: #30339
Refs: v8/v8@bfe3d6b
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: David Carlier <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 21, 2019
More precisely, make them untransferable if they were created through
*our* APIs, because those do not follow the improved free callback
mechanism that V8 uses now. All other ArrayBuffers can be transferred
between threads now, the assumption being that they were created in a
clean way that follows the V8 API on this.

This addresses a TODO comment.

Refs: #30339 (comment)

PR-URL: #30475
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: David Carlier <[email protected]>
@BridgeAR BridgeAR mentioned this pull request Nov 21, 2019
lovell added a commit to lovell/sharp that referenced this pull request Nov 28, 2019
addaleax added a commit to addaleax/node that referenced this pull request Jan 14, 2020
More precisely, make them untransferable if they were created through
*our* APIs, because those do not follow the improved free callback
mechanism that V8 uses now. All other ArrayBuffers can be transferred
between threads now, the assumption being that they were created in a
clean way that follows the V8 API on this.

This addresses a TODO comment.

Refs: nodejs#30339 (comment)

PR-URL: nodejs#30475
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: David Carlier <[email protected]>
targos pushed a commit that referenced this pull request Jan 14, 2020
More precisely, make them untransferable if they were created through
*our* APIs, because those do not follow the improved free callback
mechanism that V8 uses now. All other ArrayBuffers can be transferred
between threads now, the assumption being that they were created in a
clean way that follows the V8 API on this.

This addresses a TODO comment.

Refs: #30339 (comment)

PR-URL: #30475
Backport-PR-URL: #31355
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: David Carlier <[email protected]>
targos pushed a commit that referenced this pull request Jan 14, 2020
More precisely, make them untransferable if they were created through
*our* APIs, because those do not follow the improved free callback
mechanism that V8 uses now. All other ArrayBuffers can be transferred
between threads now, the assumption being that they were created in a
clean way that follows the V8 API on this.

This addresses a TODO comment.

Refs: #30339 (comment)

PR-URL: #30475
Backport-PR-URL: #31355
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: David Carlier <[email protected]>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
More precisely, make them untransferable if they were created through
*our* APIs, because those do not follow the improved free callback
mechanism that V8 uses now. All other ArrayBuffers can be transferred
between threads now, the assumption being that they were created in a
clean way that follows the V8 API on this.

This addresses a TODO comment.

Refs: #30339 (comment)

PR-URL: #30475
Backport-PR-URL: #31355
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: David Carlier <[email protected]>
gabylb added a commit to ibmruntimes/node-java that referenced this pull request Jun 26, 2020
webgurucan added a commit to webgurucan/node-image-handler that referenced this pull request Mar 27, 2022
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants