Enable hugepage for arrow host allocations #13914

madsbk · 2023-08-18T09:28:26Z

This PR enables Transparent Huge Pages (THP) for large (>4MB) arrow allocations (host memory only).

Performance results on a DGX-1 (`dgx14`)

	8MB	80MB	800MB	8GB	Method
cudf-native	0.006	0.049	0.485	4.787	`df.to_arrow()` (branch-23.10)
Dask-serialize	0.004	0.032	0.310	3.122	`distributed.protocol.serialize(df)`
cudf-hugepage	0.004	0.030	0.299	3.046	`df.to_arrow()` (this PR)
speedup	1.5	1.63	1.62	1.57	cudf-native vs. cudf-hugepage

Notice, Dask-serialize also use THP, which is why its performance is on par with cudf-hugepage.

Checklist

I am familiar with the Contributing Guidelines.
New or existing tests cover these changes.
The documentation is up to date with these changes.

cc. @pentschev (I think you did something similar in NumPy)

harrism

One comment.

harrism · 2023-08-22T01:35:21Z

cpp/src/interop/detail/arrow_allocator.cpp

@@ -30,7 +61,7 @@ std::unique_ptr<arrow::Buffer> allocate_arrow_buffer(int64_t const size, arrow::
  */
  auto result = arrow::AllocateBuffer(size, ar_mr);
  CUDF_EXPECTS(result.ok(), "Failed to allocate Arrow buffer");
-  return std::move(result).ValueOrDie();
+  return enable_hugepage(std::move(result).ValueOrDie());


What is the reason for std::move here? I'm not sure you need that.

Generally return std::move(...) is an antipattern. When you are returning a local variable, usually the copy will be elided anyway. If there is a move ctor, it can be used automatically.

Notice, we use std::move on the intermediate result, not the value returned by ValueOrDie(). This moves the value (std::unique_ptr<arrow::Buffer>) out of result .
I have added explicit types to make this more clear.

Right, I was assuming you just copied the std::move from the code that was there before, which just had return std::move, which I think was unnecessary (and disables the compiler from eliding the move with RVO, IIUC).

The compiler does not allow return std::move(retval) in libcudf because it disables RVO. But std::move(result).ValueOrDie() is slightly different, as it does not move the return value, just casts result to rvalue, presumably so that ValueOrDie can move the value out of result. The moved value is most likely then RVOd to the caller (not sure how this is actually called :)).

vuule

a few questions/suggestions

cpp/src/interop/detail/arrow_allocator.cpp

vuule · 2023-08-22T23:26:59Z

cpp/src/interop/detail/arrow_allocator.cpp

+  significantly, see <https://github.com/rapidsai/cudf/pull/13914>.
+*/
+template <typename T>
+T enable_hugepage(T buf)


Should we limit the type here, since we don't want to ever copy T when calling the function?

Suggested change

T enable_hugepage(T buf)

T enable_hugepage(T&& buf)

This requires the use of return std::move(buf) otherwise it fails:

/home/nfs/mkristensen/repos/cudf/cpp/src/interop/detail/arrow_allocator.cpp:36:12: error: use of deleted function 'std::unique_ptr<_Tp, _Dp>::unique_ptr(const std::unique_ptr<_Tp, _Dp>&) [with _Tp = arrow::Buffer; _Dp = std::default_delete<arrow::Buffer>]' 36 | return buf; | ^~~ In file included from /datasets/mkristensen/miniconda3/envs/cudf-0818/x86_64-conda-linux-gnu/include/c++/11.4.0/memory:76, from /datasets/mkristensen/miniconda3/envs/cudf-0818/include/arrow/array/array_base.h:22, from /datasets/mkristensen/miniconda3/envs/cudf-0818/include/arrow/array.h:41, from /datasets/mkristensen/miniconda3/envs/cudf-0818/include/arrow/api.h:22, from /home/nfs/mkristensen/repos/cudf/cpp/include/cudf/detail/interop.hpp:26, from /home/nfs/mkristensen/repos/cudf/cpp/src/interop/detail/arrow_allocator.cpp:17: /datasets/mkristensen/miniconda3/envs/cudf-0818/x86_64-conda-linux-gnu/include/c++/11.4.0/bits/unique_ptr.h:468:7: note: declared here 468 | unique_ptr(const unique_ptr&) = delete; | ^~~~~~~~~~ /home/nfs/mkristensen/repos/cudf/cpp/src/interop/detail/arrow_allocator.cpp:42:33: error: use of deleted function 'std::unique_ptr<_Tp, _Dp>::unique_ptr(const std::unique_ptr<_Tp, _Dp>&) [with _Tp = arrow::Buffer; _Dp = std::default_delete<arrow::Buffer>]' 42 | if (addr == nullptr) { return buf; } | ^~~ In file included from /datasets/mkristensen/miniconda3/envs/cudf-0818/x86_64-conda-linux-gnu/include/c++/11.4.0/memory:76, from /datasets/mkristensen/miniconda3/envs/cudf-0818/include/arrow/array/array_base.h:22, from /datasets/mkristensen/miniconda3/envs/cudf-0818/include/arrow/array.h:41, from /datasets/mkristensen/miniconda3/envs/cudf-0818/include/arrow/api.h:22, from /home/nfs/mkristensen/repos/cudf/cpp/include/cudf/detail/interop.hpp:26, from /home/nfs/mkristensen/repos/cudf/cpp/src/interop/detail/arrow_allocator.cpp:17: /datasets/mkristensen/miniconda3/envs/cudf-0818/x86_64-conda-linux-gnu/include/c++/11.4.0/bits/unique_ptr.h:468:7: note: declared here 468 | unique_ptr(const unique_ptr&) = delete; | ^~~~~~~~~~ /home/nfs/mkristensen/repos/cudf/cpp/src/interop/detail/arrow_allocator.cpp:50:10: error: use of deleted function 'std::unique_ptr<_Tp, _Dp>::unique_ptr(const std::unique_ptr<_Tp, _Dp>&) [with _Tp = arrow::Buffer; _Dp = std::default_delete<arrow::Buffer>]' 50 | return buf; | ^~~ In file included from /datasets/mkristensen/miniconda3/envs/cudf-0818/x86_64-conda-linux-gnu/include/c++/11.4.0/memory:76, from /datasets/mkristensen/miniconda3/envs/cudf-0818/include/arrow/array/array_base.h:22, from /datasets/mkristensen/miniconda3/envs/cudf-0818/include/arrow/array.h:41, from /datasets/mkristensen/miniconda3/envs/cudf-0818/include/arrow/api.h:22, from /home/nfs/mkristensen/repos/cudf/cpp/include/cudf/detail/interop.hpp:26, from /home/nfs/mkristensen/repos/cudf/cpp/src/interop/detail/arrow_allocator.cpp:17: /datasets/mkristensen/miniconda3/envs/cudf-0818/x86_64-conda-linux-gnu/include/c++/11.4.0/bits/unique_ptr.h:468:7: note: declared here 468 | unique_ptr(const unique_ptr&) = delete; | ^~~~~~~~~~

cpp/src/interop/detail/arrow_allocator.cpp

Fixes: #13864 This PR fixes an issue with `loc` indexer where some special handling needs to be done when `columns` is of type `MultiIndex`. Authors: - GALI PREM SAGAR (https://github.com/galipremsagar) Approvers: - Lawrence Mitchell (https://github.com/wence-) URL: #13929

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

vuule · 2023-08-23T07:17:25Z

Looks like RVO is not happy with the recent parameter type change to T&&?

vuule · 2023-08-23T07:19:31Z

cpp/src/interop/detail/arrow_allocator.cpp

+  significantly, see <https://github.com/rapidsai/cudf/pull/13914>.
+*/
+template <typename T>
+T enable_hugepage(T&& buf)


could we have enable_hugepage just take a host_span? Do we really need to forward std::unique_ptr<arrow::Buffer> through the function?

We don't need to but it means we can avoid an in-place function and have a simply return wrapper return enable_hugepage(...).

I still don't see the benefit, and I'm fine with that.

madsbk · 2023-08-24T08:31:30Z

/merge

madsbk added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Aug 18, 2023

github-actions bot added libcudf Affects libcudf (C++/CUDA) code. Python Affects Python cuDF API. labels Aug 18, 2023

enable_hugepage

bf367de

madsbk force-pushed the huge_page branch from 6ed6212 to bf367de Compare August 18, 2023 09:41

github-actions bot removed the Python Affects Python cuDF API. label Aug 18, 2023

madsbk marked this pull request as ready for review August 18, 2023 11:45

madsbk requested a review from a team as a code owner August 18, 2023 11:45

madsbk requested review from harrism and vuule August 18, 2023 11:45

harrism reviewed Aug 22, 2023

View reviewed changes

madsbk added 3 commits August 21, 2023 23:33

doc

167e154

Merge branch 'branch-23.10' of github.com:rapidsai/cudf into huge_page

c75d6b6

typo

b982ab4

harrism approved these changes Aug 22, 2023

View reviewed changes

vuule reviewed Aug 22, 2023

View reviewed changes

madsbk and others added 2 commits August 23, 2023 08:19

clean up

d9ab05c

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

Merge branch 'branch-23.10' into huge_page

0599b45

vuule reviewed Aug 23, 2023

View reviewed changes

std::move(buf)

52705a0

jlowe mentioned this pull request Aug 23, 2023

Consider using hugepage for some host memory buffer allocations NVIDIA/spark-rapids-jni#1366

Open

vuule approved these changes Aug 23, 2023

View reviewed changes

Merge branch 'branch-23.10' into huge_page

41298b7

rapids-bot bot merged commit f70f2cd into rapidsai:branch-23.10 Aug 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable hugepage for arrow host allocations #13914

Enable hugepage for arrow host allocations #13914

madsbk commented Aug 18, 2023 •

edited

Loading

harrism left a comment

harrism Aug 22, 2023

madsbk Aug 22, 2023

harrism Aug 22, 2023

vuule Aug 22, 2023

vuule left a comment

vuule Aug 22, 2023

madsbk Aug 23, 2023

vuule commented Aug 23, 2023

vuule Aug 23, 2023

madsbk Aug 23, 2023

vuule Aug 23, 2023

madsbk commented Aug 24, 2023

Enable hugepage for arrow host allocations #13914

Enable hugepage for arrow host allocations #13914

Conversation

madsbk commented Aug 18, 2023 • edited Loading

Performance results on a DGX-1 (dgx14)

Checklist

harrism 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

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

vuule commented Aug 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

madsbk commented Aug 24, 2023

madsbk commented Aug 18, 2023 •

edited

Loading

Performance results on a DGX-1 (`dgx14`)