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

Fix memory leak in singleton_object_pool #835

Merged
merged 3 commits into from Sep 8, 2020

Conversation

mxz297
Copy link
Member

@mxz297 mxz297 commented Sep 5, 2020

Two memory leak fixes related singleton_object_pool.

  1. The singleon_object_pool currently only destructs objects but does not deallocate memory space. This leak applies to all architectures.

  2. On ppc and arm, instruction decoder does not use smart pointers to manage InstructionAPI::Instruction constructed by a singleon_object_pool, and thus cause memory leaks.

@mxz297
Copy link
Member Author

mxz297 commented Sep 5, 2020

This PR should help with memory leaks caused by InstructionAPI and dataflowAPI. It does not help SymtabAPI.

My experience is that memory usage of code parsing for a binary with 170MB .text size decreased from 12.0GB to 9.3GB on x86. The impact on power or arm can be larger. I have a run that rans out of memory on zeroah, which has 64GB memory, and now it can finish with about using 7GB memory.

@mxz297
Copy link
Member Author

mxz297 commented Sep 7, 2020

Instrumenting all functions libxul.so used to need about 64GB memory. Now it needs about 48GB. Still a lot of memory is needed, but a decent reduction.

@hainest hainest added bug dataflowAPI This issue is directly related to dataflowAPI instructionAPI This issue is directly related to instructionAPI labels Sep 8, 2020
@hainest hainest added this to the Release 10.3 milestone Sep 8, 2020
@hainest
Copy link
Contributor

hainest commented Sep 8, 2020

https://bottle.cs.wisc.edu/search?dyninst_branch=PR835

Testing looks good. You can ignore the regressions; they are unrelated non-deterministic test_reloc failures. I agree we should split this PR into allocator changes and decoder changes. I can do that, if you don't have time.

@mxz297
Copy link
Member Author

mxz297 commented Sep 8, 2020

@hainest This PR contains changes both to the allocator and the decoder, but the changes are only for fixing memory leak. Just want to clarify that I think the improvement to the allocator should be done in a separate PR. I don't think we need to split this PR to two parts. So, if there is no regression, I am good to merge this.

@hainest
Copy link
Contributor

hainest commented Sep 8, 2020

@mxz297 I would prefer to split this one since the change to the instruction decoders is an API breaker, but the change to the allocator isn't. I can split these so you don't have to.

@mxz297
Copy link
Member Author

mxz297 commented Sep 8, 2020

@mxz297 I would prefer to split this one since the change to the instruction decoders is an API breaker, but the change to the allocator isn't. I can split these so you don't have to.

No, my changes to the decoders are internal. Note that the changed header files are internal header files. These are not public header files. So, this PR is not breaking API.

@hainest
Copy link
Contributor

hainest commented Sep 8, 2020

Oh, I didn't see that. Agreed. Merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug dataflowAPI This issue is directly related to dataflowAPI instructionAPI This issue is directly related to instructionAPI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants