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

Reduce install/bundle size #175

Closed
Aslemammad opened this issue Dec 19, 2021 · 10 comments
Closed

Reduce install/bundle size #175

Aslemammad opened this issue Dec 19, 2021 · 10 comments
Labels
enhancement New feature or request stale

Comments

@Aslemammad
Copy link

Hello folks, I'm the tinypool (the 25kb fork of piscina) creator, and we've been talking about how to reduce the piscina's install size on Twitter. I hope my findings benefit you!

Bundling & Minifying

In tinypool, I don't bundle any dependency since there's no dependency for tinypool, but it seems that we can remove some additional files (of piscina's dependencies by bundling them).
You can see those useless files included in the install size here.

https://unpkg.com/browse/eventemitter-asyncresource@1.0.0/ (src, test)
https://unpkg.com/browse/piscina@3.2.0/ (benchmark, src, test)
https://unpkg.com/browse/hdr-histogram-percentiles-obj@3.0.0/ (test)
https://unpkg.com/browse/hdr-histogram-js@2.0.1/ (test, src, benchmark, ...)

Removing dependencies

In tinypool, we remove eventemitter-asyncresource (copying its minimal content & dropping other files) & hdr-histogram since we didn't need the last one. Still, I guess piscina can't do that; utilization is one of the good features of piscina (in @vitest-dev we didn't need it).
But as it was mentioned in Twitter, node 14 already afford that for us, so I guess by dropping node 12, we can reduce a significant portion of the install size!

https://twitter.com/matteocollina/status/1471828739537391623
https://twitter.com/jasnell/status/1471831611108839425

Thanks to the Piscina team for reaching out & giving us the opportunity + to those who helped me maintain tinypool, @TrySound @vitest-dev

@jasnell
Copy link
Collaborator

jasnell commented Dec 19, 2021

It's entirely likely that we can drop the dependency on hdr-histogram in a new major release and depend directly on the createHistogram() function in Node.js. There are some API differences there, however, so I'll need to investigate. If it does look like that's possible, however, that will be a significant reduction.

I'd also like to see if we can get eventemitter-asyncresource added to Node.js as an option. It's too much of a performance hit for us to make all EventEmitter instances work with async resource tracking but having the option built in to core makes the most sense long term.

@Aslemammad
Copy link
Author

Yea, dropping the hdr-histogram seems reasonable!
I am waiting for the significant reduction.

And if you need help in the process, I'm happy to help! Thanks for the consideration!

@jasnell
Copy link
Collaborator

jasnell commented Dec 23, 2021

It'll take a while before we can rely on it being available in all supported versions, but nodejs/node#41246 merges one of the dependencies here into node.js core.

@Aslemammad
Copy link
Author

That's a good change! Thanks for that!
tinypool uses it internally, so it doesn't get that much of dependencies!

@Prinzhorn
Copy link
Contributor

With Node.js 12 being EOL, we can drop hdr-histogram-js. Is that correct?

@metcoder95 metcoder95 added the enhancement New feature or request label Jun 12, 2023
Copy link

github-actions bot commented Jun 2, 2024

This issue has been marked as stale because it has been opened 30 days without activity. Remove stale label or comment or this will be closed in 5 days.

@github-actions github-actions bot added the stale label Jun 2, 2024
@Prinzhorn
Copy link
Contributor

Not stale, I guess a lot has happened in the last years and this could now be reasonably achieved?

@metcoder95
Copy link
Member

Piscina became dependency-less since then I believe this can be closed if the goal was in that way.

@Aslemammad what are your thoughts?

@Prinzhorn
Copy link
Contributor

Nice, looks like this happened semi recently:

aa5b140

@Aslemammad
Copy link
Author

Thank you so much y'all for the effort, let's close this then!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request stale
Projects
None yet
Development

No branches or pull requests

4 participants