-
Notifications
You must be signed in to change notification settings - Fork 301
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
[REVIEW] Improve Random Walks performance #1685
Conversation
Merge latest release 0.17
Merge latest branch-0.18
Update forked branch-0.18
Update forked branch-0.18 from release
Update branch-0.19 from release
update forked from release branch-0.19
Merge from release branch-0.19
Merge latest rapidsai:branch-0.19 into aschaffer:branch-0.19
Create forked branch-0.20
merge latest release branch-0.20
Merge latest rapidsai/branch-0.20 into forked branch-0.20
aschaffer/21.08 <- rapidsai/21.08
…mory available then HORIZONTAL strategy is used; else VERTICAL.
Codecov Report
@@ Coverage Diff @@
## branch-21.08 #1685 +/- ##
================================================
- Coverage 59.77% 59.71% -0.06%
================================================
Files 80 80
Lines 3540 3540
================================================
- Hits 2116 2114 -2
- Misses 1424 1426 +2
Continue to review full report at Codecov.
|
size_t coalesced_v_count = num_paths * max_depth; | ||
auto coalesced_e_count = coalesced_v_count - num_paths; | ||
size_t req_mem_common = sizeof(vertex_t) * coalesced_v_count + | ||
sizeof(weight_t) * coalesced_e_count + // coalesced_v + coalesced_w |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand the comments here without the context. Is it dead code or are you explaining something here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am explaining how the req_mem_common
was calculated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @kaatish , can this be resolved so we can converge towards an approval? TIA.
graph_t const& graph, // graph being traversed | ||
random_walker_t const& rand_walker, // random walker object for which traversal is driven | ||
seed_t seed0, // initial seed value | ||
device_vec_t<typename graph_t::vertex_type>& d_coalesced_v, // crt coalesced vertex set |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
crt implies current here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @kaatish , can this be resolved so we can converge towards an approval? TIA.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can continue discussion/changes related to the cudaGetMemInfo
call separately and address anything we decide to do in a separate PR. Otherwise I think this looks good.
@gpucibot merge |
This tracks work on adding additional Random Walks traversal strategy: horizontal. This strategy is more efficient, but requires more GPU memory.
The previous strategy (vertical) is only used when not enough GPU memory is available to support the horizontal one. The decision on which to use is based on GPU memory availability: if enough memory is available then the more performant (horizontal) strategy is used.