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 Mac Memory leak issue #1042

Merged
merged 3 commits into from
Jul 27, 2022
Merged

Fix Mac Memory leak issue #1042

merged 3 commits into from
Jul 27, 2022

Conversation

shachau
Copy link
Contributor

@shachau shachau commented Jul 25, 2022

https://github.com/microsoft/cpp_client_telemetry_modules/issues/176

Multiple users are reporting high memory usage, especially on M1 Macs. We've made some significant gains in the last few months, but we are still running into users seeing 12+GB of usage. I've been in a thread with one such user who has provided leak traces

The "with aria" traces were collected first, in OneDrive's default configuration. The user enabled malloc_history and let the product run until it reached around 1GB usage, at which point it is my understanding that we crashed (this has to do with malloc tracing and not another bug). After that, I had the user disable Aria, and run the same trace again (the "without aria" trace). No crash this time.

From a comparison of the traces, it is obvious that Aria is responsible for significant memory consumption. What seems to be happening here (and this is consistent with when Apple looked at it) is that Aria is allocating objects inside an autoreleasepool that is never released.

What changes:
Just added autorelease pool to functions that were not releasing memory.

@shachau
Copy link
Contributor Author

shachau commented Jul 25, 2022

@sid-dahiya @lalitb for reference, this is the fix for memory leak issue on Mac that we saw in OneDrive. This fix worked for OneDrive so merging it to the common code.

@shachau
Copy link
Contributor Author

shachau commented Jul 26, 2022

Getting error.
/Users/runner/work/cpp_client_telemetry/cpp_client_telemetry/lib/pal/WorkerThread.cpp:186:17: error: expected expression
@autoreleasepool

@lalitb what compiler do we use? GCC or Clang compiler
https://stackoverflow.com/questions/13708601/autoreleasepool-expected-expression-before-token

All examples of using autoreleasepool is in mm file.
Do we need to change the macro?

@lalitb
Copy link
Contributor

lalitb commented Jul 26, 2022

@lalitb what compiler do we use? GCC or Clang compiler

Clang is used for both mac and iOS compilation. But looking into the PR now, it doesn't make sense to add @autoreleasepool in the C++ code. It is not a C++ construct and will fail with any C++ compiler. Why do we need to add it in the worker thread?

@eduardo-camacho
Copy link
Contributor

@lalitb what compiler do we use? GCC or Clang compiler

Clang is used for both mac and iOS compilation. But looking into the PR now, it doesn't make sense to add @autoreleasepool in the C++ code. It is not a C++ construct and will fail with any C++ compiler. Why do we need to add it in the worker thread?

Agree with your statement, this block do not contain Obj-C object references, hence @autoreleasepool doesn't apply here

@shachau
Copy link
Contributor Author

shachau commented Jul 26, 2022

Thanks removed the autorelease pool from WorkerThread.cpp

Copy link
Contributor

@lalitb lalitb left a comment

Choose a reason for hiding this comment

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

Thanks. LGTM

@lalitb
Copy link
Contributor

lalitb commented Jul 27, 2022

On lighter front, @autoreleasepool user can ignore all these discussions, he is unknowingly getting added here. :)

@shachau shachau merged commit 8d91321 into main Jul 27, 2022
@shachau shachau deleted the shachau/mac-memleak-fix branch July 27, 2022 03:10
@lalitb lalitb mentioned this pull request Sep 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants