Fea cleanup stream part1 #1653
Fea cleanup stream part1 #1653
Conversation
Codecov Report
@@ Coverage Diff @@
## branch-21.08 #1653 +/- ##
==============================================
Coverage ? 0.22%
==============================================
Files ? 80
Lines ? 3542
Branches ? 0
==============================================
Hits ? 8
Misses ? 3534
Partials ? 0 Continue to review full report at Codecov.
|
|
Is this PR pretty much complete (and just waiting for CI issues to be resolved) or you have more works to do? I am worried that I will see a crazy number of merge conflicts if I try to merge my DCSR PR with this PR (both will touch many files in common). If this PR is nearly complete, I can pull this to my PR first to avoid resolving merge conflicts later. |
|
I was going to do no more in this PR unless there were some review comments. There's a bunch more files to do this with, but I was going to do them all in batches like this. Just waiting for the CI issues to be resolved to request reviews. |
cpp/include/cugraph/dendrogram.hpp
Outdated
| @@ -27,10 +27,11 @@ class Dendrogram { | |||
| public: | |||
| void add_level(vertex_t first_index, | |||
| vertex_t num_verts, | |||
| cudaStream_t stream, | |||
| rmm::cuda_stream_view const &stream_view, | |||
There was a problem hiding this comment.
Should we pass a const reference here?
Considering that
- cuda_stream_view is just a wrapper for cudaStream_t.
https://github.com/rapidsai/rmm/blob/branch-21.08/include/rmm/cuda_stream_view.hpp#L108 - and cuDF passes cuda_stream_view by value
https://github.com/rapidsai/cudf/blob/branch-21.08/cpp/include/cudf/detail/copy.hpp#L76
https://github.com/rapidsai/cudf/blob/branch-21.08/cpp/include/cudf/detail/groupby.hpp#L47
There was a problem hiding this comment.
Pushed the change to pass by value.
|
@gpucibot merge |
Addresses part of #1538 and #1422
Breaking the work for these issues into smaller chunks to make it easier to test, review and approve.