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.

Sign up for GitHub

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

Jump to bottom

rt: Change max_threads to max_blocking_threads #3287

Merged
merged 3 commits into from Dec 19, 2020
Merged

rt: Change max_threads to max_blocking_threads #3287

merged 3 commits into from Dec 19, 2020

Conversation

LucioFranco
Copy link
Member

Closes #2802

@LucioFranco LucioFranco added the M-runtime Module: tokio/runtime label Dec 17, 2020
@LucioFranco LucioFranco added this to the v1.0 milestone Dec 17, 2020
@Darksonn Darksonn added the A-tokio Area: The main tokio crate label Dec 17, 2020
Darksonn
Darksonn reviewed Dec 17, 2020
View reviewed changes
tokio/src/runtime/builder.rs Outdated Show resolved Hide resolved
tokio/src/runtime/builder.rs Outdated
Comment on lines 504 to 498
let blocking_pool = blocking::create_blocking_pool(self, self.max_threads);
let blocking_pool = blocking::create_blocking_pool(self, self.max_blocking_threads);
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like we need an additional change to the blocking pool. Maybe you need to add the number of worker threads to this one, or change the blocking pool itself?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't see anywhere where we do worker_threads - max_threads to get the cap. Maybe @carllerche can chime in here?

Copy link
Member

Choose a reason for hiding this comment

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

This should be worker_threads + max_blocking_threads I think.

Copy link
Member

Choose a reason for hiding this comment

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

Eventually, we will create a special path for worker calls to spawn_blocking, but we don't have that yet.

tokio/src/runtime/builder.rs Outdated Show resolved Hide resolved
tokio/tests/rt_threaded.rs Outdated Show resolved Hide resolved
@LucioFranco
Copy link
Member Author

@carllerche I made the change you requested.

Darksonn
Darksonn approved these changes Dec 18, 2020
View reviewed changes
Copy link
Contributor

@Darksonn Darksonn 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 for now. We can look into an option to disable spawn_blocking in a follow-up.

@dekellum dekellum mentioned this pull request Dec 18, 2020
Closed
@carllerche carllerche merged commit b99b00e into master Dec 19, 2020
@carllerche carllerche deleted the lucio/update-rt-builder branch December 19, 2020 16:04
@dekellum dekellum mentioned this pull request Jan 3, 2021
Closed
@kornelski kornelski mentioned this pull request Feb 14, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dekellum dekellum dekellum left review comments

@Darksonn Darksonn Darksonn approved these changes

@carllerche carllerche Awaiting requested review from carllerche

Assignees
No one assigned
Labels
A-tokio Area: The main tokio crate M-runtime Module: tokio/runtime
Projects
None yet
Milestone
v1.0
Development

Successfully merging this pull request may close these issues.

Change max_threads to max_blocking_threads
4 participants
@LucioFranco @carllerche @dekellum @Darksonn