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

Update NVBench fixture to use new hooks, fix pinned memory segfault. #15492

Merged
merged 7 commits into from
Apr 23, 2024
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
15 changes: 12 additions & 3 deletions cpp/benchmarks/fixture/nvbench_fixture.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,8 @@ static std::string cuio_host_mem_param{
* Initializes the default memory resource to use the RMM pool device resource.
*/
struct nvbench_base_fixture {
using host_pooled_mr_t = rmm::mr::pool_memory_resource<rmm::mr::pinned_host_memory_resource>;

inline auto make_cuda() { return std::make_shared<rmm::mr::cuda_memory_resource>(); }

inline auto make_pool()
Expand Down Expand Up @@ -90,12 +92,12 @@ struct nvbench_base_fixture {

inline rmm::host_async_resource_ref make_cuio_host_pinned_pool()
{
using host_pooled_mr = rmm::mr::pool_memory_resource<rmm::mr::pinned_host_memory_resource>;
static std::shared_ptr<host_pooled_mr> mr = std::make_shared<host_pooled_mr>(
// Don't store in static, as the CUDA context may be destroyed before static destruction
this->host_pooled_mr = std::make_shared<host_pooled_mr_t>(
alliepiper marked this conversation as resolved.
Show resolved Hide resolved
std::make_shared<rmm::mr::pinned_host_memory_resource>().get(),
size_t{1} * 1024 * 1024 * 1024);

return *mr;
return *this->host_pooled_mr;
}

inline rmm::host_async_resource_ref create_cuio_host_memory_resource(std::string const& mode)
Expand Down Expand Up @@ -126,9 +128,16 @@ struct nvbench_base_fixture {
std::cout << "CUIO host memory resource = " << cuio_host_mode << "\n";
}

~nvbench_base_fixture()
{
// Ensure the the pool is freed before the CUDA context is destroyed:
cudf::io::set_host_memory_resource(this->make_cuio_host_pinned());
}

std::shared_ptr<rmm::mr::device_memory_resource> mr;
std::string rmm_mode{"pool"};

std::shared_ptr<host_pooled_mr_t> host_pooled_mr;
std::string cuio_host_mode{"pinned"};
};

Expand Down
45 changes: 29 additions & 16 deletions cpp/benchmarks/fixture/nvbench_main.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -15,29 +15,42 @@
*/

#include <benchmarks/fixture/nvbench_fixture.hpp>
#define NVBENCH_ENVIRONMENT cudf::nvbench_base_fixture

#include <nvbench/main.cuh>

#include <string>
#include <vector>

// strip off the rmm_mode and cuio_host_mem parameters before passing the
// remaining arguments to nvbench::option_parser
#undef NVBENCH_MAIN_PARSE
#define NVBENCH_MAIN_PARSE(argc, argv) \
nvbench::option_parser parser; \
std::vector<std::string> m_args; \
for (int i = 0; i < argc; ++i) { \
std::string arg = argv[i]; \
if (arg == cudf::detail::rmm_mode_param) { \
i += 2; \
} else if (arg == cudf::detail::cuio_host_mem_param) { \
i += 2; \
} else { \
m_args.push_back(arg); \
} \
} \
parser.parse(m_args)
void arg_handler(std::vector<std::string>& args)
alliepiper marked this conversation as resolved.
Show resolved Hide resolved
{
std::vector<std::string> _cudf_tmp_args;

for (std::size_t i = 0; i < args.size(); ++i) {
alliepiper marked this conversation as resolved.
Show resolved Hide resolved
std::string arg = args[i];
std::cout << "Argument: " << arg << std::endl;
davidwendt marked this conversation as resolved.
Show resolved Hide resolved
if (arg == cudf::detail::rmm_mode_param) {
i++; // skip the next argument
} else if (arg == cudf::detail::cuio_host_mem_param) {
i++; // skip the next argument
} else {
std::cout << "Adding argument: " << arg << std::endl;
davidwendt marked this conversation as resolved.
Show resolved Hide resolved
davidwendt marked this conversation as resolved.
Show resolved Hide resolved
_cudf_tmp_args.push_back(arg);
}
}

args = _cudf_tmp_args;
pmattione-nvidia marked this conversation as resolved.
Show resolved Hide resolved
}

// Install arg handler
#undef NVBENCH_MAIN_CUSTOM_ARGS_HANDLER
#define NVBENCH_MAIN_CUSTOM_ARGS_HANDLER(args) arg_handler(args)

// Global fixture setup:
#undef NVBENCH_MAIN_INITIALIZE_CUSTOM_POST
#define NVBENCH_MAIN_INITIALIZE_CUSTOM_POST(argc, argv) \
[[maybe_unused]] auto env_state = cudf::nvbench_base_fixture(argc, argv);

// this declares/defines the main() function using the definitions above
NVBENCH_MAIN
29 changes: 0 additions & 29 deletions cpp/cmake/thirdparty/patches/nvbench_global_setup.diff

This file was deleted.

9 changes: 2 additions & 7 deletions cpp/cmake/thirdparty/patches/nvbench_override.json
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,8 @@
{
"packages" : {
"nvbench" : {
"patches" : [
{
"file" : "${current_json_dir}/nvbench_global_setup.diff",
"issue" : "Fix add support for global setup to initialize RMM in nvbench [https://github.com/NVIDIA/nvbench/pull/123]",
"fixed_in" : ""
}
]
"git_url": "https://github.com/NVIDIA/nvbench.git",
"git_tag": "5ee8811a1ac5a90f73a4dc52ab8572c25724a0e8"
Copy link
Contributor

@bdice bdice Apr 9, 2024

Choose a reason for hiding this comment

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

This tag should be updated in https://github.com/rapidsai/rapids-cmake/blob/8b1a1e0e2302ec5a6cfeed762c4f281268e7adca/rapids-cmake/cpm/versions.json#L84 rather than here, if these changes can be applied across RAPIDS.

Copy link
Contributor

Choose a reason for hiding this comment

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

@bdice what's the process for making PR's that depend on rapids-cmake changes?

I don't suppose there's a way to make a cudf PR target the rapids-cmake PR? Or do we need to wait for the rapids-cmake changes to get merged first?

Copy link
Contributor

@bdice bdice Apr 9, 2024

Choose a reason for hiding this comment

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

You can open a PR to rapids-cmake and use that for testing cudf (or other RAPIDS repos that would be affected by the change). Once the rapids-cmake PR is known to be safe, it can be merged (with further changes in each repo merged concurrently, or admin-merged by ops if needed).

See example here: https://github.com/rapidsai/cudf/pull/14704/files#diff-4cf10ebc4636f2671b1909aa0f64141af8b02c1d102764dbbfb34cd493246fb1R29-R30

tl;dr Add something like this in cudf's rapids_config.cmake that points to your fork/branch of rapids-cmake:

set(rapids-cmake-repo bdice/rapids-cmake)
set(rapids-cmake-branch cccl-2.3.0)

This is copied from the rapids-cmake README: https://github.com/rapidsai/rapids-cmake?tab=readme-ov-file#overriding-rapidscmake

Copy link
Contributor

Choose a reason for hiding this comment

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

rapidsai/rapids-cmake#584 is now active to merge these changes upstream.
But once merged it will break cudf benchmarks since our current patch will fail to apply.

So to ensure no breaks, we will need to merge this PR first, merge the rapids-cmake PR, and make a follow-up that removes the nvbench_override.json ( and related CMake call ).

Copy link
Contributor

Choose a reason for hiding this comment

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

We should use the same commit in this PR and the rapids-cmake PR:

Suggested change
"git_tag": "5ee8811a1ac5a90f73a4dc52ab8572c25724a0e8"
"git_tag": "555d628e9b250868c9da003e4407087ff1982e8e"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the SHA. I'll leave this change active following Rob's suggestion above.

Copy link
Contributor

Choose a reason for hiding this comment

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

@robertmaynard I think we are ready to merge this so we can start the merge dance you described above.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds great. 👍

}
}
}
Loading