Skip to content

Fix memory leak when forget_image is called while loading#7380

Merged
emilk merged 2 commits intoemilk:mainfrom
Vanadiae:image-loader-assertion
Aug 5, 2025
Merged

Fix memory leak when forget_image is called while loading#7380
emilk merged 2 commits intoemilk:mainfrom
Vanadiae:image-loader-assertion

Conversation

@Vanadiae
Copy link
Copy Markdown
Contributor

This is the same issue that was fixed for the http bytes loader in 239ade9

  • I have followed the instructions in the PR template

Funnily, this comment describes exactly how I encountered this issue:

That assert is wrong if something calls forget between the start of the request and the end of it.

I'm displaying lots of images in a scrolling grid (20 or so visible at a time). It seems like image textures are never freed up automatically so it stacks up a lot meaning I have to unload the image textures manually with egui::Context::forget_image() in each eframe::App::update() call for the images that are no longer shown when scrolling.

Copy link
Copy Markdown
Collaborator

@lucasmerlin lucasmerlin left a comment

Choose a reason for hiding this comment

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

Thanks!

@lucasmerlin lucasmerlin added bug Something is broken egui egui_extras and removed egui labels Jul 22, 2025
@github-actions
Copy link
Copy Markdown

Preview available at https://egui-pr-preview.github.io/pr/7380-image-loader-assertion
Note that it might take a couple seconds for the update to show up after the preview_build workflow has completed.

@Vanadiae Vanadiae changed the title Remove incorrect assertion after image loading is done Draft: Remove incorrect assertion after image loading is done Jul 22, 2025
…ing it

This is the same issue that was fixed for the http bytes loader in
239ade9. However the mentioned fix was
wrong as it kept the loaded data in the cache even in the case where the
Pending cache entry was removed while loading (i.e. the image was forgotten).
Instead, use the same code as ab9f55a
(file loader) but for the ehttp and image loaders.
@Vanadiae
Copy link
Copy Markdown
Contributor Author

Actually I suspect that what 239ade9 (#3750) wanted to achieve is kind of wrong in the case where we forget() between starting the loading process and we finish loading, as it's likely expected that any ongoing loading process gets cancelled (or at least in our case that it simply discards any loading result if forget() was called during the loading process). Indeed that's what #6995 does for the file loader this time for admittedly the same root cause as ours here.

In the case of my application, it means that instead of crashing on the assertion the memory just looks looks like it's getting leaked a lot (it's trivial for me to reach 5GB RAM by just scrolling sufficiently fast that few images have even time to load fully before forget()-ting them). Once I manually call ctx.forget_all_images() (i.e. with a button) things get much more reasonable again.

I suspect that the WebP/SVG/GIF loaders do not have any of those issues as they do not use a background thread for loading (I'm not quite sure why that's something each ImageLoader implementation should care about themselves but that's another topic).

@Vanadiae Vanadiae force-pushed the image-loader-assertion branch from 7c2ff9f to 7878153 Compare July 22, 2025 20:21
@Vanadiae Vanadiae changed the title Draft: Remove incorrect assertion after image loading is done Remove incorrect assertion after image loading is done Jul 22, 2025
@Vanadiae
Copy link
Copy Markdown
Contributor Author

Now the ehttp and image loaders have the same correct behavior in case of forget() than the file loader (code from #6995). The memory consumption is much better now in terms of growth, although likely due to memory fragmentation I still need to call libc::malloc_trim() regularly due to the amount of concurrent image loading that's happening (in separate threads even worse).

@lucasmerlin lucasmerlin changed the title Remove incorrect assertion after image loading is done Fix memory leak when forget_image is called while loading Jul 23, 2025
@emilk emilk added this to the 0.32.1 milestone Aug 5, 2025
@emilk emilk merged commit 8333615 into emilk:main Aug 5, 2025
26 checks passed
lucasmerlin pushed a commit that referenced this pull request Sep 4, 2025
This is the same issue that was fixed for the http bytes loader in
239ade9

* [x] I have followed the instructions in the PR template

----------------

Funnily, [this
comment](#3747 (comment))
describes exactly how I encountered this issue:

> That assert is wrong if something calls forget between the start of
the request and the end of it.

I'm displaying lots of images in a scrolling grid (20 or so visible at a
time). It seems like image textures are never freed up automatically so
it stacks up a lot meaning I have to unload the image textures manually
with `egui::Context::forget_image()` in each `eframe::App::update()`
call for the images that are no longer shown when scrolling.

---------

Co-authored-by: Emil Ernerfeldt <[email protected]>
Masterchef365 pushed a commit to Masterchef365/egui that referenced this pull request Apr 3, 2026
This is the same issue that was fixed for the http bytes loader in
239ade9

* [x] I have followed the instructions in the PR template

----------------

Funnily, [this
comment](emilk#3747 (comment))
describes exactly how I encountered this issue:

> That assert is wrong if something calls forget between the start of
the request and the end of it.

I'm displaying lots of images in a scrolling grid (20 or so visible at a
time). It seems like image textures are never freed up automatically so
it stacks up a lot meaning I have to unload the image textures manually
with `egui::Context::forget_image()` in each `eframe::App::update()`
call for the images that are no longer shown when scrolling.

---------

Co-authored-by: Emil Ernerfeldt <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something is broken egui_extras

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants