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

Limit benchmark iterations using environment variable #10060

Merged
merged 7 commits into from
Jan 25, 2022
Merged

Limit benchmark iterations using environment variable #10060

merged 7 commits into from
Jan 25, 2022

Conversation

karthikeyann
Copy link
Contributor

To address part of #5773
This allows to run benchmarks for only specific iterations using environment variable CUDF_BENCHMARK_ITERATIONS.
except when benchmark definition itself specifies iteration count.

Also, makes pool as static to allocate pool memory resource only once per binary.

@karthikeyann karthikeyann added 3 - Ready for Review Ready for review by team tests Unit testing for project libcudf Affects libcudf (C++/CUDA) code. 4 - Needs Review Waiting for reviewer to review or respond Performance Performance related issue improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Jan 17, 2022
@karthikeyann karthikeyann requested a review from a team as a code owner January 17, 2022 15:06
@codecov
Copy link

codecov bot commented Jan 17, 2022 •
edited
Loading

Codecov Report

Merging #10060 (d156a49) into branch-22.04 (53a31d1) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@              Coverage Diff              @@
##           branch-22.04   #10060   +/-   ##
=============================================
  Coverage         10.42%   10.42%           
=============================================
  Files               119      119           
  Lines             20607    20607           
=============================================
  Hits               2148     2148           
  Misses            18459    18459

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 53a31d1...d156a49. Read the comment docs.

vuule
vuule reviewed Jan 18, 2022
View reviewed changes
Copy link
Contributor

@vuule vuule left a comment

Choose a reason for hiding this comment

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

small nitpick, looks good otherwise

cpp/benchmarks/fixture/benchmark_fixture.hpp Outdated Show resolved Hide resolved
@karthikeyann
Copy link
Contributor Author

Following test failed. (twice)
dask_cudf.tests.test_groupby.test_groupby_basic[True-std]

@karthikeyann karthikeyann requested a review from vuule January 18, 2022 09:13
@vuule
Copy link
Contributor

vuule commented Jan 18, 2022

Following test failed. (twice) dask_cudf.tests.test_groupby.test_groupby_basic[True-std]

IIRC, this test is flaky.

@vuule
Copy link
Contributor

vuule commented Jan 18, 2022

rerun tests

@davidwendt
Copy link
Contributor

Also, does this have to go into 22.02? Burn-down started last week.

jrhemstad
jrhemstad reviewed Jan 18, 2022
View reviewed changes
cpp/benchmarks/fixture/benchmark_fixture.hpp
@@ -68,6 +70,12 @@ inline auto make_pool()
*/
class benchmark : public ::benchmark::Fixture {
public:
benchmark() : ::benchmark::Fixture()
{
const char* env_iterations = std::getenv("CUDF_BENCHMARK_ITERATIONS");
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not keen on limiting the number of benchmark iterations for all benchmarks arbitrarily like this. GBench/NVbench run as many iterations as is needed to determine statistical significance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not for actual benchmarking results. This is just to enable benchmark binaries run as part of CI or nightly builds to ensure there is nothing broken in benchmarks.

@karthikeyann
Copy link
Contributor Author

rerun tests

@vyasr
Copy link
Contributor

vyasr commented Jan 20, 2022

Should this be retargeted to 22.04?

@karthikeyann
Copy link
Contributor Author

rerun tests

1 similar comment
@karthikeyann
Copy link
Contributor Author

rerun tests

@karthikeyann
Copy link
Contributor Author

Should this be retargeted to 22.04?

It does not matter if it goes to 22.02 or 22.04. It affects only benchmarks.

@karthikeyann
Copy link
Contributor Author

Moving to 22.04

@karthikeyann karthikeyann changed the base branch from branch-22.02 to branch-22.04 January 21, 2022 18:30
@karthikeyann
Copy link
Contributor Author

rerun tests

@karthikeyann
Copy link
Contributor Author

@gpucibot merge

@rapids-bot rapids-bot bot merged commit a552afb into rapidsai:branch-22.04 Jan 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidwendt davidwendt davidwendt left review comments

@jrhemstad jrhemstad jrhemstad left review comments

@codereport codereport codereport approved these changes

@vuule vuule vuule approved these changes

@ttnghia ttnghia ttnghia approved these changes

Assignees
No one assigned
Labels
3 - Ready for Review Ready for review by team 4 - Needs Review Waiting for reviewer to review or respond improvement Improvement / enhancement to an existing function libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change Performance Performance related issue tests Unit testing for project
Projects
None yet
Milestone
C++ Benchmark Runtime Improvements
Development

Successfully merging this pull request may close these issues.
7 participants
@karthikeyann @vuule @davidwendt @vyasr @ttnghia @jrhemstad @codereport