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

[MXNET-1417][Performance] Caching Dynamic Shape Checking Result #15262

Merged
merged 2 commits into from
Jun 18, 2019

Conversation

junrushao
Copy link
Member

@junrushao junrushao commented Jun 17, 2019

Description

(Please see appendix for experiment details)

PR #14192 that enables dynamic shapes slows down a model that originally runs in 235.65 ms by 7.26 ms (to 242.91 ms).

Also noted that a seemingly relevant PR #14665 suggesting itself to be improving "[performance]", does not change performance number in any means - It still runs in 242.35 ms.

This PR fixes this by caching the checking result of whether dynamic shape exists. The mechanism itself is quick simple: if the dynamic shape existence has been checked, let's simply don't do it again, because the graph does not change.

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

  • Added a new flag dynamic_shape_checked in CachedOp, which indicates whether dynamic shape existence has been checked.

Comments

Experiment environment: EC2 p2.8xlarge, CUDA 10 and cuDNN 7.5. The model itself is confidential.

The detailed benchmark is as below (mean ± stdev). The experiment is conducted in 20 runs, warmup run is excluded.

  1. On commit 39412b3 (right before PR [MXNET-1324] Add NaiveRunGraph to imperative utils #14192 is merge):
    Hybridize w/ static_alloc: 235.65 ± 0.22246 ms

  2. On commit 83d2c2d (where PR [MXNET-1324] Add NaiveRunGraph to imperative utils #14192 is merged):
    Hybridize w/ static_alloc: 242.91 ms ± 0.71125 ms

  3. PR [performance] Avoid uneccesary vector copies in imperative_utils.cc #14665 patched to commit 83d2c2d
    Hybridize w/ static_alloc: 242.35 ± 0.25124 ms

  4. After this patch applied to commit 83d2c2d
    Hybridize w/ static_alloc: 234.95 ± 0.39334 ms

CC: @szha @zheng-da please review :-)

@zheng-da
Copy link
Contributor

please add more description of the PR.

@junrushao junrushao changed the title [DO NOT REVIEW] Test [Performance] Caching Dynamic Shape Checking Result Jun 18, 2019
@junrushao junrushao changed the title [Performance] Caching Dynamic Shape Checking Result [MXNET-1417][Performance] Caching Dynamic Shape Checking Result Jun 18, 2019
@zheng-da zheng-da merged commit d7e2139 into apache:master Jun 18, 2019
@larroy
Copy link
Contributor

larroy commented Jun 18, 2019

@junrushao1994 would have been nice that you tagged me for review as well, since I was looking into this issue. The PR looks good to me anyways.

@larroy
Copy link
Contributor

larroy commented Jun 18, 2019

So the root cause of the degradation was the additional shape inferencing passes?

@junrushao
Copy link
Member Author

@larroy Sure! Will tag you next time

@larroy
Copy link
Contributor

larroy commented Jun 18, 2019

thanks for fixing this, and it's cool that your orginal commit enables dynamic shape.

@junrushao
Copy link
Member Author

So the root cause of the degradation was the additional shape inferencing passes?

Yep, the logic after these line is somewhat bulky.

haohuanw pushed a commit to haohuanw/incubator-mxnet that referenced this pull request Jun 23, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants