This repository has been archived by the owner on Nov 17, 2023. It is now read-only.
Accelerate DGL csr neighbor sampling #13588
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@aksnzhy could you please show the benchmark result? |
|
The benchmark result are as follows: |
|
The improvement in this PR has about 2-3 times speedup over the implementation in the master branch. The speed in this PR: |
zheng-da
force-pushed
the
accel_sampling
branch
from
3ee7bec
to
043c1b4
Compare
|
@BullDemonKing Thanks for the contribution! could you take a look at failed tests? |
|
@mxnet-label-bot add[Operator, pr-awaiting-review] |
aksnzhy
reviewed
Dec 16, 2018
| std::queue<ver_node> node_queue; | ||
| std::unordered_set<dgl_id_t> sub_ver_mp; | ||
| std::vector<std::pair<dgl_id_t, dgl_id_t> > sub_vers; | ||
| sub_vers.reserve(num_seeds * 10); |
There was a problem hiding this comment.
Is 10 a general good constant for sampling?
There was a problem hiding this comment.
This number is used for reserving memory, so it doesn't have to be very accurate. The goal here is to balance memory reallocation and memory consumption in std::vector. If num_hop is 1, 10 might be good enough; if num_hop is 2, 10 will be too small. But I don't feel that we need to make it overcomplex. After all, we only want to reduce the overhead of memory reallocation in std::vector.
|
It looks really good for this pr. |
| } | ||
| for (dgl_id_t i = num_vertices+1; i <= max_num_vertices; ++i) { | ||
| for (size_t i = num_vertices+1; i <= max_num_vertices; ++i) { | ||
| indptr_out[i] = indptr_out[i-1]; | ||
| } | ||
| } |
There was a problem hiding this comment.
Do you mind also enhancing the documentation for the operator? For example, the number of outputs and what each output is for. For example, -1 will be filled for vertex id not sampled.
Also, printing the output of a.asnumpy() will be helpful
[[ 0 1 2 3 4]
[ 5 0 6 7 8]
[ 9 10 0 11 12]
[13 14 15 0 16]
[17 18 19 20 0]]
There was a problem hiding this comment.
Regarding the example usage:
out = mx.nd.contrib.dgl_csr_neighbor_uniform_sample(a, seed, num_args=2, num_hops=1, num_neighbor=2, max_num_vertices=5)
I don't think num_args has to be set by the user. It should be automatically set https://github.com/apache/incubator-mxnet/blob/779bdc5e7ee3abd6a2d23e8bd97d47fc08ae6bc5/src/imperative/imperative_utils.h#L311-L313
There was a problem hiding this comment.
Also, the current example seed doesn't result in -1 in the output. Maybe we want to pick some input which is more representative
| // Let's check if there is a vertex that we haven't sampled its neighbors. | ||
| for (; idx < sub_vers.size(); idx++) { | ||
| if (sub_vers[idx].second < num_hops) { | ||
| LOG(WARNING) |
There was a problem hiding this comment.
Is LOG(WARNING) working? I was not aware that this is functional.
There was a problem hiding this comment.
it works. i can see warning messages after the number of sampled vertices exceeds the maximal number.
eric-haibin-lin
approved these changes
Dec 18, 2018
| @@ -718,20 +691,37 @@ static void SampleSubgraph(const NDArray &csr, | |||
| dgl_id_t* indptr_out = sub_csr.aux_data(0).dptr<dgl_id_t>(); | |||
There was a problem hiding this comment.
what is the problem of the indent here?
There was a problem hiding this comment.
nvm. Incorrect rendering by github. looks good
haohuanw
pushed a commit
to haohuanw/incubator-mxnet
that referenced
this pull request
Jun 23, 2019
* Speedup and fix bug in dgl_csr_sampling op * Update dgl_graph.cc * simplify functions. * avoid adding nodes in the last level in the queue. * remove a hashtable lookup in neigh_pos. * reduce a hashtable lookup in sub_ver_mp. * merge copying vids and layers. * reduce hashtable lookup when writing to output csr. * fix a bug. * limit the number of sampled vertices. * fix lint. * fix a compile error. * fix compile error. * fix compile. * remove one hashtable lookup per vertex and hashtable iteration. * remove queue. * use vector for neigh_pos. * fix lint * avoid init output arrays. * fix tests. * fix tests. * update docs. * retrigger * retrigger
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
The DGL csr neighbor sampling has many hashtable lookups. Hashtable lookups turn out to be expensive operations. This PR tries to accelerate sampling by reducing hashtable lookups.
Checklist
Essentials
Please feel free to remove inapplicable items for your PR.
Changes
Comments