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

PosixFS no longer has its own threadpool #7685

Merged
merged 6 commits into from May 9, 2019

Conversation

Projects
None yet
2 participants
@illicitonion
Copy link
Contributor

commented May 8, 2019

It uses the tokio runtime its futures are running on.

Commits are separately reviewable.

A follow-up PR will remove the fs::store::local::ByteStore use of the threadpool.

@illicitonion illicitonion requested review from stuhood and blorente May 8, 2019

PosixFS no longer has its own threadpool
It uses the tokio runtime its futures are running on.

@illicitonion illicitonion force-pushed the twitter:dwagnerhall/async-posixfs branch from 091c8a5 to 964e540 May 8, 2019

@stuhood

stuhood approved these changes May 8, 2019

Copy link
Member

left a comment

Too cool! Thanks a lot.

How does the performance look?

.unwrap(),
fn expand_all_sorted(
posix_fs: Arc<PosixFS>,
runtime: &mut tokio::runtime::Runtime,

This comment has been minimized.

Copy link
@stuhood

stuhood May 8, 2019

Member

Thoughts on this being explicit vs being thread local? How do you think that we should think about that boundary?

I'm fine either way: is easy to tweak later.

This comment has been minimized.

Copy link
@illicitonion

illicitonion May 8, 2019

Author Contributor

It turns out the blocking stuff can't happen on a current_thread runtime, so we need an explicit non-current_thread runtime here. We could make a method-local one rather than passing one in, but I suspect that would just be a little harder to follow :)

This comment has been minimized.

Copy link
@stuhood

stuhood May 8, 2019

Member

...oh! Have we actually made all usage of the Runtime explicit in the other cases as well? That's pretty awesome... I guess I missed the uppercase-static vs lowercase-local distinction. #shipit

This comment has been minimized.

Copy link
@illicitonion

illicitonion May 8, 2019

Author Contributor

We'll find out if any tests start failing!

@illicitonion

This comment has been minimized.

Copy link
Contributor Author

commented May 8, 2019

How does the performance look?

About a 4% speedup for a large list :: based off of 1.15 :)

@stuhood stuhood merged commit b4bd873 into pantsbuild:master May 9, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@stuhood stuhood deleted the twitter:dwagnerhall/async-posixfs branch May 9, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.