Fix memory Leaks on iOS #1057
Fix memory Leaks on iOS #1057
Conversation
|
pasting here from the 1ds-sdk team's channel - Hi team. We are observing significant memory spiking and leaks with 1-DS on iOS devices. I'm attaching a snapshot from the leaks profile taken from instruments. I have tested with the fixes made in this Pull Request. As we can see in the snapshot the memory is spiking quite fast and leaking a significant amount as well. On further analysis we have found out that for every 1-DS telemetry event we send, we are creating a new URL session. Instead of which if a static URL session is used, the leaks and spiking aren't observed anymore. Attaching a screenshot of the same. Wanted to know if there is any reason behind having unique URL sessions for each telemetry request and if making the URL session static might cause any issues. Here is a link to the draft PR. |
|
Adding @eduardo-camacho as reviewer, as he has written this part of iOS code. |
samirnen
requested review from
kindbe,
maxgolov,
mkoscumb,
reyang,
bliptec,
lalitb and
sid-dahiya
as code owners
lib/http/HttpClient_Apple.mm
Outdated
| @@ -29,6 +29,9 @@ | |||
| return std::string("RESP-") + std::to_string(seq.fetch_add(1)); | |||
| } | |||
|
|
|||
| static dispatch_once_t once; | |||
| static NSURLSession* m_session = nil; | |||
There was a problem hiding this comment.
Can't these static's be member variables of HttpRequestApple ?
There was a problem hiding this comment.
Yes I have tried to do the same. The HttpRequestApple class is defined in .mm file. I was getting a compilation error if I defined the static objects under the class inside .mm. A workaround for this is to define the HttpRequestApple class inside .hh header and define the static variables in the header itself. However that exposes the whole HttpRequestApple class to files which import the header. Hence I have created static objects outside of the class inside .mm file.
There was a problem hiding this comment.
Can't we split it in the cc/h file, and ensure the source file is linked when building for iOS? Just a bit concerned with these global statics.
There was a problem hiding this comment.
Can't these static's be member variables of HttpRequestApple ?
Indeed, this static variable could definitively be moved to the function itself as a local static.
There was a problem hiding this comment.
@samirnen - Do you think this is doable?
There was a problem hiding this comment.
I tried moving the variable to the function as a local static. But then the memory is leaking same as before. So didn't go ahead with that approach
There was a problem hiding this comment.
I have a create a new source file for the HTTPRequestApple class and have defined the static variables there
|
Hey @eduardo-camacho @lalitb can you please re review this PR |
lib/http/HttpRequestApple.mm
Outdated
|
|
||
| namespace MAT_NS_BEGIN { | ||
|
|
||
| static dispatch_once_t once; |
There was a problem hiding this comment.
These are still global statics, but I understand there are challenges in making it member-variable for this class - https://stackoverflow.com/questions/13856037/can-i-declare-dispatch-once-t-predicate-as-a-member-variable-instead-of-static/13858628#13858628. Also, there are issues in making it static member of the class. Will approve it if it is not practically possible to do either.
Also I understand end to end tests have been done - events are flowing, and there is no memory leak observed in this part of code.
There was a problem hiding this comment.
Effectively this change is more disruptive than previous version when Lalit commented about the global static, and the global static thing is still present; am I missing something here? If we are going to allow the global statics in this case, then it's just cleaner how it was before splitting files. Would you agree @lalitb?
lib/http/HttpRequestApple.hpp
Outdated
| @@ -0,0 +1,34 @@ | |||
| // | |||
There was a problem hiding this comment.
This file should be named HttpRequest_Apple.http (similar case with new .mm file) to match current platform specific naming scheme.
Yes, I too feel so, the initial approach was much cleaner if we can't get rid of global statics with this approach. Sorry @samirnen if there are too many iterations :) |
Observing significant leaks on iOS devices when using 1-DS to send telemetry. Making NSURLSession static instead of creating a new instance for every telemetry event resolved the issue of memory leakage. Attaching snapshots from instruments where memory usage is being tracked with a static NSURLSession and a non static one.