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

sugest to avoid to memory leak #4118

Merged
merged 5 commits into from
Sep 4, 2023
Merged

sugest to avoid to memory leak #4118

merged 5 commits into from
Sep 4, 2023

Conversation

MatejMagat305
Copy link
Contributor

avoid memory leak resume and sugest to avoid to call GO functions for empty life cycle functions ...

@MatejMagat305 MatejMagat305 changed the title Patch 1 sugest to avoid to memory leak Aug 1, 2023
@coveralls
Copy link

Coverage Status

coverage: 66.487% (-0.003%) from 66.49% when pulling b161fcd on MatejMagat305:patch-1 into ecb8c1a on fyne-io:develop.

Copy link
Member

@andydotxyz andydotxyz left a comment

Choose a reason for hiding this comment

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

I'm not sure about the way that some callbacks are deleted and others moved.

Why not make the PR just about the memory leak fix?

internal/driver/mobile/app/android.c Show resolved Hide resolved
@MatejMagat305
Copy link
Contributor Author

I'm not sure about the way that some callbacks are deleted and others moved.

Why not make the PR just about the memory leak fix?

this lines: https://github.com/fyne-io/fyne/pull/4118/files#diff-86b6487000ebad8b8c46272d0aa615cdbec1dba15013d82bed007ffa593e5464R90 is memory leaks, others are only therefore nothing theese functions do and call GO from C is recomended to eliminate ...

@Jacalz
Copy link
Member

Jacalz commented Aug 15, 2023

But you have, as stated in #4118 (comment), removed one of the functions. Why?

@andydotxyz
Copy link
Member

andydotxyz commented Aug 28, 2023

I think I see why it looks inconsistent - because the onResume was written in C to do something but not in Go. But it was called processOnResume (so that the Go callback names were consistent). If that was renamed to onResume in the C file then I think we would have a consistent code and this could be approved.
Is that OK @MatejMagat305 ?

@MatejMagat305
Copy link
Contributor Author

ok

@Jacalz
Copy link
Member

Jacalz commented Sep 3, 2023

Why did you close it? Andy just suggested changes

@MatejMagat305
Copy link
Contributor Author

ach soory, I will changes tomorow

@MatejMagat305 MatejMagat305 reopened this Sep 3, 2023
Copy link
Member

@andydotxyz andydotxyz left a comment

Choose a reason for hiding this comment

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

Thanks for making these optimisations

Copy link
Member

@Jacalz Jacalz left a comment

Choose a reason for hiding this comment

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

Thanks. If there are more places where we are passing Go functions back into C, we probably want to remove those as well some time in the future. It is quite slow from what I have understood (but a lot faster in Go 1.21).

@Jacalz Jacalz merged commit a9bd060 into fyne-io:develop Sep 4, 2023
11 checks passed
@MatejMagat305 MatejMagat305 deleted the patch-1 branch September 4, 2023 19:08
@MatejMagat305
Copy link
Contributor Author

Thanks. If there are more places where we are passing Go functions back into C, we probably want to remove those as well some time in the future. It is quite slow from what I have understood (but a lot faster in Go 1.21).

well there are some functions like onContentRectChanged or onNativeWindowCreated, which are empty, but I did not know which one will you plan use ..., life time I dare move because I saw that life time are binding with running loop (are call from other way ...)

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.

4 participants