PaddlePaddle memory leak test=develop

[pawelpiotrowicz]

@luotao1 @kbinias

[luotao1]

What's the backgroud of this PR? Could you give the memory leak case?

[pawelpiotrowicz]

@luotao1 In short, fix solves the issue below:

#11 0x48c329b in paddle::framework::details::OpInfoFiller<paddle::operators::Conv2DOpMaker, (paddle::framework::details::OpInfoFillType)1>::operator()(char const*, paddle::framework::OpInfo*) const /home/pawepiot/workspace/multi_instance_public/paddle-public/paddle/fluid/framework/details/op_registry.h:167
#12 0x48beb41 in paddle::framework::details::OperatorRegistrarRecursive<1ul, false, paddle::operators::ConvOp, paddle::operators::Conv2DOpMaker, paddle::operators::ConvOpInferVarType, paddle::operators::Conv2DGradMaker>::OperatorRegistrarRecursive(char const*, paddle::framework::OpInfo*) /home/pawepiot/workspace/multi_instance_public/paddle-public/paddle/fluid/framework/details/op_registry.h:136
#13 0x48bdb46 in paddle::framework::details::OperatorRegistrarRecursive<0ul, false, paddle::operators::ConvOp, paddle::operators::Conv2DOpMaker, paddle::operators::ConvOpInferVarType, paddle::operators::Conv2DGradMaker>::OperatorRegistrarRecursive(char const*, paddle::framework::OpInfo*) /home/pawepiot/workspace/multi_instance_public/paddle-public/paddle/fluid/framework/details/op_registry.h:139
#14 0x48bc225 in paddle::framework::OperatorRegistrar<paddle::operators::ConvOp, paddle::operators::Conv2DOpMaker, paddle::operators::ConvOpInferVarType, paddle::operators::Conv2DGradMaker>::OperatorRegistrar(char const*) /home/pawepiot/workspace/multi_instance_public/paddle-public/paddle/fluid/framework/op_registry.h:61
#15 0x48b8bd1 in __static_initialization_and_destruction_0 /home/pawepiot/workspace/multi_instance_public/paddle-public/paddle/fluid/operators/conv_op.cc:618
#16 0x48b8cf2 in _GLOBAL__sub_I_conv_op.cc /home/pawepiot/workspace/multi_instance_public/paddle-public/paddle/fluid/operators/conv_op.cc:652
#17 0x5bae6bc in __libc_csu_init (/home/pawepiot/workspace/multi_instance_public/paddle-public/build/paddle/fluid/inference/tests/api/test_analyzer_image_classification+0x5bae6bc)

SUMMARY: AddressSanitizer: 209193 byte(s) leaked in 8511 allocation(s).

[pawelpiotrowicz]

FYI : @jczaja @LeoZhao-Intel

[kbinias]

LGTM. Please add only more details about reproduction of the problem.

[pawelpiotrowicz]

The issue comes from.

${workspaceFolder}/build/paddle/fluid/inference/tests/api/test_analyzer_image_classification --infer_model=${workspaceFolder}/build/third_party/inference_demo/googlenet/model --gtest_filter=Analyzer_resnet50.profile --use_ngraph --use_analysis=false --repeat=100 --paddle_num_threads=4 --num_threads=2

• APPLY SANATIZER

[luotao1]

test_analyzer_image_classification do not have use_ngraph in develop branch. Do you change the related unit-test?

[luotao1]

Could you add more description of this memory-leak?

• for multi-instance only or for both multi-instance and one-instance?
• for use_ngraph=true only or for both --use_ngraph=false and --use_ngraph=true?
• for --use_analysis=false only or for both --use_analysis=false and --use_analysis=true?

#18382 (comment), if you change the unit-tests, please paste the patch here. We will have a double check.

[LeoZhao-Intel]

@pawelpiotrowicz my understanding is you find this memory leak with applying SANATIZER, can you give more details or general description on why/what this leak happens, it will help us to understand what's real issue.

For op_info, there is no such code to delete pointer ? so you use shared_ptr to keep it released at appropriate time for all those places, right?

[kbinias]

Answers

for multi-instance only or for both multi-instance and one-instance?

only for one instance

for use_ngraph=true only or for both --use_ngraph=false and --use_ngraph=true?

both, --with use_ngraph=false and --use_ngraph=true

for --use_analysis=false only or for both --use_analysis=false and --use_analysis=true?

both, with --use_analysis=false and --use_analysis=true

For op_info, there is no such code to delete pointer ? so you use shared_ptr to keep it released at appropriate time for all those places, right?

Yes, destructor doesn't exist.

To reproduce these memory leak

1. Merge the latest develop branch with PR [PROPOSAL] Add support for dynamic code analysis (Sanitizers) #18303 e.g. git merge kbinias/add-sanitizers
2. Build PaddlePaddle with AddressSanitizer: cmake .. -DCMAKE_BUILD_TYPE=Debug -DWITH_TESTING=ON -DWITH_INFERENCE_API_TEST=ON -DON_INFER=ON -DWITH_PYTHON=ON -DWITH_NGRAPH=ON -DWITH_GPU=OFF -DSANITIZER_TYPE="Address"
3. Run cmd: ./paddle/fluid/inference/tests/api/test_analyzer_image_classification --infer_model=./third_party/inference_demo/googlenet/model --gtest_filter=Analyzer_resnet50.profile --use_analysis=false --repeat=100 --paddle_num_threads=4 --num_threads=1

Important after applying the current PR, there will still be 2 errors that you can eliminate with PR #18622

[bingyanghuang]

Answers

for multi-instance only or for both multi-instance and one-instance?

only for one instance

for use_ngraph=true only or for both --use_ngraph=false and --use_ngraph=true?

both, --with use_ngraph=false and --use_ngraph=true

for --use_analysis=false only or for both --use_analysis=false and --use_analysis=true?

both, with --use_analysis=false and --use_analysis=true

For op_info, there is no such code to delete pointer ? so you use shared_ptr to keep it released at appropriate time for all those places, right?

Yes, destructor doesn't exist.

To reproduce these memory leak

1. Merge the latest develop branch with PR [PROPOSAL] Add support for dynamic code analysis (Sanitizers) #18303 e.g. git merge kbinias/add-sanitizers
2. Build PaddlePaddle with AddressSanitizer: cmake .. -DCMAKE_BUILD_TYPE=Debug -DWITH_TESTING=ON -DWITH_INFERENCE_API_TEST=ON -DON_INFER=ON -DWITH_PYTHON=ON -DWITH_NGRAPH=ON -DWITH_GPU=OFF -DSANITIZER_TYPE="Address"
3. Run cmd: ./paddle/fluid/inference/tests/api/test_analyzer_image_classification --infer_model=./third_party/inference_demo/googlenet/model --gtest_filter=Analyzer_resnet50.profile --use_analysis=false --repeat=100 --paddle_num_threads=4 --num_threads=1

Important after applying the current PR, there will still be 2 errors that you can eliminate with PR #18622

@luotao1 Here are the reproducible test case for sanitizers.

[LeoZhao-Intel]

LGTM

[luotao1]

@zhupengyang Could you help reproduce the problem and verify whether PR fix the error?

• how to reproduce: see PaddlePaddle memory leak test=develop #18382 (comment)
• the error result: see PaddlePaddle memory leak test=develop #18382 (comment)

[zhupengyang]

@zhupengyang Could you help reproduce the problem and verify whether PR fix the error?

• how to reproduce: see #18382 (comment)
• the error result: see #18382 (comment)

• reproduce the problem:

• verify PaddlePaddle memory leak test=develop #18382 and Fix memory leak in test #18622
  commit id: 2de7f41 6fbdda2 add1984 c6f2858
  result: there is still one error

@luotao1

[luotao1]

Thanks for @zhupengyang double-check work!
It seems the error reported in this PR is fixed, but still have some other errors. @kbinias @LeoZhao-Intel @pawelpiotrowicz

[chengduoZH]

@pawelpiotrowicz Thanks for your work.
I am curious that does this really leed to memory leak? Because OpInfoFiller is called only when registering the operators, and those actions only happen only once for each operator during the training or testing.

So that I don't think we should use smart point here.

[pawelpiotrowicz]

@chengduoZH @luotao1 In my opinion issue definitely must be solved, in future if we got advanced issue we will play with the noises from this issue, so the perspective to find real reason of error might be more complex for developer. If we can fix any warning/issue we should do this asap to make process pure. Smart pointer should be used here to prevent a way when use case will be changed.

[chengduoZH]

@pawelpiotrowicz Thanks for your reply, if you want to fix this warning, I prefer to add destructive function in OpInfo but not use smart point here.

[pawelpiotrowicz]

@chengduoZH

I prefer to add destructive function in OpInfo

Why? Do we have any reason to do this?
I don't see any advantages to applying this approach, moreover, the solution provides a way for making mistake in future. If a smart pointer can be used should be used - good practice.

[LeoZhao-Intel]

@chengduoZH

I prefer to add destructive function in OpInfo

Why? Do we have any reason to do this?
I don't see any advantages to applying this approach, moreover, the solution provides a way for making mistake in future. If a smart pointer can be used should be used - good practice.

Since paddlepaddle is cross-platform framework, there was a concern that some compilers on specific platform may not support smart pointer well. That's one of the reason to use deconstruction I guess.

[luotao1]

there was a concern that some compilers on specific platform may not support smart pointer well.

Yes, it is. We may support some other language interface later.

[bingyanghuang]

@pawelpiotrowicz Hi, pawel Do you still want to merge this PR? If yes, please update your PR by following Baidu's comments. If not, please close this PR. @kbinias

[pawelpiotrowicz]

close

[paddle-bot-old]

Since you haven't replied for more than a year, we have closed this issue/pr.
If the problem is not solved or there is a follow-up one, please reopen it at any time and we will continue to follow up.
由于您超过一年未回复，我们将关闭这个issue/pr。
若问题未解决或有后续问题，请随时重新打开，我们会继续跟进。