-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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,doc: Experimental support for SEA #42334
Conversation
Review requested:
|
@igorklopov, @jesec, @styfle could you look at what is documented/the proof of concept to confirm this is what we talked about in the next-10 mini-summit and that it's what is needed. If so would be great if you could help fill in the windows/osx versions. |
doc/contributing/maintaining-single-executable-application-support.md
Outdated
Show resolved
Hide resolved
doc/contributing/maintaining-single-executable-application-support.md
Outdated
Show resolved
Hide resolved
doc/contributing/maintaining-single-executable-application-support.md
Outdated
Show resolved
Hide resolved
doc/contributing/maintaining-single-executable-application-support.md
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
doc/contributing/maintaining-single-executable-application-support.md
Outdated
Show resolved
Hide resolved
doc/contributing/maintaining-single-executable-application-support.md
Outdated
Show resolved
Hide resolved
doc/contributing/maintaining-single-executable-application-support.md
Outdated
Show resolved
Hide resolved
doc/contributing/maintaining-single-executable-application-support.md
Outdated
Show resolved
Hide resolved
doc/contributing/maintaining-single-executable-application-support.md
Outdated
Show resolved
Hide resolved
doc/contributing/maintaining-single-executable-application-support.md
Outdated
Show resolved
Hide resolved
doc/contributing/maintaining-single-executable-application-support.md
Outdated
Show resolved
Hide resolved
doc/contributing/maintaining-single-executable-application-support.md
Outdated
Show resolved
Hide resolved
doc/contributing/maintaining-single-executable-application-support.md
Outdated
Show resolved
Hide resolved
doc/contributing/maintaining-single-executable-application-support.md
Outdated
Show resolved
Hide resolved
doc/contributing/maintaining-single-executable-application-support.md
Outdated
Show resolved
Hide resolved
doc/contributing/maintaining-single-executable-application-support.md
Outdated
Show resolved
Hide resolved
doc/contributing/maintaining-single-executable-application-support.md
Outdated
Show resolved
Hide resolved
doc/contributing/maintaining-single-executable-application-support.md
Outdated
Show resolved
Hide resolved
doc/contributing/maintaining-single-executable-application-support.md
Outdated
Show resolved
Hide resolved
doc/contributing/maintaining-single-executable-application-support.md
Outdated
Show resolved
Hide resolved
doc/contributing/maintaining-single-executable-application-support.md
Outdated
Show resolved
Hide resolved
doc/contributing/maintaining-single-executable-application-support.md
Outdated
Show resolved
Hide resolved
doc/contributing/maintaining-single-executable-application-support.md
Outdated
Show resolved
Hide resolved
Signed-off-by: Michael Dawson <[email protected]>
Signed-off-by: Michael Dawson <[email protected]>
Signed-off-by: Michael Dawson <[email protected]>
Pushed some additional commits to
|
Btw, I have a working implementation of SEA in my branch that uses postject main...RaisinTen:node:sea. It also implements the VFS (asar in this case)! The major problem I can think of is that the bootstrap script assumes that I'm using asar as the vfs, so we should probably find a way to allow users to configure it - perhaps by exposing an interface that folks can implement to hook in their custom vfs implementations? Or maybe some other approach that I couldn't think of? |
I tried to expose the embedded data as a buffer on |
Or simply treat VFS as out-of-scope and let the packager utility / embedded script do it. Implementing VFS inside Node.js does not help on its own, since a packager utility is still necessary after all. It is complicated to override Node.js APIs. Plus, there are many edge cases to handle and changes could easily become semver-major. It would significantly increase the maintenance burden for Node.js without tangible benefits (unless something like "node compile" is implemented along with it). |
I am a bit concerned about the case that the payload could be huge. It is not efficient to prematurely store the payload in memory. |
I had wondered about your comment about 4096 being big enough. In the case of using an additional segment it will already be in memory so perhaps less of a concern. Was the offset you were exposing an offset into the file instead of memory? |
I believe we should keep it as out-of-scope at least to start. Having the SEA support be minimal I think is best and we can then possibly adjust that based on feedback later on. |
But wouldn't that promote monkey-patching in Node.js? I'm not sure if that's necessarily a good thing that we want to promote.
I totally agree with your points but if we support SEA in node, it would mean that we should at least support a way to implement a VFS, even if we don't provide a complete VFS implementation out of the box. The reason is that SEA won't be usable if node can't make sense of the data that was embedded in it. I can think of 2 solutions here:
|
The current approach allocates a I wonder if there's a way to prevent this extra allocation. 🤔 |
@RaisinTen If you look at what is in this PR, if isntead of providing the address we provided a buffer created on the native side by passing in the pointer to the binary data it would avoid the copy in a similar manner to now it avoids copying any of the parameters/scripts passed. ie when its in a segement it just uses the memory already loaded when the binary was run. |
@RaisinTen in particular if you look at the Node-api code here: napi_status NAPI_CDECL napi_create_external_buffer(napi_env env, API docs That should be creating a buffer without allocating/copying. It does still mean that if the addition is large it will be loaded into memory. Having the binary data come from a read to the exe on disk might still have an advantage if that is done as needed as opposed to pre-loaded. |
This may be being discussed as part of the loaders effort. -> #41076 |
Refs: https://github.com/nodejs/node/issues/43432 Refs: #42334 Refs: https://github.com/nodejs/node/blob/main/doc/contributing/technical-priorities.md#single-executable-applications Signed-off-by: Michael Dawson <[email protected]> PR-URL: #43611 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
@RaisinTen it would be good to work towards a PR that we might land. Any chance you could PR changes you think are needed to this PR? I think at very least your changes to provide access to a buffer instead of the address would make sense. It would also be a good way to discuss any others and start heading towards 1 shared patch versus us each working on possibly different ones. |
I would be happy to send a PR for this but last week I had arranged a meeting between the Postject team and @jesec where we found out that there are some areas that need some more research, like these ones: VFS:
Postject:
... etc. I believe, having a team in node where we can discuss everything about SEA would be the right thing to do here. I've DM'ed you to learn about the process for creating a team and I'll follow that. |
Refs: https://github.com/nodejs/node/issues/43432 Refs: #42334 Refs: https://github.com/nodejs/node/blob/main/doc/contributing/technical-priorities.md#single-executable-applications Signed-off-by: Michael Dawson <[email protected]> PR-URL: #43611 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
Refs: https://github.com/nodejs/node/issues/43432 Refs: nodejs/node#42334 Refs: https://github.com/nodejs/node/blob/main/doc/contributing/technical-priorities.md#single-executable-applications Signed-off-by: Michael Dawson <[email protected]> PR-URL: nodejs/node#43611 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
Closing in favor of #45038 |
Signed-off-by: Michael Dawson [email protected]