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(async): add extra async versions of public APIs #6

Merged
merged 1 commit into from
Oct 14, 2019
Merged

Conversation

zkat
Copy link
Owner

@zkat zkat commented Sep 1, 2019

(this PR is still a WIP - a few APIs still need porting)

This adds full async support to all relevant public cacache APIs, which should allow for concurrently handling a bunch of files.

I'm a bit concerned about the performance, but this might just be the overhead from task::block_on. I'm not really sure if it would literally double the time spent, though. That seems like a lot. /cc @stjepang @yoshuawuyts:

     Running target/release/deps/benchmarks-88cb7943d1ff2a9e                    
Benchmarking read_hash: Collecting 100 samples in estimated 5.0106 s (348k itera
                                                                                
read_hash               time:   [14.112 us 14.213 us 14.324 us]                 
Found 2 outliers among 100 measurements (2.00%)                                 
  2 (2.00%) high mild                                                           
                                                                                
Benchmarking async_read_hash: Collecting 100 samples in estimated 5.1412 s (146k

async_read_hash         time:   [35.696 us 35.971 us 36.231 us]
Found 5 outliers among 100 measurements (5.00%)
  2 (2.00%) high mild
  3 (3.00%) high severe

@zkat
Copy link
Owner Author

zkat commented Sep 1, 2019

Update: this is ready to merge just as soon as async-std merges async-rs/async-std#108

@yoshuawuyts
Copy link

yoshuawuyts commented Sep 1, 2019

@zkat heh, so if this is entirely bottlenecked by fs access then 2x perf drop sounds about what we've measured in the past.

The reason for this is that in order for a blocking call to be wrapped in a future, an extra memcpy needs to happen. Which leads to about a 2x drop in benchmarks that we've observed. We have an idea on how we could get rid of this, but it's not implemented yet.

edit: it's probably worth pointing out that imo using async APIs is probably still worth it because we'll probably be able to figure out how to remove the perf drop, and likely eventually also include faster fs access through iocp for windows and io_uring for linux without the need to change the APIs.

@zkat
Copy link
Owner Author

zkat commented Oct 14, 2019

No streaming support for now, because that'll be more tricky -- I want to make sure I get this PR in first. Streams will definitely be a big deal to add, and will more than justify the async perf hit because they'll be able to handle much bigger files.

@zkat zkat merged commit 18190bf into latest Oct 14, 2019
@zkat zkat deleted the zkat/async branch October 14, 2019 00:06
@yoshuawuyts
Copy link

@zkat wooooot! 🎉 🎉

@zkat
Copy link
Owner Author

zkat commented Oct 14, 2019

@yoshuawuyts also of interest might be that there was a significant boost in performance of the async version. It went from 34us to 24us on the same benchmarks. IDK what caused that change in perf.

@yoshuawuyts
Copy link

@zkat yay that's great to hear, thanks for sharing! We recently added a workstealing executor algorithm to async-std, which may explain some of the perf improvements you're seeing!

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