Support for dot(dns, csr) = dns and dot(dns, csr.T) = dns on CPU by XiaotaoChen · Pull Request #11113 · apache/mxnet

XiaotaoChen · 2018-05-31T16:51:45Z

Description

Support for dot(dns, csr) = dns and dot(dns, csr.T) = dns on CPU

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

The PR title starts with [MXNET-JIRA_ID], where JIRA_ID refers to the relevant JIRA issue created (except PRs with tiny changes)
Changes are complete (i.e. I finished coding on this PR)
All changes have test coverage:
Unit tests are added for small changes to verify correctness (e.g. adding a new operator)
Nightly tests are added for complicated/long-running ones (e.g. changing distributed kvstore)
Build tests will be added for build configuration changes (e.g. adding a new build option with NCCL)
Code is well-documented:
For user-facing API changes, API doc string has been updated.
For new C++ functions in header files, their functionalities and arguments are documented.
For new examples, README.md is added to explain the what the example does, the source of the dataset, expected performance on test set and reference to the original paper if applicable
Check the API doc at http://mxnet-ci-doc.s3-accelerate.dualstack.amazonaws.com/PR-PR_ID/BUILD_ID/index.html
To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

Changes

Feature1, tests, (and when applicable, API doc)
Feature2, tests, (and when applicable, API doc)

Comments

We implemented dot(dns,csr/csr.T)=dns on cpu inspired by the implementation of dot(dns,csr/csr.T)=dns on gpu by @haojin2 . It gains 190 times speed up when density of csr is 0.01%. The details as follows.

---test dot(dns1, csr) = dns2, shape_lhs=(256,30000), shape_rhs=(30000,30000)---
1.00 % with fallback: 2.205663, without fallback: 1.179663
0.50 % with fallback: 4.471481, without fallback: 2.309979
0.10 % with fallback: 20.552546, without fallback: 10.095501
0.05 % with fallback: 39.141270, without fallback: 18.930919
0.01 % with fallback: 155.199133, without fallback: 75.759737
---test dot(dns1, csr.T) = dns2, shape_lhs=(256,30000), shape_rhs=(30000,30000)---
1.00 % with fallback: 3.031672, without fallback: 1.474902
0.50 % with fallback: 6.197231, without fallback: 3.054169
0.10 % with fallback: 29.555133, without fallback: 14.591607
0.05 % with fallback: 56.621382, without fallback: 28.377709
0.01 % with fallback: 190.064799, without fallback: 94.031036

The benchmark script is here: https://github.com/XiaotaoChen/incubator-mxnet/blob/mytest/example/sparse/cxt-test/test_dot.py . And also can test this with haojin's script by replacing mx.gpu() with mx.cpu() : #10371
@pengzhao-intel @TaoLv

pengzhao-intel · 2018-06-01T04:29:48Z

@haojin2 @eric-haibin-lin for the review :)

haojin2 · 2018-06-01T04:59:46Z

src/operator/tensor/dot-inl.h

        dispatched = storage_type_assign(&out_stype, kCSRStorage, dispatch_mode,
                                         DispatchMode::kFComputeEx);
+      // dns, csr/csr.T -> dns on CPU
+      } else if (target_stype ==  kDefaultStorage) {


nit: extra space at "== kDefaultStorage"

haojin2 · 2018-06-01T05:00:44Z

src/operator/tensor/dot-inl.h

    }
  }
-  if (!dispatched && dev_mask == mshadow::gpu::kDevMask && !param.transpose_a &&
+  // if (!dispatched && dev_mask == mshadow::gpu::kDevMask && !param.transpose_a &&


Please remove unused code.

haojin2 · 2018-06-01T05:07:43Z

src/operator/tensor/dot-inl.h

+              , data_r.dptr<DType>(), indptr_r.dptr<IType>()
+              , col_idx_r.dptr<CType>(), seg_len
+              , dns.shape_[0], dns.shape_[1]
+              , rhs.shape()[0], rhs.shape()[1]);


For line breaks please follow google c++ style guide, a sample may be found here: https://gist.github.com/davidzchen/9187878#file-sample-google-c-L110-L114

haojin2 · 2018-06-01T05:08:44Z

src/operator/tensor/dot-inl.h

                             const OpReqType req, NDArray* ret,
                             const bool transpose_b) {
-  LOG(FATAL) << "dot(dense, csr) = dense is not implemented on CPU";
+  if (kNullOp == req) return;


nit: personally I would prefer "req == kNullOp".

haojin2 · 2018-06-01T05:09:06Z

src/operator/tensor/dot-inl.h

+    MSHADOW_IDX_TYPE_SWITCH(indptr_r.type_flag_, IType, {  // indptr type
+      MSHADOW_IDX_TYPE_SWITCH(col_idx_r.type_flag_, CType, {  // col idx type
+        dim_t num_threads;
+        if (kWriteTo == req) {


nit: Personally I would prefer "req == kWriteTo"

Thx, I have modified those according to your suggestion.

eric-haibin-lin · 2018-06-02T06:12:13Z

src/operator/tensor/dot-inl.h

  if (!dispatched && lhs_stype == kDefaultStorage && rhs_stype == kCSRStorage &&
      !param.transpose_a) {
    target_stype = hint_has_value ? target_stype : kCSRStorage;
-    // dns, csr -> csr on CPU


Please also update doc in https://github.com/apache/incubator-mxnet/blob/master/src/operator/tensor/dot.cc#L63-L64

eric-haibin-lin · 2018-06-02T06:16:10Z

src/operator/tensor/dot-inl.h

+struct DotDnsCsrTransDnsByRowBlocks {
+  /*!
+   * \brief
+   * \param i the i-th thread


Please complete documentation

Thx, it's done.

XiaotaoChen · 2018-06-08T00:53:38Z

@haojin2 @eric-haibin-lin I have solved the problems mentioned in your comments. Can you accept this PR ?

haojin2 · 2018-06-08T00:55:14Z

LGTM, will wait for @eric-haibin-lin to take a final look.

eric-haibin-lin

Thanks for the contribution! a few comments

eric-haibin-lin · 2018-06-08T23:15:52Z

src/operator/tensor/dot-inl.h

+  const TBlob& data_l = dns;
+  const TBlob data_out = ret->data();
+
+  MSHADOW_SGL_DBL_TYPE_SWITCH(data_r.type_flag_, DType, {  // data type


fp16 will fail with this branch. I think there's a MSHADOW_REAL_TYPE_SWITCH

eric-haibin-lin · 2018-06-08T23:19:38Z

src/operator/tensor/dot-inl.h

+    MSHADOW_IDX_TYPE_SWITCH(indptr_r.type_flag_, IType, {  // indptr type
+      MSHADOW_IDX_TYPE_SWITCH(col_idx_r.type_flag_, CType, {  // col idx type
+        dim_t num_threads;
+        if (req == kWriteTo) {


req == writeto || req == writeinplace

Thx, it's Done ! May you review it again :)

…AL_TYPE_SWITCH

XiaotaoChen · 2018-06-10T12:54:29Z

Thx, it's Done ! May you review it again :) @eric-haibin-lin

…che#11113) * implement dot(dns, csr/csr.T)=dns on cpu * complete documentaion related to dot(dns, csr/csr.T)=dns on cpu * support fp16 by replacing MSHADOW_SGL_DBL_TYPE_SWITCH with MSHADOW_REAL_TYPE_SWITCH

XiaotaoChen force-pushed the cxt-dns_csr/csr.T_dns branch 2 times, most recently from c481dff to 99b4bf8 Compare June 1, 2018 01:24

haojin2 reviewed Jun 1, 2018

View reviewed changes

XiaotaoChen force-pushed the cxt-dns_csr/csr.T_dns branch 4 times, most recently from 5b78686 to 7fad590 Compare June 2, 2018 01:54

implement dot(dns, csr/csr.T)=dns on cpu

dd989d6

XiaotaoChen force-pushed the cxt-dns_csr/csr.T_dns branch from 7fad590 to dd989d6 Compare June 2, 2018 04:12

eric-haibin-lin reviewed Jun 2, 2018

View reviewed changes

XiaotaoChen added 2 commits June 2, 2018 15:58

complete documentaion related to dot(dns, csr/csr.T)=dns on cpu

8f53309

Merge branch 'master' into cxt-dns_csr/csr.T_dns

4a7389c

haojin2 approved these changes Jun 8, 2018

View reviewed changes

eric-haibin-lin reviewed Jun 8, 2018

View reviewed changes

XiaotaoChen added 2 commits June 9, 2018 09:17

Merge branch 'master' into cxt-dns_csr/csr.T_dns

08394b4

support fp16 by replacing MSHADOW_SGL_DBL_TYPE_SWITCH with MSHADOW_RE…

84cc230

…AL_TYPE_SWITCH

eric-haibin-lin merged commit 935fc55 into apache:master Jun 10, 2018

Conversation

XiaotaoChen commented May 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pengzhao-intel commented Jun 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

XiaotaoChen commented Jun 8, 2018

Uh oh!

haojin2 commented Jun 8, 2018

Uh oh!

eric-haibin-lin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

XiaotaoChen commented Jun 10, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

XiaotaoChen commented May 31, 2018 •

edited

Loading