Clean up allocations from finalized object links #1566
Conversation
This fixes a small memory leak from java objects being passed from java to javascript and then being garbage collected
This fixes a small memory leak from java objects being passed from java to javascript and then being garbage collected
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign the CLA at https://www.nativescript.org/cla. |
| po->Get(m_isolate)->SetInternalField(jsInfoIdx, Undefined(m_isolate)); | ||
| po->Reset(); | ||
| auto it = m_idToObject.find(javaObjectID); | ||
| m_idToObject.erase(it); |
There was a problem hiding this comment.
It would be easier and safer to directly m_idToObject.erase(javaObjectID); here.
There was a problem hiding this comment.
Agreed. Does your team prefer this commit to be modified (rebase with a fixup) or an extra commit?
There was a problem hiding this comment.
Extra commit should do it, thanks.
| m_idToObject.erase(it); | ||
| delete po; | ||
| delete callbackState; | ||
| } |
There was a problem hiding this comment.
Also delete the po and callbackState variables here: https://github.com/NativeScript/android-runtime/pull/1566/files#diff-6c6c152bfa906bd1defa04a2daacdedbR327
There was a problem hiding this comment.
Good point, thanks, we'll add that.
|
@FrancisDuvivierTHEO could you, please, sign the CLA here - https://www.nativescript.org/cla. |
3 participants
Description
This PR is for fixing a memory leak which is caused by passing java objects to the javascript runtime. This memory leak is small, but with a lot of objects passed, it is certainly measurable. From our measurements. Every java object passed to javascript adds about 85bytes to the native memory which never gets cleaned up even if the object is garbage collected on both sides.
We found the native objects causing this leak and deleted them on finalization.
Does your commit message include the wording below to reference a specific issue in this repo?
No there is no issue for it in the repo currently
Related Pull Requests
None
Does your pull request have [unit tests]
No, this kind of memory leak is not unit-testable from java or javascript as far as I can tell.
Setting up a cpp unit test suite for this would be overkill.