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

Configurable thread pool size #79

Merged
merged 1 commit into from
Oct 19, 2017

Conversation

gsquire
Copy link
Contributor

@gsquire gsquire commented Oct 19, 2017

Fixes #27

I'm not sure if this is desired so if it isn't, feel free to close it.

@ashleygwilliams
Copy link
Collaborator

hey @gsquire ! thanks so much for this. this makes the default value way more intelligent. it turns out that the original spirit of my extremely not detailed issue ( #27 ) was that we would also accept an env variable that would designate the number.

if you are interested in adding that to this PR we would love it! otherwise we'll accept this and do the env variable work in another PR. just lemme know! thanks for the contribution!

Copy link
Collaborator

@ashleygwilliams ashleygwilliams left a comment

Choose a reason for hiding this comment

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

this looks good, but there's another small feature we may wait on if the @gsquire is interested, so holding off merging til they respond :)

@gsquire
Copy link
Contributor Author

gsquire commented Oct 19, 2017

I can certainly add that. Here is how I envision the flow:

If the environment variable is present (let me know what name sounds good), try and parse it as a u32. If this fails or the variable isn't present, then I will default to the number of CPUs. How does that sound?

@ashleygwilliams
Copy link
Collaborator

hey @gsquire ! that sounds absolutely perfect. thank you so much!!

@ashleygwilliams
Copy link
Collaborator

i am not picky about the name, maybe NUM_POOLS or just POOLS, open to whatever you think is best :)

@ashleygwilliams
Copy link
Collaborator

cc i dont know if @steveklabnik has a pref

@gsquire
Copy link
Contributor Author

gsquire commented Oct 19, 2017

I can update that soon and then change it if need be based on his response :)

@steveklabnik
Copy link
Owner

Rust's testing framework uses RUST_TEST_THREADS to set the number of threads, what about SIMPLESERVER_THREADS?

Env vars don't have namespacing, so you kind of have to do it in the name...

@gsquire gsquire force-pushed the configurable_cores branch from c8255a6 to e4f0269 Compare October 19, 2017 17:09
@gsquire
Copy link
Contributor Author

gsquire commented Oct 19, 2017

Ok I adjusted it to SIMPLESERVER_THREADS. Let me know if you would like me to squash my commits.

@ashleygwilliams
Copy link
Collaborator

this looks great @gsquire ! if you wanna fix up that last commit into the main commit you've got i think we can ship this! thank u so much for your patience, flexibility, and work ✨ ❤️

@gsquire gsquire force-pushed the configurable_cores branch from d6e8c24 to 5955b73 Compare October 19, 2017 17:13
@gsquire
Copy link
Contributor Author

gsquire commented Oct 19, 2017

No problem, happy to contribute. Should be all set now.

@gsquire gsquire force-pushed the configurable_cores branch from 5955b73 to 09b0dcf Compare October 19, 2017 17:16
pool size while falling back to num_cpus if it is not present
@gsquire gsquire force-pushed the configurable_cores branch from 09b0dcf to 41071a9 Compare October 19, 2017 17:22
@ashleygwilliams ashleygwilliams merged commit 5b7b701 into steveklabnik:master Oct 19, 2017
@gsquire gsquire deleted the configurable_cores branch October 19, 2017 17:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

make number of Pools configurable
3 participants