Testing stream pool implementation #14437

shrshi · 2023-11-16T23:52:34Z

Description

The goal of this PR is to create a global pool containing only the cudf test stream which is to be used all stream tests that invoke fork_streams in their execution path. The stream pool is constructed by create_global_cuda_stream_pool() in identify_stream_usage.cpp, and overrides the function implementation in utilities/stream_pool.cpp when preloaded.

The test checks for only cudaLaunchKernel being invoked with the wrong stream.

Checklist

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

…_global_cuda_stream_pool

copy-pr-bot · 2023-11-16T23:52:38Z

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

shrshi · 2023-11-17T00:01:05Z

/ok to test

shrshi · 2023-11-17T00:25:36Z

/ok to test

shrshi · 2023-11-17T17:37:02Z

/ok to test

harrism

The code looks fine, but I have a little bit of confusion that I would like to clear up. Thanks!

cpp/tests/streams/pool_test.cu

cpp/tests/utilities/identify_stream_usage.cpp

vyasr · 2023-11-29T01:57:38Z

cpp/include/cudf/detail/utilities/stream_pool.hpp

This is just a copy-paste of the declarations from stream_pool.cpp, right? It's a good change, just want to make sure I'm not missing any other changes.

Yes, I have moved the parent class declaration and create_global_cuda_stream_pool from stream_pool.cpp to the header file so that test_cuda_stream_pool in identify_stream_usage.cpp can include it.

Notes from an offline discussion - cuda_stream_pool was "hidden" in the source file on purpose; we want the pool usage to be limited to fork_streams/join_streams. The change here exposes more of the stream pool than we'd like.
However, we don't see a better solution, since the current approach at least does not require additional APIs in libcudf.

cpp/tests/utilities/identify_stream_usage.cpp

vyasr · 2023-11-29T02:01:14Z

cpp/tests/utilities/identify_stream_usage.cpp

+class test_cuda_stream_pool : public cuda_stream_pool {
+ public:
+  rmm::cuda_stream_view get_stream() override { return cudf::test::get_default_stream(); }
+  rmm::cuda_stream_view get_stream(stream_id_type stream_id) override


Can we remove the variable name since it's never used (or if that causes unforeseen problems, mark it as [[ maybe_unused ]])?

Suggested change

rmm::cuda_stream_view get_stream(stream_id_type stream_id) override

rmm::cuda_stream_view get_stream(stream_id_type) override

cpp/tests/utilities/identify_stream_usage.cpp

…ting-stream-pool

shrshi · 2023-12-12T00:59:53Z

/ok to test

shrshi · 2023-12-12T01:14:13Z

/ok to test

cpp/include/cudf/detail/utilities/stream_pool.hpp

cpp/tests/utilities/identify_stream_usage.cpp

… comment Co-authored-by: Vyas Ramasubramani <[email protected]>

shrshi · 2023-12-12T22:38:52Z

/ok to test

shrshi · 2023-12-12T23:25:51Z

/ok to test

cpp/tests/streams/pool_test.cu

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

shrshi · 2023-12-13T22:51:39Z

/ok to test

ttnghia · 2023-12-14T00:21:47Z

cpp/include/cudf/detail/utilities/stream_pool.hpp

+   *
+   * @return the number of stream objects in the pool
+   */
+  virtual std::size_t get_stream_pool_size() const = 0;


The name is a bit redundant. We know that this is a stream pool.

Suggested change

virtual std::size_t get_stream_pool_size() const = 0;

virtual std::size_t get_pool_size() const = 0;

or just size().

get_pool_size() is consistent with rmm::cuda_stream_pool. Surprised this wasn't caught before.

yeah, should be get_pool_size. Not a change for this PR, IMO.

Okay, but then please address it in a follow up work.

Thank you for pointing this out! I'll work on this change in a follow-up PR.

Why isn't this a change for this PR, if all the code in this section is new in this PR?

cpp/tests/streams/pool_test.cu

ttnghia · 2023-12-14T00:24:49Z

cpp/tests/streams/pool_test.cu

+
+class StreamPoolTest : public cudf::test::BaseFixture {};
+
+__global__ void do_nothing_kernel() {}


I am not sure if this kernel without any code will be optimized out so the for loop below will never be executed?

Thanks for pointing this out! I printed the GPU trace from the nsys profiler to check this and it shows that the do_nothing_kernel is being executed.

shrshi · 2023-12-14T06:13:45Z

/ok to test

harrism

Nice work.

vuule · 2023-12-19T17:42:47Z

/merge

shrshi added 2 commits November 15, 2023 23:08

extern c dlopen capers

2ac86e8

moved test stream pool creation to preload library to override create…

1247e6d

…_global_cuda_stream_pool

shrshi added CMake CMake build issue improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Nov 16, 2023

github-actions bot added the libcudf Affects libcudf (C++/CUDA) code. label Nov 16, 2023

Cmakelists cleanup

3fa0140

Merge branch 'branch-24.02' into testing-stream-pool

3959e9e

shrshi marked this pull request as ready for review November 17, 2023 19:49

shrshi requested a review from a team as a code owner November 17, 2023 19:49

shrshi requested review from harrism and davidwendt November 17, 2023 19:49

GregoryKimball requested review from vyasr and vuule November 20, 2023 20:11

harrism requested changes Nov 21, 2023

View reviewed changes

cpp/tests/streams/pool_test.cu Outdated Show resolved Hide resolved

cpp/tests/streams/pool_test.cu Outdated Show resolved Hide resolved

cpp/tests/utilities/identify_stream_usage.cpp Show resolved Hide resolved

cpp/tests/utilities/identify_stream_usage.cpp Show resolved Hide resolved

vyasr requested changes Nov 29, 2023

View reviewed changes

shrshi added 3 commits December 12, 2023 00:57

adding comments

c612fd6

Merge branch 'branch-24.02' into testing-stream-pool

fe166a4

Merge branch 'testing-stream-pool' of github.com:shrshi/cudf into tes…

ca98a45

…ting-stream-pool

forgot to run precommit checks

4e04b8e

shrshi requested a review from vyasr December 12, 2023 01:17

vyasr approved these changes Dec 12, 2023

View reviewed changes

cpp/include/cudf/detail/utilities/stream_pool.hpp Outdated Show resolved Hide resolved

cpp/tests/utilities/identify_stream_usage.cpp Show resolved Hide resolved

cpp/tests/utilities/identify_stream_usage.cpp Show resolved Hide resolved

Update cpp/include/cudf/detail/utilities/stream_pool.hpp - clarifying…

a720444

… comment Co-authored-by: Vyas Ramasubramani <[email protected]>

Merge branch 'branch-24.02' into testing-stream-pool

362cdd1

vuule reviewed Dec 13, 2023

View reviewed changes

cpp/tests/streams/pool_test.cu Show resolved Hide resolved

vuule reviewed Dec 13, 2023

View reviewed changes

cpp/tests/streams/pool_test.cu Outdated Show resolved Hide resolved

Update cpp/tests/streams/pool_test.cu - looping through streams in test

bb54197

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

ttnghia reviewed Dec 14, 2023

View reviewed changes

cpp/tests/streams/pool_test.cu Outdated Show resolved Hide resolved

ttnghia reviewed Dec 14, 2023

View reviewed changes

cpp/tests/streams/pool_test.cu Show resolved Hide resolved

ttnghia reviewed Dec 14, 2023

View reviewed changes

addressing PR reviews

c4e72d2

vuule approved these changes Dec 14, 2023

View reviewed changes

ttnghia approved these changes Dec 14, 2023

View reviewed changes

vuule requested a review from harrism December 14, 2023 23:34

harrism approved these changes Dec 19, 2023

View reviewed changes

rapids-bot bot merged commit 5dfafaf into rapidsai:branch-24.02 Dec 19, 2023
67 checks passed

shrshi mentioned this pull request Jan 5, 2024

Expose streams in ORC reader and writer APIs #14350

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Testing stream pool implementation #14437

Testing stream pool implementation #14437

shrshi commented Nov 16, 2023

copy-pr-bot bot commented Nov 16, 2023

shrshi commented Nov 17, 2023

shrshi commented Nov 17, 2023

shrshi commented Nov 17, 2023

harrism left a comment

vyasr Nov 29, 2023

shrshi Dec 12, 2023 •

edited

Loading

vuule Dec 13, 2023

vyasr Nov 29, 2023

shrshi commented Dec 12, 2023

shrshi commented Dec 12, 2023

shrshi commented Dec 12, 2023

shrshi commented Dec 12, 2023

shrshi commented Dec 13, 2023

ttnghia Dec 14, 2023

etseidl Dec 14, 2023

vuule Dec 14, 2023

ttnghia Dec 14, 2023

shrshi Dec 14, 2023

harrism Dec 19, 2023

ttnghia Dec 14, 2023

shrshi Dec 14, 2023

shrshi commented Dec 14, 2023

harrism left a comment

vuule commented Dec 19, 2023

	rmm::cuda_stream_view get_stream(stream_id_type stream_id) override
	rmm::cuda_stream_view get_stream(stream_id_type) override

	virtual std::size_t get_stream_pool_size() const = 0;
	virtual std::size_t get_pool_size() const = 0;


		class StreamPoolTest : public cudf::test::BaseFixture {};

		__global__ void do_nothing_kernel() {}

Testing stream pool implementation #14437

Testing stream pool implementation #14437

Conversation

shrshi commented Nov 16, 2023

Description

Checklist

copy-pr-bot bot commented Nov 16, 2023

shrshi commented Nov 17, 2023

shrshi commented Nov 17, 2023

shrshi commented Nov 17, 2023

harrism left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shrshi Dec 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shrshi commented Dec 12, 2023

shrshi commented Dec 12, 2023

shrshi commented Dec 12, 2023

shrshi commented Dec 12, 2023

shrshi commented Dec 13, 2023

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

shrshi commented Dec 14, 2023

harrism left a comment

Choose a reason for hiding this comment

vuule commented Dec 19, 2023

shrshi Dec 12, 2023 •

edited

Loading