Add peak memory usage tracking to cuIO benchmarks #7770

devavret · 2021-03-30T22:10:57Z

Uses rmm::mr::statistics_resource_adapter to track peak memory usage in cuIO benchmarks.

And sample use in parquet writer bench

Separate out memory_tracking_resource into its own header and remove association with benchmark fixture

cpp/benchmarks/common/memory_tracking_resource.hpp

Co-authored-by: David <[email protected]>

davidwendt

This is way cool.

cpp/benchmarks/common/memory_tracking_resource.hpp

vuule

Great work. Mostly got questions :)
(requesting changes just for the copyright years)

cpp/benchmarks/fixture/benchmark_fixture.hpp

cpp/benchmarks/io/parquet/parquet_writer_benchmark.cpp

vuule · 2021-03-31T01:02:06Z

cpp/benchmarks/io/parquet/parquet_writer_benchmark.cpp

+  rmm::mr::device_memory_resource* mr = rmm::mr::get_current_device_resource();
+  cudf::memory_tracking_resource<rmm::mr::device_memory_resource> tracking_mr(mr);


we need to make changes to all benchmarks. Do you want to add this in a separate PR?

No I just wanted to get reviews on the method first, before going ahead and changing all the benchmarks. In the first commit, I made the tracking resource part of the base fixture and that automatically enabled it for every benchmark. But a problem with that was that it tracked memory usage for the setup as well.

Added in 4664f9a

vuule · 2021-03-31T01:04:41Z

cpp/benchmarks/io/parquet/parquet_writer_benchmark.cpp


  state.SetBytesProcessed(data_size * state.iterations());
+  state.counters["peak_memory_usage"] = tracking_mr.max_allocated_size();


Could we use this to catch memory leaks in tests (just query current instead of max on exit)?
Edit: leaks are pretty unlikely given libcudf's coding style, we probably don't need to invest time into this check.

It would be pretty easy. But since this is more of a test (curr value should always be 0 at the end) rather than a benchmark, should we make this opt-in? If anyone suspects a leak, they can add a check.

I was thinking of adding this check to the unit tests, rather than benchmarks.

Well sure. I can do that in a subsequent PR.

I just realized that any leak we have would probably be due to memory allocated without the rmm resource. Anything with rmm is likely using some RAII object to hold the memory. And this doesn't track non-rmm allocated memory.

vuule · 2021-03-31T01:11:50Z

cpp/benchmarks/io/parquet/parquet_writer_benchmark.cpp


  state.SetBytesProcessed(data_size * state.iterations());
+  state.counters["peak_memory_usage"] = tracking_mr.max_allocated_size();


Are you getting consistent numbers between runs? @kaatish mentioned that the peak usage varies when running the ORC writer benchmark.

I just tested, it reports the same number for corresponding benchmarks across runs for ORC. @kaatish, what method did you use to test peak memory usage? If you're using nsys, you need to turn off the pool allocator.

I used the script referenced in issue #7661 which calls pynvml methods.

into memory-tracking-bench

codecov · 2021-03-31T11:00:58Z

Codecov Report

❗ No coverage uploaded for pull request base (branch-21.08@3ed87f3). Click here to learn what that means.
The diff coverage is n/a.

@@               Coverage Diff               @@
##             branch-21.08    #7770   +/-   ##
===============================================
  Coverage                ?   10.53%           
===============================================
  Files                   ?      116           
  Lines                   ?    18916           
  Branches                ?        0           
===============================================
  Hits                    ?     1993           
  Misses                  ?    16923           
  Partials                ?        0

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 3ed87f3...baa4150. Read the comment docs.

davidwendt · 2021-03-31T13:20:47Z

Should this be going into 0.19. I thought we were in burndown.

harrism · 2021-07-06T05:34:07Z

Why not make this apply to all benchmarks by putting it in the base fixture?

devavret · 2021-07-06T09:29:55Z

Why not make this apply to all benchmarks by putting it in the base fixture?

I did that previously in this PR c34b32b but changed it to per benchmark in b9147f4 because adding it to base fixture would cause allocations during the setup stage to also be included. We only care about the allocations happening in the API being benchmarked.

harrism · 2021-07-07T02:07:54Z

What allocations are there in the setup stage? Can't those be moved before where you create the statistics tracking resource and set it as the default?

devavret · 2021-07-07T15:51:25Z

What allocations are there in the setup stage? Can't those be moved before where you create the statistics tracking resource and set it as the default?

Setup happens in most benchmarks' primary function. e.g. BM_parq_read_varying_input where a table is created to write and then a call to write_parquet happens before read_parquet is profiled in the for (auto _ : state) loop. This entire function is called after cudf::benchmark::SetUp. We want to activate the statistics_resource right before the for (auto _ : state) loop

harrism · 2021-07-08T00:47:10Z

I see. Perhaps we can provide another function in the fixture that all benchmarks can use to initiate statistics logging...

devavret · 2021-07-09T20:10:00Z

I see. Perhaps we can provide another function in the fixture that all benchmarks can use to initiate statistics logging...

How about an RAII object that does this 8d85fa9.

harrism · 2021-07-13T00:00:37Z

I like it. Easy to add to other benchmarks.

devavret · 2021-07-16T19:33:07Z

rerun tests

vuule

Looks good, just got an optional nitpick.

cpp/benchmarks/fixture/benchmark_fixture.hpp

Co-authored-by: Vukasin Milovanovic <[email protected]>

devavret · 2021-07-19T08:51:00Z

@gpucibot merge

devavret added 2 commits March 31, 2021 00:51

Initial memory tracking resource

c34b32b

And sample use in parquet writer bench

Localize the memory usage calculator to just the API being benchmarked

b9147f4

Separate out memory_tracking_resource into its own header and remove association with benchmark fixture

devavret requested a review from a team as a code owner March 30, 2021 22:10

devavret requested review from karthikeyann and davidwendt March 30, 2021 22:10

github-actions bot added the libcudf Affects libcudf (C++/CUDA) code. label Mar 30, 2021

devavret requested a review from vuule March 30, 2021 22:11

devavret added 3 - Ready for Review Ready for review by team cuIO cuIO issue non-breaking Non-breaking change improvement Improvement / enhancement to an existing function labels Mar 30, 2021

davidwendt reviewed Mar 30, 2021

View reviewed changes

cpp/benchmarks/common/memory_tracking_resource.hpp Outdated Show resolved Hide resolved

Update cpp/benchmarks/common/memory_tracking_resource.hpp

c49d60b

Co-authored-by: David <[email protected]>

davidwendt approved these changes Mar 31, 2021

View reviewed changes

cpp/benchmarks/common/memory_tracking_resource.hpp Outdated Show resolved Hide resolved

cpp/benchmarks/common/memory_tracking_resource.hpp Outdated Show resolved Hide resolved

vuule requested changes Mar 31, 2021

View reviewed changes

devavret requested a review from harrism March 31, 2021 08:25

devavret added 3 commits March 31, 2021 14:37

Expand mem tracking to all cuIO benchmarks

4664f9a

Merge branch 'memory-tracking-bench' of https://github.com/devavret/cudf

d10265c

into memory-tracking-bench

Update docs

5c90e76

devavret changed the base branch from branch-0.19 to branch-0.20 March 31, 2021 13:22

devavret requested review from a team as code owners March 31, 2021 13:22

devavret requested review from cwharris and isVoid and removed request for cwharris March 31, 2021 13:22

github-actions bot added conda Java Affects Java cuDF API. Python Affects Python cuDF API. labels Jul 1, 2021

Move to new statistics resource

30a921c

devavret changed the base branch from branch-21.06 to branch-21.08 July 1, 2021 18:57

devavret requested a review from harrism July 1, 2021 19:03

Proposed RAII statistics resource wrapper

8d85fa9

github-actions bot removed CMake CMake build issue Python Affects Python cuDF API. gpuCI Java Affects Java cuDF API. labels Jul 9, 2021

Change all io benchmarks to use RAII stats logger

804cb80

harrism approved these changes Jul 14, 2021

View reviewed changes

devavret added 2 commits July 15, 2021 17:11

Merge branch 'branch-21.08' into memory-tracking-bench

71c5d32

Remove extra header

4662f6e

vuule approved these changes Jul 16, 2021

View reviewed changes

cpp/benchmarks/fixture/benchmark_fixture.hpp Outdated Show resolved Hide resolved

Update cpp/benchmarks/fixture/benchmark_fixture.hpp

baa4150

Co-authored-by: Vukasin Milovanovic <[email protected]>

rapids-bot bot merged commit fdf4901 into rapidsai:branch-21.08 Jul 19, 2021

vyasr mentioned this pull request Mar 28, 2022

Add peak_memory_usage to all nvbench benchmarks #10528

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add peak memory usage tracking to cuIO benchmarks #7770

Add peak memory usage tracking to cuIO benchmarks #7770

devavret commented Mar 30, 2021 •

edited

Loading

davidwendt left a comment

vuule left a comment

vuule Mar 31, 2021

devavret Mar 31, 2021

devavret Mar 31, 2021

vuule Mar 31, 2021

devavret Mar 31, 2021

vuule Mar 31, 2021

devavret Mar 31, 2021

devavret Mar 31, 2021

vuule Mar 31, 2021

devavret Mar 31, 2021

kaatish Mar 31, 2021

codecov bot commented Mar 31, 2021 •

edited

Loading

davidwendt commented Mar 31, 2021

harrism commented Jul 6, 2021

devavret commented Jul 6, 2021

harrism commented Jul 7, 2021

devavret commented Jul 7, 2021

harrism commented Jul 8, 2021

devavret commented Jul 9, 2021

harrism commented Jul 13, 2021

devavret commented Jul 16, 2021

vuule left a comment

devavret commented Jul 19, 2021

		rmm::mr::device_memory_resource* mr = rmm::mr::get_current_device_resource();
		cudf::memory_tracking_resource<rmm::mr::device_memory_resource> tracking_mr(mr);


		state.SetBytesProcessed(data_size * state.iterations());
		state.counters["peak_memory_usage"] = tracking_mr.max_allocated_size();

Add peak memory usage tracking to cuIO benchmarks #7770

Add peak memory usage tracking to cuIO benchmarks #7770

Conversation

devavret commented Mar 30, 2021 • edited Loading

davidwendt left a comment

Choose a reason for hiding this comment

vuule left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Mar 31, 2021 • edited Loading

Codecov Report

davidwendt commented Mar 31, 2021

harrism commented Jul 6, 2021

devavret commented Jul 6, 2021

harrism commented Jul 7, 2021

devavret commented Jul 7, 2021

harrism commented Jul 8, 2021

devavret commented Jul 9, 2021

harrism commented Jul 13, 2021

devavret commented Jul 16, 2021

vuule left a comment

Choose a reason for hiding this comment

devavret commented Jul 19, 2021

devavret commented Mar 30, 2021 •

edited

Loading

codecov bot commented Mar 31, 2021 •

edited

Loading