-
Notifications
You must be signed in to change notification settings - Fork 3k
chore: speedup builds for binary and prepackage #6022
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
Conversation
- 70s to 20s
- reduction by 4 seconds
Your cubic subscription is currently inactive. Please reactivate your subscription to receive AI reviews and use cubic. |
✅ Deploy Preview for continuedev ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
This reverts commit 6ca088f.
|
if (msg.error) { | ||
reject(); | ||
} | ||
resolve(); |
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.
In the copySqlite function, the promise is resolved unconditionally after the error check. If msg.error is true, the code will still call resolve() right after calling reject(), leading to an inconsistent promise state. The resolve() call should be in an else block to ensure mutual exclusion with the error case.
React with 👍 to tell me that this comment was useful, or 👎 if not (and I'll stop posting more comments like this in the future)
if (msg.error) { | ||
reject(); | ||
} | ||
resolve(); |
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.
Similar to the copySqlite function, the copyEsbuild function has the same promise resolution issue where resolve() is called unconditionally after the error check. The resolve() call should be in an else block to prevent resolving after an error has occurred.
React with 👍 to tell me that this comment was useful, or 👎 if not (and I'll stop posting more comments like this in the future)
😱 Found 2 issues. Time to roll up your sleeves! 😱 |
normal.extension.launch.mp4faster.extension.launch.mp4 |
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.
I tested this on my machine, inspected most of the code to be sure it is really just a refactor + parallelization, had multiple LLM review sessions, and tested building a version of the extension. All worked. This is great. We should keep an eye on it though, because anything in the build process has potential to cause very serious problems
Description
Using child processes and parallel executions, this PR accelerates installations, copying and downloads. In most cases, the performance improvement is more than 2x.
installNodeModuleInTempDirAndCopyToCurrent
intoinstall-copy-nodemodule.js
for copying lancedb targets on parallel in child processnpm-install.js
download-copy-sqlite-esbuild.js
bundle-binary.js
generate-copy-config.js
performance improvements from my machine:
CON-2267
Checklist
Screenshots
[ For visual changes, include screenshots. Screen recordings are particularly helpful, and appreciated! ]
Tests
[ What tests were added or updated to ensure the changes work as expected? ]