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

[BUGFIX] Free up executor memory when reassigning new executor #19214

Merged
merged 1 commit into from
Sep 25, 2020

Conversation

access2rohit
Copy link
Contributor

Description

Fixes memory leak caused due to not freeing up of graph executor object.

Checklist

Essentials

  • PR's title starts with a category (e.g. [BUGFIX], [MODEL], [TUTORIAL], [FEATURE], [DOC], etc)
  • Changes are complete (i.e. I finished coding on this PR)
  • All changes have test coverage
  • Code is well-documented

@mxnet-bot
Copy link

Hey @access2rohit , 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-cpu, centos-gpu, website, windows-gpu, sanity, unix-cpu, edge, centos-cpu, unix-gpu, miscellaneous]


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.

@mseth10
Copy link
Contributor

mseth10 commented Sep 24, 2020

Looks like we only instantiate the object the first time because we want to check whether subgraph property is set, which has nothing to do with the executor really. We can define a variable like this and use it in the condition: https://github.com/apache/incubator-mxnet/blob/07e65e5cf3c629c78f206798f33a3529e95c0fc4/src/executor/graph_executor.cc#L58
This will simplify the code and then we only need to instantiate object once after the if block. WDYT? @access2rohit

@access2rohit
Copy link
Contributor Author

access2rohit commented Sep 24, 2020

@mseth10 Can you review again ?

if (!exec->subgraph_property().empty()) {
const auto& backend_name = exec->subgraph_property();
const auto& backend_name = dmlc::GetEnv("MXNET_SUBGRAPH_BACKEND", exec::GetDefaultSubgraphBackend());
if (!backend_name.empty()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

we should also include the check backend_name == "None" or we can do something like this before the condition https://github.com/apache/incubator-mxnet/blob/3b0b9dc9d865ee0b1456e8f400f6f4a25c77b5db/src/executor/graph_executor.cc#L59-L60

Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer we dont duplicate code/functionality if at all possible. Since we create exec with new, and then use it to get the backend_name, why dont we just call delete on exec before line 2080 when creating a new executor from the partitioned symbol?

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah I agree, we can just use delete exec for both functions

@mseth10
Copy link
Contributor

mseth10 commented Sep 24, 2020

We also need to make this change for the SimpleBind API, it is implemented in a similar fashion https://github.com/apache/incubator-mxnet/blob/3b0b9dc9d865ee0b1456e8f400f6f4a25c77b5db/src/executor/graph_executor.cc#L2009

Copy link
Contributor

@mseth10 mseth10 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@samskalicky samskalicky left a comment

Choose a reason for hiding this comment

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

Thanks for the delete change, this looks very succinct!

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.

4 participants