Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add the call of DropKids at the end of executor.Run to delete the local scopes created in operators #10403

Merged
merged 3 commits into from
May 7, 2018

Conversation

Xreki
Copy link
Contributor

@Xreki Xreki commented May 4, 2018

No description provided.

@Xreki Xreki added the 预测 原名Inference,包含Capi预测问题等 label May 4, 2018
@Xreki Xreki requested a review from jacquesqiao May 4, 2018 09:29
@Xreki Xreki changed the title Add the call of DeleteScope to delete the memory of scope created by NewScope Add the call of DropKids at the end of executor.Run to delete the local scopes created in operators May 4, 2018
if (create_vars && create_local_scope) {
scope->DeleteScope(local_scope);
} else {
// Delete the local scopes created in operators.
scope->DropKids();
Copy link
Member

Choose a reason for hiding this comment

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

if create_vars is False and create_local_scope is True, do we need to use

local_scope->DropKids()

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The local_scope itself is deleted by scope->DeleteScope(local_scope);, all the kids will be deleted, see the implementation of the destructor:

Scope::~Scope() {
DropKids();
for (auto& kv : vars_) {
VLOG(3) << "Destroy variable " << kv.first;
delete kv.second;
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @Xreki , maybe we can't delete the kids in the execute function, some conditions like dist training, pserver will execute the optimize blocks on the same scope instead of a new scope for each optimizes action.

related issued #10625

Copy link
Member

@jacquesqiao jacquesqiao left a comment

Choose a reason for hiding this comment

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

LGTM!

@Xreki Xreki merged commit fd1971c into PaddlePaddle:develop May 7, 2018
@Xreki Xreki deleted the core_fix_delete_scope branch November 14, 2018 02:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
预测 原名Inference,包含Capi预测问题等
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants