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 leaks #33

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Fix: memory leaks #33

wants to merge 5 commits into from

Conversation

EmixamPP
Copy link
Contributor

@EmixamPP EmixamPP commented Jul 2, 2024

This PR fix some memory leaks, as well as the one observed in #19.
I attached two screen captures of a small stress test that switch between the tabs of the demo app as proof.
mem_leak.webm
no_mem_leak.webm

I found others in the runtime, but they would be addressed if #32 is accepted.

@derekstavis
Copy link
Collaborator

derekstavis commented Aug 2, 2024

Once I get #32 merged and then looking to integrate this one

@derekstavis
Copy link
Collaborator

derekstavis commented Aug 5, 2024

When I use this branch I'm seeing a JS error in the widgets demo when I try to change colors:

TypeError: Cannot convert undefined or null to object
    at onClick (demo/widgets/index.js:18765:13)
    at call (native)
    at fireEvent (demo/widgets/index.js:13712:35)

It seems like event targets here are null:

{ target: undefined, currentTarget: undefined }

While in main they are defined:

{
 target: 
   {
    uid: '88',
     style: { uid: '88', style: [Circular], dataset: [Object] },
     dataset: { color: 16007990 }
   },
  currentTarget: 
   {
    uid: '88',
     style: { uid: '88', style: [Circular], dataset: [Object] },
     dataset: { color: 16007990 }
   }
}

Note: I rebased this against master before testing.

@EmixamPP
Copy link
Contributor Author

EmixamPP commented Aug 7, 2024

Ok, I will investigate that issue in the coming weeks

reference and proxy to the components where preventing GC
fix for lvgl#23
@EmixamPP
Copy link
Contributor Author

EmixamPP commented Aug 26, 2024

Rebase on master (+ code adjustment)
I will now look to the issue you described

@EmixamPP
Copy link
Contributor Author

It is fixed now. The [] operator on the instance map wasn't working, I had to use get() instead. Quite strange since this is not directly related to one of my changes.

@EmixamPP
Copy link
Contributor Author

EmixamPP commented Oct 1, 2024

Again, a fix for crashes related to object deletion.

These bindings really need a big refactoring concerning the memory management of the lvgl object coupled with the C++ code. Because of the async nature, this code seems really not memory safe

At least, all of this is already an improvement, but a good modern C++ refactoring would be needed to be sure that these bindings are stable even with larger project.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants