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

Flaky test-single-executable-application #47741

Closed
tniessen opened this issue Apr 27, 2023 · 7 comments · Fixed by #47588
Closed

Flaky test-single-executable-application #47741

tniessen opened this issue Apr 27, 2023 · 7 comments · Fixed by #47588
Labels
flaky-test Issues and PRs related to the tests with unstable failures on the CI. macos Issues and PRs related to the macOS platform / OSX. single-executable Issues and PRs related to single-executable applications

Comments

@tniessen
Copy link
Member

tniessen commented Apr 27, 2023

Test

test-single-executable-application

Platform

macOS x64

Console output

00:23:42 not ok 2739 parallel/test-single-executable-application
00:23:42   ---
00:23:42   duration_ms: 120056.97200
00:23:42   severity: fail
00:23:42   exitcode: -15
00:23:42   stack: |-
00:23:42     timeout
00:23:42     Wrote single executable preparation blob to sea-prep.blob
00:23:42   ...

Build links

Additional information

@tniessen tniessen added flaky-test Issues and PRs related to the tests with unstable failures on the CI. single-executable Issues and PRs related to single-executable applications labels Apr 27, 2023
@github-actions github-actions bot added the macos Issues and PRs related to the macOS platform / OSX. label Apr 27, 2023
@RaisinTen
Copy link
Contributor

I noticed it too. It became more prominent when I added another parallel e2e sea test in #47588. After I moved the test to sequential, the next fresh CI run didn't have any related failures.

@debadree25
Copy link
Member

It seems it gets stuck somewhere over here when run in parallel

node/src/node_sea.cc

Lines 160 to 163 in e8cfa95

FPrintF(stderr,
"Wrote single executable preparation blob to %s\n",
config.output_path);

@RaisinTen
Copy link
Contributor

My hunch is that this has something to do with how the postject wasm blob uses memory - https://github.com/nodejs/postject/blob/3edd1dd1e0690167d7db0f502620e41888b2f82e/CMakeLists.txt#L23:

-sALLOW_MEMORY_GROWTH -sINITIAL_MEMORY=268435456 -sMAXIMUM_MEMORY=4294967296

From my testing, using LIEF purely through the c++ APIs is relatively faster than using it through wasm, which is what we're doing in postject.

Not sure at the moment how this could be improved though.

Also, cc @nodejs/single-executable

@joyeecheung
Copy link
Member

Can we just use a C++ addon instead of going throw WASM? I think postject locally is also surprisingly slow considering it's doing a pretty straight-forward thing...

@RaisinTen
Copy link
Contributor

RaisinTen commented Apr 28, 2023

@joyeecheung I had asked about whether implementing the injector in JS (i.e., without native deps) is necessary in @mhdawson's initial SEA PR and Jesse, one of the primary maintainers of pkg, said #42334 (comment):

Yes. Portability suffers and complexity greatly increases if we have to depend on some native utilities. Also, it becomes harder to promote the feature if user/developer found it hard to use.

So IIUC, if we do this, we would need to maintain prebuilt LIEF binaries for all supported platforms and architectures, even though LIEF's code doesn't really do platform-specific stuff. But since postject uses wasm, we only need to maintain a single blob and that works on all platforms.

@mhdawson
Copy link
Member

mhdawson commented May 2, 2023

I think the extra overhead/complications of having to maintain prebuilt LIEF binaries, or building them as part of postject would add significant work/complications.

It might be better to leave as is since @RaisinTen mentioned that the test issue can be addressed by a move to sequential and look to see if we can understand why it's slower than the native implementation. Maybe there is something in our uvwasi implementation that needs to be optimized or maybe LIEF is doing reads/writes in a way that magnifies overhead in WASM (as the only thing I can think of being slower in WASM is the read/write of the binary)

nodejs-github-bot pushed a commit that referenced this issue May 4, 2023
Refs: nodejs/single-executable#60
Signed-off-by: Darshan Sen <raisinten@gmail.com>
PR-URL: #47588
Fixes: #47741
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Tierney Cyren <hello@bnb.im>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
targos pushed a commit that referenced this issue May 12, 2023
Refs: nodejs/single-executable#60
Signed-off-by: Darshan Sen <raisinten@gmail.com>
PR-URL: #47588
Fixes: #47741
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Tierney Cyren <hello@bnb.im>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@RaisinTen
Copy link
Contributor

RaisinTen commented May 16, 2023

Btw, I have found out the bottleneck that was causing Postject to be so slow in nodejs/postject#85 and I have created a PR to fix it - nodejs/postject#86.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flaky-test Issues and PRs related to the tests with unstable failures on the CI. macos Issues and PRs related to the macOS platform / OSX. single-executable Issues and PRs related to single-executable applications
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants