Multithreaded Inference Support

[anirudh2290]

Description

To get some background on the PR, please go through the related github issue : #16431 . Below, I have added a brief overview for the PR.

Brief Overview

1. This PR adds a multithreaded inference interface (EXPERIMENTAL) for MXNet. The approach it takes for this is to add a thread safe version of cached op with minimal set of features required for inference.
2. Adds C API to create cached op thread safe version: The API: CreateCachedOpEX takes an additional parameter thread_safe. When thread_safe is set to true, invokes on the cached op handle will invoke the thread safe version of cached op.

  /*!
   * \brief create cached operator, allows to choose thread_safe version
   * of cachedop
   */
  MXNET_DLL int MXCreateCachedOpEX(SymbolHandle handle,
                                   int num_flags,
                                   const char** keys,
                                   const char** vals,
                                   CachedOpHandle *out,
                                   bool thread_safe DEFAULT(false));

3. This PR adds tests and verification for thread safety of engine, for running parallel inference with the thread safe version of CachedOp with different models and different number of threads and different number of inferences per thread.
4. Model Testing has been limited to : resnet18, resnet50_v1, resnet152_v1
5. Adds CI stages and CI tests for running the thread safety tests. Please see : capi-cpp-package GPU in unix-gpu stage.

Feedback/Acknowledgements

Below is the feedback/help received from offline code/design reviews:

1. @nswamy - Early inputs and offline design discussions related to this project, specifically with respect to dmlc::ThreadLocalStore.
2. @frankfliu - Explaining the requirements and use case for MXNet backend from a customer perspective, code review suggestions to have thread safe API for Create only and to determine whether to invoke cached op/cached op threadsafe based on handle.
3. @andreaolgiati - Suggestion to document current threading model of MXNet (still a TODO), requesting adding support for dynamic shapes for NLP case (not supported yet), suggested documenting the long term plan with respect to thread safe inference in mxnet. (Documented in limitations and Future work section of the doc, will add required github issues).
4. @lanking520 , @larroy , @zachgk - Suggestion to add a new API for Threadsafe cached op version, since modifying existing one will break backward compatibility.
5. @frankfliu and @oorqueda - Suggestion for continuous maintenance (addition of support) of the models supported table. Will be maintained on the doc and RFC issue.
6. @samskalicky - offline code review suggestions with respect to code comments for locking strategy and comments for changes made for thread safe cached op.
7. @Vikas-kum - better document the performance benefits. [still a TODO]

Thanks to all above for the suggestions, review and feedback.

Thanks to @marcoabreu and @arcadiaphy for the valuable comments on the RFC.

Additionally, thanks to @roywei, @access2rohit , @stu1130 , @ChaiBapchya , @rondogency , @yuxihu, @vrakesh, @keerthanvasist, @josephevans for code/design review.

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

• 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 https://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

[anirudh2290]

On the other hand, I am not sure if we can accept this cached op without bulking and subgraph support. Subgraph is used for multiple accelerators for MXNet inference. Also, bulking is essential to inference speed on GPUs. For instance, we saw 20% perf regression if bulking scope is smaller than the optimal scope #9055 and it would be even more significant if no bulking is enabled at all. I am not sure if this can be shipped with these unknown limitations. Thinking about how to better support thread safe bulking and subgraph may affect the implementation and design.

1. I have added bulking support.

2. For subgraphing, If I am correct, MXNet still doesnt support subgraph API with gluon. It is still supported only with symbol and MXNET_SUBGRAPH_BACKEND env variable works only with graph executor. What I meant to say is that the param subgraph in CachedOp is not supported. Now you can use a symbol which has already been converted and replaced with subgraph ops with the cached_op_threadsafe version. I have added a test to demonstrate that. I think this should address your concern with respect to subgraphing.

Do we have evidence that the newly introduced cached op is performant? I'm concerned about the mutex for the whole forward function - that means no thread can concurrently push operations to the engine. What I observed is - that could take a long time depending on the model used.

Although the mutex in forward is just pushing the ops, which may have delays between threads, the execution of these ops will be scheduled in parallel, especially when there is no dependency from the ops different threads and should improve performance.

https://github.com/awslabs/djl did some benchmarks and were able to see improved throughput upto 2x with naive engine. I was also able to see performance improvements upto 1.7x when you increase the number of worker threads on the threaded engine for cpu.

[anirudh2290]

I have addressed all the comments here. Please let me know if you have additional comments.

[eric-haibin-lin]

@TaoLv @PatricZhao could you help review this PR, since it's related to inference?

[pengzhao-intel]

We're on the long vocation and will back to office in 10th Feb.

[cjolivier01]

Thanks so much for doing this! Badly needed feature!

[anirudh2290]

Thanks a lot for your kind words Chris! Appreciate your dedication towards the mxnet community!

[cjolivier01]

hahaha...

…

On Thu, Jan 30, 2020 at 8:41 PM Anirudh Subramanian < ***@***.***> wrote: Thanks a lot for your kind words Chris! Appreciate your dedication towards the mxnet community! — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#16654>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACVWZ7JKKZD7RO4IKJUZHFDRAOTZBANCNFSM4JFW2Z2Q> .

[larroy]

Did you run performance benchmarks to see that there's no regression? as I understand this is a change that could have an impact on performance in the engine. Would be useful to run the usual models we have in the performance dashboards.

[leezu]

@anirudh2290 this seems to have broken C++ examples. See #17514

[eric-haibin-lin]

@zhreshold FYI