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
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 <mdawson@devrus.com>
Signed-off-by: Michael Dawson <mdawson@devrus.com>
Signed-off-by: Michael Dawson <mdawson@devrus.com>
|
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 <mdawson@devrus.com> PR-URL: #43611 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
|
@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 <mdawson@devrus.com> PR-URL: #43611 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
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 <mdawson@devrus.com> PR-URL: nodejs/node#43611 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
|
Closing in favor of #45038 |
Signed-off-by: Michael Dawson mdawson@devrus.com