embedding: refactor NewIsolate() #26525
Conversation
Use a RAII approach by default, and make it possible for embedders to use the `ArrayBufferAllocator` directly as a V8 `ArrayBuffer::Allocator`, e.g. when passing to `Isolate::CreateParams` manually.
Split the API up into its essential parts, namely setting up the creation parameters for the Isolate, creating it, and performing Node.js-specific customization afterwards.
addaleax
added
semver-minor
nodejs-github-bot
added
c++
| NODE_EXTERN v8::Isolate* NewIsolate(ArrayBufferAllocator* allocator, | ||
| struct uv_loop_s* event_loop); | ||
| NODE_EXTERN v8::Isolate* NewIsolate(ArrayBufferAllocator* allocator, | ||
| struct uv_loop_s* event_loop, | ||
| MultiIsolatePlatform* platform); | ||
|
|
There was a problem hiding this comment.
Does it make sense to have a NewIsolate constructor that takes only the event_loop , and builds one from the default allocator?
There was a problem hiding this comment.
How would we transfer ownership of the allocator in that case?
There was a problem hiding this comment.
like, say for example a class of embedder is not interested in creating or managing an allocator, so not interested in its ownership?
furthermore, do we have a guidance (or user data) that suggests the desired / recommended level of embedding node? With these items (v8, uv loop, allocator, isolate, env, inspector and probably more) that can be custom-created or delegated, what level of control we foresee to give to embedders, and what to keep within node?
There was a problem hiding this comment.
(I should admit that I don't have insight on this, and did not work with any embedding users that had reported issues or improvements in this area)
There was a problem hiding this comment.
like, say for example a class of embedder is not interested in creating or managing an allocator, so not interested in its ownership?
Yeah, I mean, I understand why you asked, I just don’t see how we could technically realize this without significant complexity (or maybe at all)? And the embedder would likely okay be okay with maintaining a reference to the allocator, because it has to do that for the Isolate already and can use the same code paths for that.
furthermore, do we have a guidance (or user data) that suggests the desired / recommended level of
embeddingnode? With these items (v8, uv loop, allocator, isolate, env, inspector and probably more) that can be custom-created or delegated, what level of control we foresee to give to embedders, and what to keep within node?
Yeah, it’s a bit difficult to tell where to draw the line. I’d prefer to give embedders more control though if in doubt, because they might need it (for example, Electron hooks into Node’s internals a lot and it would be great to make that unnecessary).
There was a problem hiding this comment.
ok, thanks for the explanation, make sense to me!
|
Resume CI again: https://ci.nodejs.org/job/node-test-pull-request/21463/ |
|
CI failures were because I was dumb and always forced debug mode for ArrayBuffer allocations by default, which is definitely not what I wanted. It’s nice to know our test suite catches that in terms of resource usage and otherwise passes, though. CI: https://ci.nodejs.org/job/node-test-pull-request/21489/ (✔️) |
addaleax
added
the
author ready
|
Landed in 17ab2ed...b21e7c7 |
Use a RAII approach by default, and make it possible for embedders to use the `ArrayBufferAllocator` directly as a V8 `ArrayBuffer::Allocator`, e.g. when passing to `Isolate::CreateParams` manually. PR-URL: #26525 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
Split the API up into its essential parts, namely setting up the creation parameters for the Isolate, creating it, and performing Node.js-specific customization afterwards. PR-URL: #26525 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
Use a RAII approach by default, and make it possible for embedders to use the `ArrayBufferAllocator` directly as a V8 `ArrayBuffer::Allocator`, e.g. when passing to `Isolate::CreateParams` manually. PR-URL: #26525 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
Split the API up into its essential parts, namely setting up the creation parameters for the Isolate, creating it, and performing Node.js-specific customization afterwards. PR-URL: #26525 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
Use a RAII approach by default, and make it possible for embedders to use the `ArrayBufferAllocator` directly as a V8 `ArrayBuffer::Allocator`, e.g. when passing to `Isolate::CreateParams` manually. PR-URL: #26525 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
Split the API up into its essential parts, namely setting up the creation parameters for the Isolate, creating it, and performing Node.js-specific customization afterwards. PR-URL: #26525 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
|
@nodejs/embedders, should this land on v10.x? Please add the |
embedding: refactor public
ArrayBufferAllocatorAPIUse a RAII approach by default, and make it possible for embedders
to use the
ArrayBufferAllocatordirectly as a V8ArrayBuffer::Allocator, e.g. when passing toIsolate::CreateParamsmanually.
embedding: make
NewIsolate()API more flexibleSplit the API up into its essential parts, namely setting up
the creation parameters for the Isolate, creating it, and performing
Node.js-specific customization afterwards.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes