[MXNET-365] handle inplace in mkldnn FallBackCompute #10591

ashokei · 2018-04-18T00:41:21Z

Description

handle inplace in mkldnn FallBackCompute. This bug is noticed in mkldnn sum, if one of the inputs is cpu layout

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

If this change is a backward incompatible change, why must this change be made.
Interesting edge cases to note here

zheng-da · 2018-04-18T04:56:34Z

src/operator/nn/mkldnn/mkldnn_base.cc

-    if (req[i] == kWriteTo)
+    // ensure output does not use mkldnn mem.
+    // for inplace, we already converted & copied input above.
+    if ((req[i] == kWriteTo) || (req[i] == kWriteInplace))
      const_cast<NDArray &>(outputs[i]).InvalidateMKLDNNData();


Actually, we need to handle kAddTo properly here as well. If it's kAddTo, we can convert the layout of the output array to the default format directly. since it's output array, we expect its memory will be written.

zheng-da · 2018-04-20T00:41:40Z

src/operator/nn/mkldnn/mkldnn_base.cc

+    // for inplace, we already converted & copied input above.
+    if ((req[i] == kWriteTo) || (req[i] == kWriteInplace))
+      const_cast<NDArray &>(output).InvalidateMKLDNNData();
+    if (req[i] == kAddTo)


could you make this "else if"?

ashokei · 2018-04-26T20:38:04Z

@zheng-da added your feedback comments, @piiswrong can you please merge if ok. thanks.

zheng-da · 2018-04-26T23:25:54Z

can you add a test for this?

ashokei · 2018-04-27T03:37:07Z

@zheng-da added unittest for this, please accept review if ok , thanks

ashokei · 2018-05-02T20:53:06Z

@piiswrong can you please merge, thanks.

ashokei · 2018-05-04T00:31:58Z

@piiswrong ping...

ashokei · 2018-05-05T01:17:31Z

@marcoabreu is this something you can merge. thanks.

marcoabreu

MKL tests are currently not run. Please remove --build from https://github.com/apache/incubator-mxnet/blob/master/Jenkinsfile#L112

zheng-da · 2018-05-05T15:59:48Z

Why do MKL tests not run? We need to enable them to test MKLDNN-specific scripts, right?

marcoabreu · 2018-05-05T16:10:18Z

There is a mistake in our launch command

ashokei · 2018-05-08T00:13:48Z

@marcoabreu made requested changes, can we please merge

zheng-da · 2018-05-08T00:58:15Z

tests/python/mkl/test_mkldnn.py

        # output should have 0.3376348
-        assert_almost_equal(y[0, 0, 0, 0], 0.3376348)
+        assert_almost_equal(y[0, 0, 0, 0], 0.016711406)


could you remind me what this is? why do you need to change it?
BTW, you need to update the comment as well.

it just computes with fixed 1s as input, and this is the value i see as a result with GPU or cpu (non-mkldnn mode).

ashokei · 2018-05-08T21:20:31Z

@marcoabreu this one is ready for merge too, made changes suggested by you. thanks.

marcoabreu · 2018-05-08T21:21:56Z

Let me just retrigger CI

marcoabreu · 2018-05-08T21:23:22Z

src/operator/nn/mkldnn/mkldnn_base.cc

+    // ensure output does not use mkldnn mem.
+    // for inplace, we already converted & copied input above.
+    if ((req[i] == kWriteTo) || (req[i] == kWriteInplace))
+      const_cast<NDArray &>(output).InvalidateMKLDNNData();


Is this threadsafe? What happens if multiple threads try to read/write or they call it in sequence?

called in sequence, this PR only adds kWriteInplace option to existing kWriteTo handling.

What I mean is whether this operation does have any side effects that could cause problems if they are called from multiple threads in parallel or multiple times in sequence.
We have seen quite a few regressions in terms of race conditions related to MKLDNN and I would like to highlight threadsafety a bit more in our reviews

These are output arrays. If the dependency works correctly in the execution engine, no other threads will read or modify them when this function runs. InvalidateMKLDNNData has a side effect. so is writing data to the output arrays.

I guess you refer to the bug I reported in my other PR. As I said, the race condition isn't MKLDNN-specific. It's just MKLDNN that makes it noticeable. A lot of our tests just run the computation and don't verify the correctness of the data.

marcoabreu

LGTM, but please assess the thread safety before merging

ashokei · 2018-05-08T23:00:29Z

@marcoabreu we now have CI running though these test with -- build change you suggested above.

zheng-da · 2018-05-09T15:14:41Z

Could you please change the name of the tests? Thanks

ashokei · 2018-05-10T18:54:17Z

@zheng-da updated PR to use testname, @marcoabreu can you merge now, thanks.

* handle inplace in mkldnn FallBackCompute * add comments * handle kAddTo in mkldnn FallBackCompute * add PR feedback * add unittest for mkldnn inplace sum with cpu data * add back mkldnn engine threading unittest * separate mkldnn install test and fix pylint issue * remove --build from mkldnn jenkins test * update mkldnn unittests * update comments for mkldnn test * remove python test doc string so unittest name is used

zheng-da reviewed Apr 18, 2018

View reviewed changes

ashokei force-pushed the mkldnn_fallback_inplace_bug branch from ba9de4d to 4a25536 Compare April 19, 2018 19:31

zheng-da reviewed Apr 20, 2018

View reviewed changes

ashokei force-pushed the mkldnn_fallback_inplace_bug branch from 4a25536 to 3c792b9 Compare April 25, 2018 20:19

ashokei force-pushed the mkldnn_fallback_inplace_bug branch from 3c792b9 to 7a4c26d Compare April 27, 2018 03:36

zheng-da approved these changes Apr 27, 2018

View reviewed changes

ashokei changed the title ~~handle inplace in mkldnn FallBackCompute~~ [MXNET-365] handle inplace in mkldnn FallBackCompute Apr 27, 2018

ashokei force-pushed the mkldnn_fallback_inplace_bug branch 4 times, most recently from 7d16a4b to 8f8b792 Compare May 1, 2018 22:59

ashokei mentioned this pull request May 1, 2018

Add more gluon computation test on MKLDNN with memory operation(s)(slice, reshape etc.) #10764

Merged

5 tasks

ashokei force-pushed the mkldnn_fallback_inplace_bug branch from 8f8b792 to 0a8ce05 Compare May 2, 2018 18:22

zheng-da approved these changes May 3, 2018

View reviewed changes

marcoabreu suggested changes May 5, 2018

View reviewed changes

ashokei force-pushed the mkldnn_fallback_inplace_bug branch 3 times, most recently from 36f0367 to fdc17d4 Compare May 7, 2018 18:42

zheng-da reviewed May 8, 2018

View reviewed changes

ashokei force-pushed the mkldnn_fallback_inplace_bug branch 2 times, most recently from 2511534 to bd9d4e1 Compare May 8, 2018 17:07

ashokei force-pushed the mkldnn_fallback_inplace_bug branch from bd9d4e1 to e2ac42f Compare May 8, 2018 19:13

marcoabreu reviewed May 8, 2018

View reviewed changes

marcoabreu approved these changes May 8, 2018

View reviewed changes

ashokei force-pushed the mkldnn_fallback_inplace_bug branch from e2ac42f to 4b19536 Compare May 9, 2018 20:08

ashokei added 11 commits May 10, 2018 09:31

handle inplace in mkldnn FallBackCompute

a865e8c

add comments

a0bfb73

handle kAddTo in mkldnn FallBackCompute

73e4ae7

add PR feedback

4c019ac

add unittest for mkldnn inplace sum with cpu data

4e5c5f9

add back mkldnn engine threading unittest

a40a64f

separate mkldnn install test and fix pylint issue

fbfea09

remove --build from mkldnn jenkins test

d023308

update mkldnn unittests

c7e2ea5

update comments for mkldnn test

4f06e10

remove python test doc string so unittest name is used

df08174

ashokei force-pushed the mkldnn_fallback_inplace_bug branch from 4b19536 to df08174 Compare May 10, 2018 16:31

marcoabreu merged commit 5858a4f into apache:master May 11, 2018

anirudh2290 mentioned this pull request Jun 8, 2018

cherry-pick bug fixes in MKLDNN for v1.2.0 #11212

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MXNET-365] handle inplace in mkldnn FallBackCompute #10591

[MXNET-365] handle inplace in mkldnn FallBackCompute #10591

ashokei commented Apr 18, 2018 •

edited

Loading

zheng-da Apr 18, 2018

zheng-da Apr 20, 2018

ashokei commented Apr 26, 2018

zheng-da commented Apr 26, 2018

ashokei commented Apr 27, 2018

ashokei commented May 2, 2018

ashokei commented May 4, 2018

ashokei commented May 5, 2018

marcoabreu left a comment

zheng-da commented May 5, 2018

marcoabreu commented May 5, 2018

ashokei commented May 8, 2018

zheng-da May 8, 2018

ashokei May 8, 2018

ashokei commented May 8, 2018

marcoabreu commented May 8, 2018

marcoabreu May 8, 2018

ashokei May 8, 2018

marcoabreu May 9, 2018

zheng-da May 9, 2018

marcoabreu left a comment

ashokei commented May 8, 2018

zheng-da commented May 9, 2018

ashokei commented May 10, 2018

[MXNET-365] handle inplace in mkldnn FallBackCompute #10591

[MXNET-365] handle inplace in mkldnn FallBackCompute #10591

Conversation

ashokei commented Apr 18, 2018 • edited Loading

Description

Checklist

Essentials

Changes

Comments

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashokei commented Apr 26, 2018

zheng-da commented Apr 26, 2018

ashokei commented Apr 27, 2018

ashokei commented May 2, 2018

ashokei commented May 4, 2018

ashokei commented May 5, 2018

marcoabreu left a comment

Choose a reason for hiding this comment

zheng-da commented May 5, 2018

marcoabreu commented May 5, 2018

ashokei commented May 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashokei commented May 8, 2018

marcoabreu commented May 8, 2018

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

marcoabreu left a comment

Choose a reason for hiding this comment

ashokei commented May 8, 2018

zheng-da commented May 9, 2018

ashokei commented May 10, 2018

ashokei commented Apr 18, 2018 •

edited

Loading