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(storage): make benchmark more robust on stalls #7113

Merged

Conversation

coryan
Copy link
Contributor

@coryan coryan commented Aug 4, 2021

Change the benchmark to use a single queue (LIFO) with the set of
objects to download, and have all the threads service this queue.
Previously we separated the objects in groups and assigned them to
threads. The previous approach wastes threads if one (or a small
fraction) of the downloads gets stalled. I think the new approach better
models a well-designed application.


This change is Reviewable

Change the benchmark to use a single queue (LIFO) with the set of
objects to download, and have all the threads service this queue.
Previously we separated the objects in groups and assigned them to
threads. The previous approach wastes threads if one (or a small
fraction) of the downloads gets stalled. I think the new approach better
models a well-designed application.
@product-auto-label product-auto-label bot added the api: storage Issues related to the Cloud Storage API. label Aug 4, 2021
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Aug 4, 2021
@google-cloud-cpp-bot
Copy link
Collaborator

Google Cloud Build Logs
For commit: 02f004a5096e4afb7cac582e5683b2eaaec63c15

ℹ️ NOTE: Kokoro logs are linked from "Details" below.

@codecov
Copy link

codecov bot commented Aug 4, 2021

Codecov Report

Merging #7113 (02f004a) into main (ae0fbe6) will increase coverage by 0.00%.
The diff coverage is 96.07%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #7113   +/-   ##
=======================================
  Coverage   94.49%   94.49%           
=======================================
  Files        1309     1309           
  Lines      113025   113035   +10     
=======================================
+ Hits       106801   106812   +11     
+ Misses       6224     6223    -1     
Impacted Files Coverage Δ
...orage/benchmarks/aggregate_throughput_benchmark.cc 88.55% <96.07%> (+0.59%) ⬆️
...bigtable/examples/bigtable_hello_instance_admin.cc 81.31% <0.00%> (-2.20%) ⬇️
google/cloud/pubsub/samples/samples.cc 91.67% <0.00%> (-0.08%) ⬇️
.../cloud/storage/benchmarks/throughput_experiment.cc 74.87% <0.00%> (+0.50%) ⬆️
...le/cloud/storage/internal/curl_download_request.cc 89.15% <0.00%> (+1.20%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ae0fbe6...02f004a. Read the comment docs.

@coryan coryan marked this pull request as ready for review August 4, 2021 01:09
@coryan coryan requested a review from a team as a code owner August 4, 2021 01:09
Copy link
Member

@dbolduc dbolduc left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, all discussions resolved (waiting on @coryan)


google/cloud/storage/benchmarks/aggregate_throughput_benchmark.cc, line 257 at r1 (raw file):

Quoted 4 lines of code…
    std::transform(configs.begin(), configs.end(), tasks.begin(),
                   [&task](TaskConfig const& c) {
                     return std::async(std::launch::async, task, std::cref(c));
                   });

if you did something like this, you can get rid of thetask lambda:

std::transform(configs.begin(), configs.end(), tasks.begin(),
               [&iteration](TaskConfig const& c) {
                 return std::async(std::launch::async, &Iteration::DownloadTask, iteration, std::cref(c));
               });

The aesthetic judgment is up to you. I don't have a preference.

Copy link
Contributor Author

@coryan coryan left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, all discussions resolved (waiting on @coryan)


google/cloud/storage/benchmarks/aggregate_throughput_benchmark.cc, line 257 at r1 (raw file):

Previously, dbolduc (Darren Bolduc) wrote…
    std::transform(configs.begin(), configs.end(), tasks.begin(),
                   [&task](TaskConfig const& c) {
                     return std::async(std::launch::async, task, std::cref(c));
                   });

if you did something like this, you can get rid of thetask lambda:

std::transform(configs.begin(), configs.end(), tasks.begin(),
               [&iteration](TaskConfig const& c) {
                 return std::async(std::launch::async, &Iteration::DownloadTask, iteration, std::cref(c));
               });

The aesthetic judgment is up to you. I don't have a preference.

I am going to leave as-is, and probably change my mind later

@coryan coryan merged commit 913eff7 into googleapis:main Aug 4, 2021
@coryan coryan deleted the feat-storage-make-benchmark-more-robust branch August 4, 2021 12:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: storage Issues related to the Cloud Storage API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants