Fix memory leaks in some cache maps #2101

fpabl0 · 2021-03-19T21:45:49Z

Description:

Potentially fix #348, #735, #2069.

Checklist:

Tests included.
Lint and formatter run with no errors.
Tests all pass.
~~Call cache.Clean on mobile too.~~ Update mobile driver, canvas and windows to have the same desktop changes.
Find a better way to clean textures cache.
Move svg_cache to cache package.
Decision about minSize cache on mobile driver (needs refactor).
Remove the mutex (added in this PR) for texture cache as it is not needed (texture cache is only manipulated in one goroutine).
Solve texture cache cleaning for multiple windows.

fpabl0 · 2021-03-19T21:51:24Z

Oh I forgot that ticker.Reset does not exist in Go 1.12, I will update later.

Jacalz · 2021-03-20T07:41:13Z

Oh I forgot that ticker.Reset does not exist in Go 1.12, I will update later.

Just add a comment about it (after you update the code) so that we can change it when we update the minimum Go version 🙂

…utine, added cache.NotifyCanvasRefresh, fix the way textures are freed, fix memory leak on window.Close

fpabl0 · 2021-03-21T19:07:12Z

@andydotxyz @Jacalz it is not finished yet, but I would like to know your thoughts/suggestions about the current changes :)

Basically, in the latest changes I have removed the go routine used to clean the cache in favor of using the paint events as the ticker.

Some of my doubts are:

Should we destroy expired renderers independently of a canvas refresh action? (this is the current behavior with this PR)
Texture cache map does not need a mutex because its functions are always called from the same go routine, however as you can see now the texture cache map saves a reference for the canvas which the primitive belongs to, by doing it we could use that map (only for primitives) inside CanvasForObject instead of the canvases cache map, this action would allow us to reduce the canvases cache map (because it would not contain primitives anymore) and delete more quickly them when they are destroyed (if we use canvases cache map we would need to wait them for expiration), however this implies to add a mutex to the textures cache map (CanvasForObject can be called from anywhere), what do you think? Should we keep all objects in canvases cache or just the ones that are not primitives?

andydotxyz · 2021-03-22T17:28:00Z

This is a good question. I think it was put that way for thinking it was faster. But other approaches probably OK :)
I'm not sure about the underlying thought here. Many of our primitives don't translate to graphics primitives (yet?). We need to rasterise some elements (like rectangles with borders, and lines) so that's why the cache exists. Does that impact your thoughts?

fpabl0 · 2021-03-22T23:41:50Z

This is a good question. I think it was put that way for thinking it was faster. But other approaches probably OK :)

Indeed it is faster. Current PR use both of them (destroy on refresh and destroy when expired). However destroy on refresh will not destroy objects that are out of scope (because they no longer belonged to the object tree).

I'm not sure about the underlying thought here. Many of our primitives don't translate to graphics primitives (yet?). We need to rasterise some elements (like rectangles with borders, and lines) so that's why the cache exists. Does that impact your thoughts?

Hmm for primitives I mean the objects created with canvas.New... (Rectangle, Image, Text and others) they all are textures for gl and so they are saved in the textures cache map. Basically this is the functionality of every cache map:

rendereres map => widgets associated with their renderers
textures map => canvas.New... objects associated with their texture.
canvases map => all objects (widgets, containers and canvas.New... objects)

Textures are created and destroyed really fast. The destroy action will delete immediately the texture from the textures cache map but not from the canvases map (we would need to wait them to expire to release that memory).

So if we change the map data to:

rendereres map => widgets associated with their renderers
textures map => canvas.New... objects associated with their texture and canvas.
canvases map => all objects (only widgets and containers)

So canvases map will not longer hold texture objects, and by doing so, we would just need to delete the texture from the textures cache map.

Then CanvasForObject should only check the type of the object. If it is a widget or container, it will use the canvases map, otherwise it will use the textures map to get its canvas.

andydotxyz · 2021-03-23T10:13:06Z

However destroy on refresh will not destroy objects that are out of scope (because they no longer belonged to the object tree).

Good point!

So if we change the map data to:

rendereres map => widgets associated with their renderers

textures map => canvas.New... objects associated with their texture and canvas.

canvases map => all objects (only widgets and containers)

So canvases map will not longer hold texture objects, and by doing so, we would just need to delete the texture from the textures cache map.

I don't think this is wise from what I have read - the texture cache refers to the current rendering of an object. If this object is changed and Refresh() called then the object should remain in the canvases map, but it's texture needs to be invalidated.

fpabl0 · 2021-03-23T13:52:38Z

I don't think this is wise from what I have read - the texture cache refers to the current rendering of an object. If this object is changed and Refresh() called then the object should remain in the canvases map, but it's texture needs to be invalidated.

On yes, you are right, I missed that fact. Thanks :)

…o theme changes and remove 10ms delay on every paint event

fpabl0 · 2021-03-25T17:16:55Z

@andydotxyz I have moved the svg_cache to the cache package and removed the 10 ms delay in mobile driver. Could you a do quick review in internal/driver/gomobile/driver.go if you are agreed with the changes? I think that change probably solve the theme change issue on mobile.

fpabl0 · 2021-04-09T22:21:44Z

@andydotxyz Ok this is almost done. Desktop side is ready (maybe better naming for the added cache functions is needed), however on mobile side I think a refactorization is needed specially to rid off the minSizeCache and use renderCacheNode-canvasesCache solution used in desktop (or viceversa, I am not sure what is the better solution, maybe was it discussed before?)

So should I add TODO comments and leave this PR as it is now? Or should this PR wait until mobile driver is refactored (mainly just add the renderCacheNode and queueEvent method)?

andydotxyz · 2021-04-12T09:29:45Z

Or should this PR wait until mobile driver is refactored (mainly just add the renderCacheNode and queueEvent method)?

It would make it a lot easier to apply this in a shared-code manner if the mobile driver had the equivalent of 7461574 applied.

I am nervous that the drivers are moving further apart instead of coming together, so let's not make the differences and performance gap greater yet.

fpabl0 · 2021-04-12T15:07:28Z

It would make it a lot easier to apply this in a shared-code manner if the mobile driver had the equivalent of 7461574 applied.

@andydotxyz Totally agreed. So should I open another PR as the initial step to try sharing code between the two drivers? Then this PR should wait until that PR is landed. Is that ok?

andydotxyz · 2021-04-14T09:49:06Z

@andydotxyz Totally agreed. So should I open another PR as the initial step to try sharing code between the two drivers? Then this PR should wait until that PR is landed. Is that ok?

If you're OK doing that I think it sounds like a great idea

…mobile window is closed

internal/cache/base.go

internal/cache/widget.go

…Renderer function

fpabl0 · 2021-06-02T18:40:48Z

Yes, it is :)

Jacalz

This is looking really good. Just have a few suggestions for changes :)

internal/cache/texture_common.go

internal/cache/base.go

Co-authored-by: Jacob <Jacalz@users.noreply.github.com>

Jacalz

Thanks for this. It is certainly a good improvement. I checked the memory usage before and after and and it certainly does seem to be much better at releasing memory. I still see the memory slowly climbing without me doing anything inside the application, but I guess that's another issue :)

fpabl0 · 2021-06-03T12:10:50Z

I still see the memory slowly climbing without me doing anything inside the application

Hmm the memory (after some time) should be constant (without increasing) 🤔. Could you share the code you are using to test this?

I think I should add some test cases, I will try to add them later today.

Jacalz · 2021-06-03T12:38:19Z

I still see the memory slowly climbing without me doing anything inside the application

Hmm the memory (after some time) should be constant (without increasing) 🤔. Could you share the code you are using to test this?

I was just using fyne_demo inside the branch that this PR is based on (on Linux). I think it started at 22Mb and then it slowly climbed to 29Mb. I don't know if this was before or after the time it should have stabilised in.

andydotxyz

Great work, but it's not quite in-keeping with our code style.
You've included a lot of comments, some that are blocks to separate methods, and some that are telling what a line of code does - both seem superfluous.
If code needs a comment then it generally should be more cleanly written.

Additionally cache/base.go adds a lot of new code but I don't think that any tests cover it.

andydotxyz · 2021-06-03T12:53:52Z

internal/cache/base.go

+	cleanTaskInterval = cacheDuration / 2
+
+	canvasRefreshCh = make(chan struct{}, 1)
+	expiredObjects  = make([]fyne.CanvasObject, 0, 50)


I don't think this needs to be global, it is only used internally in two methods, both of which append and then clean out in a single run.

I made it global only to avoid having to recreate the slice on every clean task, in that way the same slice is reused on every task without the need to allocate memory again. Should I make it local?

Making it global requires locking, surely being able to avoid that outweighs any time required in allocating memory?

It does not require locking because cache.Clean won't run asynchronously

Is this ok?

@andydotxyz Any comment about this?

internal/cache/base.go

internal/driver/glfw/window.go

fpabl0 · 2021-06-04T06:58:48Z

I was just using fyne_demo inside the branch that this PR is based on (on Linux). I think it started at 22Mb and then it slowly climbed to 29Mb. I don't know if this was before or after the time it should have stabilised in.

Hmm in fyne_demo, is a bit difficult to test this, because if you go to the screens that already have memory leaks, then the memory will never be stable

fpabl0 · 2021-06-04T06:59:56Z

Additionally cache/base.go adds a lot of new code but I don't think that any tests cover it.

Yes, you are right, I will try to add them later today.

internal/driver/glfw/window.go

…ed in the PR review

fpabl0 · 2021-06-04T20:28:24Z

@andydotxyz I have worked in some of your suggestions, waiting for your review now. If everything goes fine, I will start writing the tests.

…() for glfw driver as suggested in PR review

andydotxyz · 2021-06-14T09:56:57Z

Looking good thanks. If tests can be added things should be good.

fpabl0 · 2021-06-14T19:50:35Z

Looking good thanks. If tests can be added things should be good.

Great :), I'll try to add them tonight.

Jacalz

There are most likely more things we can do to improve the memory usage but this is a great step in the right direction. Nice work! 👍

andydotxyz · 2021-06-17T17:00:49Z

Yes, nice work thanks.

fpabl0 added 2 commits March 18, 2021 23:30

fix memory leak in widget renderers, textures and canvases caches

5ec29d8

fix the way textures are destroyed from cache

1b8916c

fpabl0 added 2 commits March 20, 2021 20:44

remove texture cache mutex

c1c52b6

use draw events to run the clean task instead of creating a new go ro…

32b0d45

…utine, added cache.NotifyCanvasRefresh, fix the way textures are freed, fix memory leak on window.Close

move svg_cache to cache package, improve mobile driver by listening t…

801d271

…o theme changes and remove 10ms delay on every paint event

fpabl0 added 4 commits March 25, 2021 13:40

revert mobile driver changes

7157855

Merge branch 'develop' into fix/cache-renderer-memleak

34ee183

remove repeated code

29ae75f

apply clearing cache tasks to mobile driver

36842d9

fpabl0 added 4 commits May 12, 2021 16:32

Merge branch 'develop' into fix/cache-renderer-memleak

378dc5d

Merge branch 'develop' into fix/cache-renderer-memleak

e97fd54

added cache logic to common driver and canvas, free resources when go…

3223933

…mobile window is closed

group variables

07a7d30

fpabl0 commented May 13, 2021

View reviewed changes

internal/cache/base.go Outdated Show resolved Hide resolved

fpabl0 marked this pull request as ready for review May 13, 2021 20:24

fpabl0 commented May 14, 2021

View reviewed changes

internal/cache/widget.go Outdated Show resolved Hide resolved

fpabl0 added 2 commits May 29, 2021 12:53

Merge branch 'develop' into fix/cache-renderer-memleak

16c8d56

destroy widget renderers only when canvas refresh and improve Destroy…

2a3abfb

…Renderer function

Jacalz requested changes Jun 2, 2021

View reviewed changes

internal/cache/texture_common.go Outdated Show resolved Hide resolved

internal/cache/base.go Outdated Show resolved Hide resolved

internal/cache/base.go Show resolved Hide resolved

internal/cache/base.go Outdated Show resolved Hide resolved

Update comment in internal/cache/texture_common.go

d0041e5

Co-authored-by: Jacob <Jacalz@users.noreply.github.com>

fyne-io deleted a comment from ddkwork Jun 3, 2021

Jacalz previously approved these changes Jun 3, 2021

View reviewed changes

andydotxyz requested changes Jun 3, 2021

View reviewed changes

andydotxyz reviewed Jun 4, 2021

View reviewed changes

internal/driver/glfw/window.go Show resolved Hide resolved

fpabl0 added 2 commits June 4, 2021 11:57

Merge branch 'develop' into fix/cache-renderer-memleak

8764345

rename CleanTask to Clean and ForceCleanFor to CleanCanvas as suggest…

6fc2bfc

…ed in the PR review

fpabl0 dismissed Jacalz’s stale review via 6fc2bfc June 4, 2021 17:10

fpabl0 added 2 commits June 4, 2021 13:54

remove unnecessary comments

ad7f974

remove NotifyCanvasRefresh logic

342712a

fpabl0 requested a review from andydotxyz June 4, 2021 20:28

call cache.CleanCanvas in window.destroy() instead of in window.Close…

569a354

…() for glfw driver as suggested in PR review

fpabl0 added 2 commits June 15, 2021 02:41

Merge branch 'develop' into fix/cache-renderer-memleak

1d35466

added tests

2c65c07

Jacalz approved these changes Jun 15, 2021

View reviewed changes

andydotxyz approved these changes Jun 17, 2021

View reviewed changes

Merge branch 'develop' into fix/cache-renderer-memleak

b018a2b

fpabl0 merged commit f14ac77 into fyne-io:develop Jun 18, 2021

fpabl0 deleted the fix/cache-renderer-memleak branch June 18, 2021 08:28

elliot-aeroqual mentioned this pull request Feb 9, 2023

Clear render cache when software rendering #3632

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix memory leaks in some cache maps #2101

Fix memory leaks in some cache maps #2101

fpabl0 commented Mar 19, 2021 •

edited

fpabl0 commented Mar 19, 2021

Jacalz commented Mar 20, 2021 •

edited

fpabl0 commented Mar 21, 2021 •

edited

andydotxyz commented Mar 22, 2021

fpabl0 commented Mar 22, 2021 •

edited

andydotxyz commented Mar 23, 2021

fpabl0 commented Mar 23, 2021

fpabl0 commented Mar 25, 2021

fpabl0 commented Apr 9, 2021

andydotxyz commented Apr 12, 2021

fpabl0 commented Apr 12, 2021 •

edited

andydotxyz commented Apr 14, 2021

fpabl0 commented Jun 2, 2021

Jacalz left a comment

Jacalz left a comment

fpabl0 commented Jun 3, 2021 •

edited

Jacalz commented Jun 3, 2021

andydotxyz left a comment

andydotxyz Jun 3, 2021

fpabl0 Jun 4, 2021

andydotxyz Jun 4, 2021

fpabl0 Jun 4, 2021

fpabl0 Jun 9, 2021

fpabl0 Jun 14, 2021

fpabl0 commented Jun 4, 2021

fpabl0 commented Jun 4, 2021

fpabl0 commented Jun 4, 2021

andydotxyz commented Jun 14, 2021

fpabl0 commented Jun 14, 2021

Jacalz left a comment

andydotxyz commented Jun 17, 2021

Fix memory leaks in some cache maps #2101

Fix memory leaks in some cache maps #2101

Conversation

fpabl0 commented Mar 19, 2021 • edited

Description:

Checklist:

fpabl0 commented Mar 19, 2021

Jacalz commented Mar 20, 2021 • edited

fpabl0 commented Mar 21, 2021 • edited

andydotxyz commented Mar 22, 2021

fpabl0 commented Mar 22, 2021 • edited

andydotxyz commented Mar 23, 2021

fpabl0 commented Mar 23, 2021

fpabl0 commented Mar 25, 2021

fpabl0 commented Apr 9, 2021

andydotxyz commented Apr 12, 2021

fpabl0 commented Apr 12, 2021 • edited

andydotxyz commented Apr 14, 2021

fpabl0 commented Jun 2, 2021

Jacalz left a comment

Choose a reason for hiding this comment

Jacalz left a comment

Choose a reason for hiding this comment

fpabl0 commented Jun 3, 2021 • edited

Jacalz commented Jun 3, 2021

andydotxyz left a comment

Choose a reason for hiding this comment

andydotxyz Jun 3, 2021

Choose a reason for hiding this comment

fpabl0 Jun 4, 2021

Choose a reason for hiding this comment

andydotxyz Jun 4, 2021

Choose a reason for hiding this comment

fpabl0 Jun 4, 2021

Choose a reason for hiding this comment

fpabl0 Jun 9, 2021

Choose a reason for hiding this comment

fpabl0 Jun 14, 2021

Choose a reason for hiding this comment

fpabl0 commented Jun 4, 2021

fpabl0 commented Jun 4, 2021

fpabl0 commented Jun 4, 2021

andydotxyz commented Jun 14, 2021

fpabl0 commented Jun 14, 2021

Jacalz left a comment

Choose a reason for hiding this comment

andydotxyz commented Jun 17, 2021

fpabl0 commented Mar 19, 2021 •

edited

Jacalz commented Mar 20, 2021 •

edited

fpabl0 commented Mar 21, 2021 •

edited

fpabl0 commented Mar 22, 2021 •

edited

fpabl0 commented Apr 12, 2021 •

edited

fpabl0 commented Jun 3, 2021 •

edited