Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

Improve log_softmax op performance by using DNNL support #18320

Merged
merged 4 commits into from
Jun 1, 2020
Merged

Improve log_softmax op performance by using DNNL support #18320

merged 4 commits into from
Jun 1, 2020

Conversation

bgawrych
Copy link
Contributor

Description

Improve log_softmax op performance by using DNNL support

Native implementation:

shape time (ms/iter)
(5, 512, 512) 5.3858582973480225
(5, 512, 1536) 18.289981842041016
(5, 512, 2048) 23.175915956497192
(5, 2048, 512) 24.09080457687378
(4, 512, 512) 4.271230220794678

MKLDNN implementation:

shape time (ms/iter)
(5, 512, 512) 1.0982356071472168
(5, 512, 1536) 3.9154343605041504
(5, 512, 2048) 5.493232250213623
(5, 2048, 512) 6.6350884437561035
(4, 512, 512) 0.8532240390777588

Checklist

Essentials

  • Changes are complete (i.e. I finished coding on this PR)
  • To the best of my knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

Comments

  • Tests have increased tolerance - the reason of this is that DNNL sometimes have significantly different value of single element in array

@mxnet-bot
Copy link

Hey @bgawrych , Thanks for submitting the PR
All tests are already queued to run once. If tests fail, you can trigger one or more tests again with the following commands:

  • To trigger all jobs: @mxnet-bot run ci [all]
  • To trigger specific jobs: @mxnet-bot run ci [job1, job2]

CI supported jobs: [clang, windows-gpu, miscellaneous, unix-gpu, website, unix-cpu, centos-gpu, edge, windows-cpu, sanity, centos-cpu]

Note:
Only following 3 categories can trigger CI :PR Author, MXNet Committer, Jenkins Admin.
All CI tests must pass before the PR can be merged.

@bgawrych
Copy link
Contributor Author

@mxnet-bot run ci [centos-cpu, unix-cpu]

@mxnet-bot
Copy link

Jenkins CI successfully triggered : [unix-cpu, centos-cpu]

@bgawrych
Copy link
Contributor Author

@TaoLv @pengzhao-intel Can you look at this and ping interested people also?

TaoLv
TaoLv reviewed May 18, 2020
View reviewed changes
src/operator/nn/log_softmax.cc Outdated
}

inline static bool LogSoftmaxStorageType(const nnvm::NodeAttrs& attrs,
const int dev_mask,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix indent.

src/operator/nn/log_softmax.cc Outdated
}

inline static bool LogSoftmaxGradStorageType(const nnvm::NodeAttrs& attrs,
const int dev_mask,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto.

src/operator/nn/log_softmax.cc Outdated
.set_attr<nnvm::FGradient>("FGradient", SoftmaxFGradient{"_backward_log_softmax"})
.set_attr<nnvm::FInferType>("FInferType", SoftmaxOpType)
.set_num_inputs(1)
.set_num_outputs(1)
.set_attr<mxnet::FInferShape>("FInferShape", ElemwiseShape<1, 1>)
.set_attr<mxnet::FInferShape>("FInferShape", SoftmaxOpShape)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please elaborate?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Residue after use_length support

src/operator/nn/mkldnn/mkldnn_log_softmax.cc Outdated
const OpReqType &req,
const NDArray &out_data) {
if (req == kNullOp) return;
// same as the FCompute path, softmax only supports kWriteTo and kWriteInplace for now.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix the comment - should it be log_softmax?

src/operator/nn/mkldnn/mkldnn_log_softmax.cc Outdated
const NDArray &data,
const NDArray &output) {
// MKLDNN does not support temperature argument in their softmax function
// now. Need update this once they start to support it.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicated comments. Also change softmax to log_softmax.

@bgawrych
Copy link
Contributor Author

@mxnet-bot run ci [miscellaneous]

@mxnet-bot
Copy link

Jenkins CI successfully triggered : [miscellaneous]

@pengzhao-intel
Copy link
Contributor

Thanks, @bgawrych thanks for the contribution :)

You can cc Tao and me when you filed a PR so that we can notice reviewing your code in time and please cherry-pick the change to 1.x after the code merge.

@pengzhao-intel
Copy link
Contributor

Please add the new op into our list
https://mxnet.apache.org/versions/1.5.0/tutorials/mkldnn/operator_list.html

@bgawrych
Copy link
Contributor Author

I have tested accuracy of Tree LSTM model to make sure MKLDNN log_softmax
doesn't cause accuracy drop.
Trainings was done on the same dataset and the same seed.

Tree LSTM model accuracy with MKLDNN log_softmax

epoch pearsonr mse
0 0,097245 1,096977
1 0,100926 1,051807
2 0,124511 1,039598
3 0,104311 1,040804
4 0,095321 1,04387
5 0,126679 1,040978
6 0,108294 1,043001
7 0,068355 1,048689
8 0,11179 1,04342
9 0,089656 1,044691

Tree LSTM model accuracy with native log_softmax

epoch pearsonr mse
0 0,097245 1,096978
1 0,100925 1,051807
2 0,124513 1,039598
3 0,104312 1,040804
4 0,095321 1,04387
5 0,126685 1,040978
6 0,108294 1,043001
7 0,068355 1,048689
8 0,111792 1,04342
9 0,089656 1,044691

There are some slightly differences (bolded), but overall it looks like floating point precision issue.
CC: @TaoLv
@pengzhao-intel

Please add the new op into our list
https://mxnet.apache.org/versions/1.5.0/tutorials/mkldnn/operator_list.html

Is this message to me or someone from amazon? Because I can't find any file responsible for this page

@pengzhao-intel
Copy link
Contributor

pengzhao-intel commented May 27, 2020 •
edited
Loading

@pengzhao-intel

Please add the new op into our list
https://mxnet.apache.org/versions/1.5.0/tutorials/mkldnn/operator_list.html

Is this message to me or someone from amazon? Because I can't find any file responsible for this page

@xinyu-intel could you help point out where to change the doc?

pengzhao-intel
pengzhao-intel approved these changes May 28, 2020
View reviewed changes
Copy link
Contributor

@pengzhao-intel pengzhao-intel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for your contribution.

@bgawrych
Copy link
Contributor Author

bgawrych commented Jun 1, 2020

@pengzhao-intel @TaoLv
Is this change gonna be merged or need more approves?

@pengzhao-intel
Copy link
Contributor

Thanks for the contribution. Merging now.

@pengzhao-intel pengzhao-intel merged commit cbbb864 into apache:master Jun 1, 2020
bgawrych added a commit to bgawrych/incubator-mxnet that referenced this pull request Jun 2, 2020
@bgawrych
[v1.x] Backport of improve log_softmax op performance by using DNNL s…
7a64d65 
…upport (apache#18320)

* Improve log_softmax performance by OneDNN library

* Adapt tests for MKLDNN log_softmax

* Fix lint errors

* Fix indent and comments
@bgawrych bgawrych mentioned this pull request Jun 2, 2020
Merged
ys2843 pushed a commit to ys2843/incubator-mxnet that referenced this pull request Jun 2, 2020
@bgawrych @ys2843
Improve log_softmax op performance by using DNNL support (apache#18320)
e5d4c18 
* Improve log_softmax performance by OneDNN library

* Adapt tests for MKLDNN log_softmax

* Fix lint errors

* Fix indent and comments
pengzhao-intel pushed a commit that referenced this pull request Jun 3, 2020
@bgawrych
[v1.x] Backport of improve log_softmax op performance by using DNNL s…
a64825f 
…upport (#18320) (#18469)

* Improve log_softmax performance by OneDNN library

* Adapt tests for MKLDNN log_softmax

* Fix lint errors

* Fix indent and comments
@bgawrych bgawrych mentioned this pull request Jun 3, 2020
Closed
yijunc pushed a commit to yijunc/incubator-mxnet that referenced this pull request Jun 9, 2020
@bgawrych @yijunc
Improve log_softmax op performance by using DNNL support (apache#18320)
33fd8e8 
* Improve log_softmax performance by OneDNN library

* Adapt tests for MKLDNN log_softmax

* Fix lint errors

* Fix indent and comments
AntiZpvoh pushed a commit to AntiZpvoh/incubator-mxnet that referenced this pull request Jul 6, 2020
@bgawrych @AntiZpvoh
Improve log_softmax op performance by using DNNL support (apache#18320)
307ab9e 
* Improve log_softmax performance by OneDNN library

* Adapt tests for MKLDNN log_softmax

* Fix lint errors

* Fix indent and comments
ChaiBapchya pushed a commit to ChaiBapchya/mxnet that referenced this pull request Aug 15, 2020
@bgawrych @ChaiBapchya
[v1.x] Backport of improve log_softmax op performance by using DNNL s…
859c71d 
…upport (apache#18320) (apache#18469)

* Improve log_softmax performance by OneDNN library

* Adapt tests for MKLDNN log_softmax

* Fix lint errors

* Fix indent and comments
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@pengzhao-intel pengzhao-intel pengzhao-intel approved these changes

@TaoLv TaoLv TaoLv approved these changes

Assignees
No one assigned
Labels
MKLDNN
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@bgawrych @mxnet-bot @pengzhao-intel @TaoLv