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

perf(npm): use linked binary package for startup boost #2920

Closed

Conversation

dalisoft
Copy link

@dalisoft dalisoft commented Apr 8, 2024

This changes/PR should improve startup performance of CLI by up 8 times and very huge impact for lint-staged, pre-commit and pre-push commits

Benchmark using hyperfine

Command Mean [ms] Min [ms] Max [ms] Relative
oxlint 51.7 ± 0.4 51.0 52.2 7.84 ± 0.33
./node_modules/@oxlint/darwin-arm64/oxlint 6.6 ± 0.3 6.3 7.1 1.00
Benchmark script
#!/bin/sh
set -eu

# Exports for binaries access
export PATH="./node_modules/.bin:$PATH"

hyperfine --warmup 3 --runs 10 'oxlint' './node_modules/@oxlint/darwin-arm64/oxlint' --export-markdown "oxlint.md"

This script should improve startup performance of CLI by ~2-3 times and very huge impact for lint-staged, pre-commit and pre-push commits
@Boshen
Copy link
Member

Boshen commented Apr 8, 2024

How does the root package know which binary to call if the oxlint bin script is removed 🤔 ?

@dalisoft
Copy link
Author

dalisoft commented Apr 8, 2024

@Boshen it is will used from one of optional dependency

@dalisoft
Copy link
Author

dalisoft commented Apr 8, 2024

@Boshen oxlint itself blazingly fast but nodejs overhead caused it to be slow for startup.

Actually, biome, dprint and most of rust projects suffers from this bug/slowdown

see https://github.com/dalisoft/json-lint-benchmark?tab=readme-ov-file#pull-requests as reference to other rust projects slowness PR

@Boshen
Copy link
Member

Boshen commented Apr 8, 2024

This doesn't work. node refuses to run without a main or bin entry.

image

And I think you didn't test this locally by running the generate-packages.mjs script because it errors out.

@dalisoft
Copy link
Author

dalisoft commented Apr 8, 2024

@Boshen it should work after upload/publish and install into project. After build, try link and install on some project, then test

@dalisoft
Copy link
Author

dalisoft commented Apr 8, 2024

@Boshen i'm added my own repo to demonstrate the how fast is oxlint itself

https://github.com/dalisoft/oxlint-rs-npm/tree/master/benchmark

Command Mean [ms] Min [ms] Max [ms] Relative
oxlint 61.9 ± 8.4 57.6 84.4 14.12 ± 2.84
./node_modules/oxlint-rs-npm/oxlint 4.4 ± 0.7 3.5 5.5 1.00

@Boshen
Copy link
Member

Boshen commented Apr 8, 2024

We need the bin script in the oxlint package, otherwise npx oxlint@latest will not work.

For your benchmark, it's ok to call the binary via node_modules/.bin/oxlint.

@dalisoft
Copy link
Author

dalisoft commented Apr 8, 2024

I understood, thank you. So i'll close the this PR then

@dalisoft dalisoft closed this Apr 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants