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

Import bundled Insect in index.js #322

Merged
merged 1 commit into from
Jul 26, 2022

Conversation

triallax
Copy link
Contributor

@triallax triallax commented Jul 20, 2022

Fixes #310, and provides a start for #306.

This improves the startup time on my machine significantly. I'll let the benchmark results speak for themselves:

Before:

Command Mean [ms] Min [ms] Max [ms] Relative
Startup time (insect '1+1') 513.9 ± 24.4 496.4 581.2 1.00
Command Mean [ms] Min [ms] Max [ms] Relative
Startup time, stdin mode (insect < computations-1.ins) 535.5 ± 14.2 524.6 572.7 1.00
Command Mean [ms] Min [ms] Max [ms] Relative
Evaluation speed (insect < computations-160.ins) 1502.1 ± 42.4 1466.2 1580.7 1.00

After:

Command Mean [ms] Min [ms] Max [ms] Relative
Startup time (insect '1+1') 192.4 ± 18.5 185.4 259.1 1.00
Command Mean [ms] Min [ms] Max [ms] Relative
Startup time, stdin mode (insect < computations-1.ins) 208.1 ± 2.0 204.8 212.2 1.00
Command Mean [ms] Min [ms] Max [ms] Relative
Evaluation speed (insect < computations-160.ins) 1153.0 ± 17.6 1136.6 1198.3 1.00

Also, this PR causes output to not be published to NPM, which makes for a much smaller installation size: Insect 5.7.0 on NPM has an unpacked size of 29.2 MBs, but npm publish --dry-run with this PR reports 378.8 kB. .npmignore was also removed, as the files property in package.json includes only the files we want, so .npmignore does basically nothing. Read https://docs.npmjs.com/cli/v8/configuring-npm/package-json/#files for more information.

This PR additionally makes a small modification to index.js to make it work with Node 12. Unfortunately, at least one of our dependencies (clipboardy) does not support Node 10, so I could not make index.js work with Node 10.

@triallax triallax force-pushed the use-bundled-file-with-cli branch from e45b6a9 to 3944a20 Compare July 20, 2022 16:30
@sharkdp
Copy link
Owner

sharkdp commented Jul 20, 2022

This improves the startup time on my machine significantly. I'll let the benchmark results speak for themselves:

Fantastic 🥳 Thank you so much!

Also, this commit causes output to not be published to NPM, which makes for a much installation size: Insect 5.7.0 on NPM has an unpacked size of 29.2 MBs, but npm publish --dry-run with this commit reports 378.8 kB. .npmignore was also removed, as the files property in package.json includes only the files we want, so .npmignore does basically nothing. Read https://docs.npmjs.com/cli/v8/configuring-npm/package-json/#files for more information.

Nice!

I need to look this up somewhere (issue tracker, Git log) but I believe I remember that I tried this in the past (remove the output folder) and it caused some problems for people that wanted to install Insect without having to recompile everything from (PureScript) source?

@sharkdp
Copy link
Owner

sharkdp commented Jul 20, 2022

I need to look this up somewhere (issue tracker, Git log) but I believe I remember that I tried this in the past (remove the output folder) and it caused some problems for people that wanted to install Insect without having to recompile everything from (PureScript) source?

Oh, I think I understand now. With the changes proposed here, the output folder will not be necessary at all - of course.

@sharkdp
Copy link
Owner

sharkdp commented Jul 20, 2022

Maybe retarget the PR towards your spago branch? (if that's possible across forks)

@triallax
Copy link
Contributor Author

Fantastic 🥳 Thank you so much!

You're welcome. :)

Maybe retarget the PR towards your spago branch? (if that's possible across forks)

I could certainly do that, but then the PR would be merged into my spago branch. I think we can just wait till my Spago PR is merged, then merge this one when it's ready.

Fixes sharkdp#310, and provides a start for sharkdp#306.

This improves the startup time on my machine significantly. I'll let the
benchmark results speak for themselves:

Before commit:

| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|:---|---:|---:|---:|---:|
| `Startup time (insect '1+1')` | 513.9 ± 24.4 | 496.4 | 581.2 | 1.00 |

| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|:---|---:|---:|---:|---:|
| `Startup time, stdin mode (insect < computations-1.ins)` | 535.5 ± 14.2 | 524.6 | 572.7 | 1.00 |

| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|:---|---:|---:|---:|---:|
| `Evaluation speed (insect < computations-160.ins)` | 1502.1 ± 42.4 | 1466.2 | 1580.7 | 1.00 |

After commit:

| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|:---|---:|---:|---:|---:|
| `Startup time (insect '1+1')` | 192.4 ± 18.5 | 185.4 | 259.1 | 1.00 |

| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|:---|---:|---:|---:|---:|
| `Startup time, stdin mode (insect < computations-1.ins)` | 208.1 ± 2.0 | 204.8 | 212.2 | 1.00 |

| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|:---|---:|---:|---:|---:|
| `Evaluation speed (insect < computations-160.ins)` | 1153.0 ± 17.6 | 1136.6 | 1198.3 | 1.00 |

Also, this commit causes `output` to not be published to NPM, which
makes for a much smaller installation size: Insect 5.7.0 on NPM has an
unpacked size of 29.2 MBs, but `npm publish --dry-run` with this commit
reports 378.8 kB. `.npmignore` was also removed, as the `files` property
in `package.json` includes only the files we want, so `.npmignore` does
basically nothing. Read
https://docs.npmjs.com/cli/v8/configuring-npm/package-json/#files for
more information.

This commit additionally makes a small modification to `index.js` to
make it work with Node 12. Unfortunately, at least one of our
dependencies (`clipboardy`) does not support Node 10, so I could not
make `index.js` work with Node 10.
@triallax triallax force-pushed the use-bundled-file-with-cli branch from 3944a20 to 948e218 Compare July 24, 2022 20:36
@triallax triallax marked this pull request as ready for review July 24, 2022 20:36
Copy link
Owner

@sharkdp sharkdp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@sharkdp sharkdp merged commit 03c641f into sharkdp:master Jul 26, 2022
@triallax triallax deleted the use-bundled-file-with-cli branch July 26, 2022 12:36
@sharkdp sharkdp mentioned this pull request Aug 22, 2022
2 tasks
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.

Investigate and fix startup time increase
2 participants