-
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
Flaky test-single-executable-application #47741
Comments
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. |
It seems it gets stuck somewhere over here when run in parallel Lines 160 to 163 in e8cfa95
|
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:
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 |
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... |
@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):
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. |
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) |
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>
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>
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. |
Test
test-single-executable-application
Platform
macOS x64
Console output
Build links
Additional information
The text was updated successfully, but these errors were encountered: