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

[MXNET-703] Minor refactor of TensorRT code #13311

Merged
merged 1 commit into from
Jan 26, 2019
Merged

[MXNET-703] Minor refactor of TensorRT code #13311

merged 1 commit into from
Jan 26, 2019
+14 −15

Conversation

KellenSunderland
Copy link
Contributor

Description

[MXNET-703] Minor refactor of TensorRT code

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

@KellenSunderland KellenSunderland force-pushed the trt_cleanup branch 2 times, most recently from 3162e57 to b422b7d Compare November 17, 2018 18:52
@kalyc
Copy link
Contributor

kalyc commented Nov 19, 2018

@mxnet-label-bot add [pr-awaiting-review]

@marcoabreu marcoabreu added the pr-awaiting-review PR is waiting for code review label Nov 19, 2018
@vandanavk
Copy link
Contributor

@larroy @lebeg @marcoabreu for review

larroy
larroy reviewed Nov 29, 2018
View reviewed changes
src/executor/trt_graph_executor.cc
@@ -434,7 +435,7 @@ Executor *TrtGraphExecutor::TensorRTBind(nnvm::Symbol symbol,
std::unordered_map<std::string, NDArray> *shared_buffer,
Executor *shared_exec) {
auto exec = new exec::TrtGraphExecutor();
exec->Init(symbol, default_ctx, group2ctx,
exec->Init(std::move(symbol), default_ctx, group2ctx,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is Init accepting an rvalue reference?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a good question I wanted to dive into a little. My understanding is that the Init function can bind its variable to either an lvalue or an rvalue when passing by value. You don't have to create an overload or anything to determine which one it expects. Here's an example:

https://gist.github.com/KellenSunderland/d431119580a116410c672b9535d85170
Which is using code borrowed from:
https://www.chromium.org/rvalue-references?tmpl=%2Fsystem%2Fapp%2Ftemplates%2Fprint%2F&showPrintDialog=1

Copy link
Contributor

Choose a reason for hiding this comment

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

Well the question if the move is doing anything, because I don't know if there's an Init signature accepting an rvalue reference to a symbol, that was the original question.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I was also curious about this. The example shows a similar pattern and it does save a copy.

@roywei
Copy link
Member

roywei commented Dec 11, 2018

@KellenSunderland thanks for your contribution, ping for update and fix CI failure.

larroy
larroy approved these changes Dec 11, 2018
View reviewed changes
Copy link
Contributor

@larroy larroy left a comment

Choose a reason for hiding this comment

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

LGTM, just had a minor comment about rvalue reference.

@sandeep-krishnamurthy
Copy link
Contributor

@KellenSunderland - Can you please rebase?

@anirudhacharya
Copy link
Member

@KellenSunderland can you please rebase and resolve the merge conflicts?

@KellenSunderland KellenSunderland force-pushed the trt_cleanup branch 4 times, most recently from 32dd73f to 46f5193 Compare January 12, 2019 08:09
@KellenSunderland
Copy link
Contributor Author

I've rebased and I believe most comments are closed out. Could you have a quick look and see if anything else is needed @anirudh2290 or @sandeep-krishnamurthy.

larroy
larroy approved these changes Jan 15, 2019
View reviewed changes
Copy link
Contributor

@larroy larroy left a comment

Choose a reason for hiding this comment

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

LGTM

@KellenSunderland
Copy link
Contributor Author

Rebased and fixed the last nit about static_cast. I believe this can be merged if a committer could do a quick review.

@KellenSunderland KellenSunderland mentioned this pull request Jan 15, 2019
Merged
5 tasks
sandeep-krishnamurthy
sandeep-krishnamurthy approved these changes Jan 26, 2019
View reviewed changes
Copy link
Contributor

@sandeep-krishnamurthy sandeep-krishnamurthy left a comment

Choose a reason for hiding this comment

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

Thanks looks good to me.

@sandeep-krishnamurthy sandeep-krishnamurthy merged commit 3df0917 into apache:master Jan 26, 2019
jessr92 pushed a commit to jessr92/incubator-mxnet that referenced this pull request Jan 27, 2019
@KellenSunderland
[MXNET-703] Minor refactor of TensorRT code (apache#13311)
b378641
jessr92 pushed a commit to jessr92/incubator-mxnet that referenced this pull request Jan 27, 2019
@KellenSunderland
[MXNET-703] Minor refactor of TensorRT code (apache#13311)
b8f08e1
stephenrawls pushed a commit to stephenrawls/incubator-mxnet that referenced this pull request Feb 16, 2019
@KellenSunderland @stephenrawls
[MXNET-703] Minor refactor of TensorRT code (apache#13311)
571b65e
haohuanw pushed a commit to haohuanw/incubator-mxnet that referenced this pull request Jun 23, 2019
@KellenSunderland
[MXNET-703] Minor refactor of TensorRT code (apache#13311)
bf83e02
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@anirudh2290 anirudh2290 anirudh2290 left review comments

@sandeep-krishnamurthy sandeep-krishnamurthy sandeep-krishnamurthy approved these changes

@larroy larroy larroy approved these changes

Assignees
No one assigned
Labels
pr-awaiting-review PR is waiting for code review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@KellenSunderland @kalyc @vandanavk @roywei @sandeep-krishnamurthy @anirudhacharya @larroy @anirudh2290 @marcoabreu