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

Add bytes_per_second to transpose benchmark #14170

Merged
merged 15 commits into from
Oct 10, 2023
Merged

Add bytes_per_second to transpose benchmark #14170

Changes from 5 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
66f43e4
Add `bytes_per_second` to transpose benchmark
Blonck Sep 22, 2023
d2676b5
Run clang-format on transpose benchmark
Blonck Sep 22, 2023
f594a81
Refactor column type in transpose benchmark
Blonck Sep 27, 2023
dcd9ef1
Merge branch 'branch-23.12' into processed_bytes_transpose_bench
Blonck Sep 27, 2023
60845b7
Fix transpose benchmark type issue
Blonck Sep 27, 2023
05e1afa
Merge branch 'branch-23.12' into processed_bytes_transpose_bench
Blonck Sep 28, 2023
e9280cc
Change way column type is handled in transpose bench
Blonck Sep 28, 2023
4187495
Merge branch 'branch-23.12' into processed_bytes_transpose_bench
Blonck Sep 30, 2023
c50a5ef
Fix code style in transpose benchmark
Blonck Sep 30, 2023
8083756
Avoid potential integer overflow in transpose benchmark.
Blonck Oct 1, 2023
e6d9f24
Update cpp/benchmarks/transpose/transpose.cpp
Blonck Oct 2, 2023
82cbcf9
Fix code style in transpose benchmark
Blonck Oct 3, 2023
ed8aa1e
Merge branch 'branch-23.12' into processed_bytes_transpose_bench
harrism Oct 3, 2023
943b9f3
Merge branch 'branch-23.12' into processed_bytes_transpose_bench
Blonck Oct 4, 2023
09f10d8
Merge branch 'branch-23.12' into processed_bytes_transpose_bench
harrism Oct 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 22 additions & 7 deletions cpp/benchmarks/transpose/transpose.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,17 +20,19 @@
#include <cudf/column/column_factories.hpp>
#include <cudf/table/table.hpp>
#include <cudf/transpose.hpp>
#include <cudf/types.hpp>

#include <thrust/iterator/counting_iterator.h>
#include <thrust/iterator/transform_iterator.h>

static void BM_transpose(benchmark::State& state)
{
auto count = state.range(0);
constexpr auto column_type = cudf::data_type{cudf::type_id::INT32};
auto int_column_generator =
thrust::make_transform_iterator(thrust::counting_iterator(0), [count](int i) {
return cudf::make_numeric_column(
cudf::data_type{cudf::type_id::INT32}, count, cudf::mask_state::ALL_VALID);
column_type, count, cudf::mask_state::ALL_VALID);
});

auto input_table = cudf::table(std::vector(int_column_generator, int_column_generator + count));
Expand All @@ -40,16 +42,29 @@ static void BM_transpose(benchmark::State& state)
cuda_event_timer raii(state, true);
auto output = cudf::transpose(input);
}

// Collect memory statistics.
auto const bytes_read = input.num_columns() * input.num_rows() * cudf::size_of(column_type);
Copy link
Member

Choose a reason for hiding this comment

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

💡 suggestion: ‏ This is one way to do it. But I think the way I suggested is a bit better because it all happens at compile time, whereas cudf::size_of() invokes the type dispatcher at run time. Not that it will affect benchmarks, but it just seems cleaner to use sizeof(cudf::id_to_type<column_type>).

auto const bytes_written = bytes_read;
// Account for nullability in input and output.
auto const null_bytes =
2 * input.num_columns() * cudf::bitmask_allocation_size_bytes(input.num_rows());
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: ‏This one could also overflow, I think, perhaps:

Suggested change
2 * input.num_columns() * cudf::bitmask_allocation_size_bytes(input.num_rows());
2 * static_cast<uint64_t>(input.num_columns()) * cudf::bitmask_allocation_size_bytes(input.num_rows());

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you sure about this one? Since the return type of cudf::bitmask_allocation_size_bytes is std::size_t which is either unsigned long or unsigned long long so for reasonable input sizes the integer type promotion will avoid the overflow (https://cppinsights.io/s/26f977cb).

That said, just having this discussion indicates I should've included an explicit cast upfront to clear up any potential confusion.

Copy link
Contributor

Choose a reason for hiding this comment

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

Left-to-right associativity means that this is evaluated as (2 * ncol) * nrow, the first multiplication is performed in size_type (AKA, int32_t), so that could overflow, no? Although I think these benchmarks are generally run with fewer than $2^{30}$ rows, there's in general no reason why they couldn't be (although the transpose performance will be terrible I grant you).

Copy link
Member

Choose a reason for hiding this comment

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

Why not just always put the thing that returns size_t (sizeof or bitmask_allocation_size_bytes) first in the arithmetic in all of these PRs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Personally, I would keep the cast explicit to make visible what is happening, but I don't have a strong stance on it.


state.SetBytesProcessed(static_cast<int64_t>(state.iterations()) *
(bytes_read + bytes_written + null_bytes));
}

class Transpose : public cudf::benchmark {};

#define TRANSPOSE_BM_BENCHMARK_DEFINE(name) \
BENCHMARK_DEFINE_F(Transpose, name)(::benchmark::State & state) { BM_transpose(state); } \
BENCHMARK_REGISTER_F(Transpose, name) \
->RangeMultiplier(4) \
->Range(4, 4 << 13) \
->UseManualTime() \
#define TRANSPOSE_BM_BENCHMARK_DEFINE(name) \
BENCHMARK_DEFINE_F(Transpose, name)(::benchmark::State & state) \
{ \
BM_transpose(state); \
} \
BENCHMARK_REGISTER_F(Transpose, name) \
->RangeMultiplier(4) \
->Range(4, 4 << 13) \
->UseManualTime() \
->Unit(benchmark::kMillisecond);

TRANSPOSE_BM_BENCHMARK_DEFINE(transpose_simple);