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

Feat/dynamic pools #237

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

haggholm
Copy link
Contributor

Dynamically sized pools: If an idle interval is specified, periodically kill idle worker threads (optionally, specify a minimum size below which the pool size may not shrink)

@andywer
Copy link
Owner

andywer commented Apr 21, 2020

Interesting feature. Thanks for sharing, @haggholm!

It's a pretty advanced feature that would now increase the already complex pool implementation quite a bit. May I suggest a slightly different implementation approach?

It would be great to implement the auto-resizing as an additional "layer" on top of the generic pool implementation. How about we only add a resize() to the generic pool and then have a separate AutoResizingPool or so that uses the generic pool and only contains the resizing behavior logic.

Additional benefit: The auto-resizing code can then be tree-shaken away if not used (which will be the case for most pool users). You could also turn the auto-resizing pool into a separate package then if you want to.

@haggholm
Copy link
Contributor Author

It would be great to implement the auto-resizing as an additional "layer" on top of the generic pool implementation. How about we only add a resize() to the generic pool and then have a separate AutoResizingPool or so that uses the generic pool and only contains the resizing behavior logic.

I’m pretty sure that’s in general a very good idea. This week/month in particular, well—I added this patch to your package because unexpected corner cases in a more generic pool package, combined with threads for basic workers, bit me in the ass (in this case generic-pool and the surprising fact that borrowed + available !== size!). So in part, what I was trying to achieve here was dynamic pool size without trying to be so generic as to be potentially surprising, because while being generic has tremendous benefits, premature generic-ness is almost as bad as Dijsktra told us premature optimization is…

Well, I’m not trying to be disagreeable; I love this package, I'm very happy with how receptive you are to contributions, and your general point is entirely sound; but (from my POV) it needs to not be desperately/excessively generic, and I’d like to keep a nice, simple API like this, so I may not have the resources to refactor immediately.

@andywer
Copy link
Owner

andywer commented Aug 10, 2020

Hey @haggholm! Any chance of an overhaul of the PR in the near future?

Otherwise I will close it for now.

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.

2 participants