Clean up image resources for the current window #45969
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cla-bot
bot
added
the
cla-signed
Contributor
Author
|
/cherry-pick preview |
github-actions bot
pushed a commit
that referenced
this pull request
Jan 2, 2026
Closes #35894 Trace: https://drive.google.com/file/d/1pXDFzOg4ZS4p2SX8fk9Cjnyy1Qd1Pkrx/view?usp=sharing [Relevant part](https://github.com/user-attachments/files/24412472/image_leak.txt) Release Notes: - Fixed a memory leak when opening images
zed-zippy bot
added a commit
that referenced
this pull request
Jan 2, 2026
… to preview) (#45970) Cherry-pick of #45969 to preview ---- Closes #35894 Trace: https://drive.google.com/file/d/1pXDFzOg4ZS4p2SX8fk9Cjnyy1Qd1Pkrx/view?usp=sharing [Relevant part](https://github.com/user-attachments/files/24412472/image_leak.txt) Release Notes: - Fixed a memory leak when opening images Co-authored-by: Kirill Bulatov <kirill@zed.dev>
SomeoneToIgnore
added a commit
that referenced
this pull request
Jan 4, 2026
…#45971) When working on #45969 I've noticed that whenever I try to open a binary file ``` ~/Desktop/svenska ❯ du -ha "Svenska А2B1. 07.09.2025 [u6qEIe9-COc].mkv" 456M Svenska А2B1. 07.09.2025 [u6qEIe9-COc].mkv ``` Zed allocates all its size <img width="214" height="104" alt="image" src="https://github.com/user-attachments/assets/e67ad522-b8a4-4e8e-9961-13030a34df1c" /> <img width="345" height="154" alt="image" src="https://github.com/user-attachments/assets/ea691020-0d02-4acc-88c3-476385f309bb" /> only to show me this: <img width="979" height="677" alt="image" src="https://github.com/user-attachments/assets/ff91cc9d-eb59-4cee-b06f-5758acd1bd5d" /> Given that our existing code checks first 1024 bytes to decide whether to bail on a binary file or not, this seems very wasteful — hence, adjusted the code to read the "header" and check that first, and only continue reading the entire file after the checks are successful. I suspect this should also help the project search, esp. the crashes and memory usage during that when many binary files are present? Release Notes: - Improved Zed's memory usage when attempting to open binary files
rtfeldman
pushed a commit
that referenced
this pull request
Jan 5, 2026
Closes #35894 Trace: https://drive.google.com/file/d/1pXDFzOg4ZS4p2SX8fk9Cjnyy1Qd1Pkrx/view?usp=sharing [Relevant part](https://github.com/user-attachments/files/24412472/image_leak.txt) Release Notes: - Fixed a memory leak when opening images
rtfeldman
pushed a commit
that referenced
this pull request
Jan 5, 2026
…#45971) When working on #45969 I've noticed that whenever I try to open a binary file ``` ~/Desktop/svenska ❯ du -ha "Svenska А2B1. 07.09.2025 [u6qEIe9-COc].mkv" 456M Svenska А2B1. 07.09.2025 [u6qEIe9-COc].mkv ``` Zed allocates all its size <img width="214" height="104" alt="image" src="https://github.com/user-attachments/assets/e67ad522-b8a4-4e8e-9961-13030a34df1c" /> <img width="345" height="154" alt="image" src="https://github.com/user-attachments/assets/ea691020-0d02-4acc-88c3-476385f309bb" /> only to show me this: <img width="979" height="677" alt="image" src="https://github.com/user-attachments/assets/ff91cc9d-eb59-4cee-b06f-5758acd1bd5d" /> Given that our existing code checks first 1024 bytes to decide whether to bail on a binary file or not, this seems very wasteful — hence, adjusted the code to read the "header" and check that first, and only continue reading the entire file after the checks are successful. I suspect this should also help the project search, esp. the crashes and memory usage during that when many binary files are present? Release Notes: - Improved Zed's memory usage when attempting to open binary files
SomeoneToIgnore
added a commit
that referenced
this pull request
Jan 14, 2026
This reverts commit 94faaeb.
This was referenced Jan 14, 2026
SomeoneToIgnore
added a commit
that referenced
this pull request
Jan 14, 2026
…6779) This reverts commit 94faaeb. The logic changed in the [original PR](#45969) is either misplaced or lacks a counterpart that reacts on `gpui::Image` drop one way or another. The change was dropping the texture out of the global rendering atlas when an image preview tab was disposed of, while in reality, another instance (agent panel or another image preview tab) could require the very same atlas entry to be rendered meanwhile. Currently, only `RetainAllImageCache` in Zed does any kind of image cleanup, and any other image usages will leak memory. What makes it harder is that the atlas entry needs to live as long as a particular `Arc<Image>` lives, and some public `gpui` API expects this type to stay: https://github.com/zed-industries/zed/blob/e747cfc955e8cfd9327d8d6b8d617cf1d3a6bcaa/crates/gpui/src/platform.rs#L1851-L1866 For image viewer in particular, we need to consider why `RetainAllImageCache` is not used there; for the global, normal fix, we need to consider ways to have `cx` and `window` and a way to react on `Image` drop. As an option, we could break the `gpui` API (as it [happens periodically](#46183) already) and use `Entity<Image>` instead of `Arc`, then react with `cx.on_release_in` for each such image. Closes #46755 Closes #46435 Release Notes: - Fixed panics on concurrent image handling
github-actions bot
pushed a commit
that referenced
this pull request
Jan 14, 2026
…6779) This reverts commit 94faaeb. The logic changed in the [original PR](#45969) is either misplaced or lacks a counterpart that reacts on `gpui::Image` drop one way or another. The change was dropping the texture out of the global rendering atlas when an image preview tab was disposed of, while in reality, another instance (agent panel or another image preview tab) could require the very same atlas entry to be rendered meanwhile. Currently, only `RetainAllImageCache` in Zed does any kind of image cleanup, and any other image usages will leak memory. What makes it harder is that the atlas entry needs to live as long as a particular `Arc<Image>` lives, and some public `gpui` API expects this type to stay: https://github.com/zed-industries/zed/blob/e747cfc955e8cfd9327d8d6b8d617cf1d3a6bcaa/crates/gpui/src/platform.rs#L1851-L1866 For image viewer in particular, we need to consider why `RetainAllImageCache` is not used there; for the global, normal fix, we need to consider ways to have `cx` and `window` and a way to react on `Image` drop. As an option, we could break the `gpui` API (as it [happens periodically](#46183) already) and use `Entity<Image>` instead of `Arc`, then react with `cx.on_release_in` for each such image. Closes #46755 Closes #46435 Release Notes: - Fixed panics on concurrent image handling
github-actions bot
pushed a commit
that referenced
this pull request
Jan 14, 2026
…6779) This reverts commit 94faaeb. The logic changed in the [original PR](#45969) is either misplaced or lacks a counterpart that reacts on `gpui::Image` drop one way or another. The change was dropping the texture out of the global rendering atlas when an image preview tab was disposed of, while in reality, another instance (agent panel or another image preview tab) could require the very same atlas entry to be rendered meanwhile. Currently, only `RetainAllImageCache` in Zed does any kind of image cleanup, and any other image usages will leak memory. What makes it harder is that the atlas entry needs to live as long as a particular `Arc<Image>` lives, and some public `gpui` API expects this type to stay: https://github.com/zed-industries/zed/blob/e747cfc955e8cfd9327d8d6b8d617cf1d3a6bcaa/crates/gpui/src/platform.rs#L1851-L1866 For image viewer in particular, we need to consider why `RetainAllImageCache` is not used there; for the global, normal fix, we need to consider ways to have `cx` and `window` and a way to react on `Image` drop. As an option, we could break the `gpui` API (as it [happens periodically](#46183) already) and use `Entity<Image>` instead of `Arc`, then react with `cx.on_release_in` for each such image. Closes #46755 Closes #46435 Release Notes: - Fixed panics on concurrent image handling
zed-zippy bot
added a commit
that referenced
this pull request
Jan 14, 2026
…6779) (cherry-pick to stable) (#46782) Cherry-pick of #46779 to stable ---- This reverts commit 94faaeb. The logic changed in the [original PR](#45969) is either misplaced or lacks a counterpart that reacts on `gpui::Image` drop one way or another. The change was dropping the texture out of the global rendering atlas when an image preview tab was disposed of, while in reality, another instance (agent panel or another image preview tab) could require the very same atlas entry to be rendered meanwhile. Currently, only `RetainAllImageCache` in Zed does any kind of image cleanup, and any other image usages will leak memory. What makes it harder is that the atlas entry needs to live as long as a particular `Arc<Image>` lives, and some public `gpui` API expects this type to stay: https://github.com/zed-industries/zed/blob/e747cfc955e8cfd9327d8d6b8d617cf1d3a6bcaa/crates/gpui/src/platform.rs#L1851-L1866 For image viewer in particular, we need to consider why `RetainAllImageCache` is not used there; for the global, normal fix, we need to consider ways to have `cx` and `window` and a way to react on `Image` drop. As an option, we could break the `gpui` API (as it [happens periodically](#46183) already) and use `Entity<Image>` instead of `Arc`, then react with `cx.on_release_in` for each such image. Closes #46755 Closes #46435 Release Notes: - Fixed panics on concurrent image handling Co-authored-by: Kirill Bulatov <kirill@zed.dev>
zed-zippy bot
added a commit
that referenced
this pull request
Jan 14, 2026
…6779) (cherry-pick to preview) (#46781) Cherry-pick of #46779 to preview ---- This reverts commit 94faaeb. The logic changed in the [original PR](#45969) is either misplaced or lacks a counterpart that reacts on `gpui::Image` drop one way or another. The change was dropping the texture out of the global rendering atlas when an image preview tab was disposed of, while in reality, another instance (agent panel or another image preview tab) could require the very same atlas entry to be rendered meanwhile. Currently, only `RetainAllImageCache` in Zed does any kind of image cleanup, and any other image usages will leak memory. What makes it harder is that the atlas entry needs to live as long as a particular `Arc<Image>` lives, and some public `gpui` API expects this type to stay: https://github.com/zed-industries/zed/blob/e747cfc955e8cfd9327d8d6b8d617cf1d3a6bcaa/crates/gpui/src/platform.rs#L1851-L1866 For image viewer in particular, we need to consider why `RetainAllImageCache` is not used there; for the global, normal fix, we need to consider ways to have `cx` and `window` and a way to react on `Image` drop. As an option, we could break the `gpui` API (as it [happens periodically](#46183) already) and use `Entity<Image>` instead of `Arc`, then react with `cx.on_release_in` for each such image. Closes #46755 Closes #46435 Release Notes: - Fixed panics on concurrent image handling Co-authored-by: Kirill Bulatov <kirill@zed.dev>
rtfeldman
pushed a commit
that referenced
this pull request
Jan 14, 2026
…6779) This reverts commit 94faaeb. The logic changed in the [original PR](#45969) is either misplaced or lacks a counterpart that reacts on `gpui::Image` drop one way or another. The change was dropping the texture out of the global rendering atlas when an image preview tab was disposed of, while in reality, another instance (agent panel or another image preview tab) could require the very same atlas entry to be rendered meanwhile. Currently, only `RetainAllImageCache` in Zed does any kind of image cleanup, and any other image usages will leak memory. What makes it harder is that the atlas entry needs to live as long as a particular `Arc<Image>` lives, and some public `gpui` API expects this type to stay: https://github.com/zed-industries/zed/blob/e747cfc955e8cfd9327d8d6b8d617cf1d3a6bcaa/crates/gpui/src/platform.rs#L1851-L1866 For image viewer in particular, we need to consider why `RetainAllImageCache` is not used there; for the global, normal fix, we need to consider ways to have `cx` and `window` and a way to react on `Image` drop. As an option, we could break the `gpui` API (as it [happens periodically](#46183) already) and use `Entity<Image>` instead of `Arc`, then react with `cx.on_release_in` for each such image. Closes #46755 Closes #46435 Release Notes: - Fixed panics on concurrent image handling
LivioGama
pushed a commit
to LivioGama/zed
that referenced
this pull request
Jan 20, 2026
Closes zed-industries#35894 Trace: https://drive.google.com/file/d/1pXDFzOg4ZS4p2SX8fk9Cjnyy1Qd1Pkrx/view?usp=sharing [Relevant part](https://github.com/user-attachments/files/24412472/image_leak.txt) Release Notes: - Fixed a memory leak when opening images
LivioGama
pushed a commit
to LivioGama/zed
that referenced
this pull request
Jan 20, 2026
…zed-industries#45971) When working on zed-industries#45969 I've noticed that whenever I try to open a binary file ``` ~/Desktop/svenska ❯ du -ha "Svenska А2B1. 07.09.2025 [u6qEIe9-COc].mkv" 456M Svenska А2B1. 07.09.2025 [u6qEIe9-COc].mkv ``` Zed allocates all its size <img width="214" height="104" alt="image" src="https://github.com/user-attachments/assets/e67ad522-b8a4-4e8e-9961-13030a34df1c" /> <img width="345" height="154" alt="image" src="https://github.com/user-attachments/assets/ea691020-0d02-4acc-88c3-476385f309bb" /> only to show me this: <img width="979" height="677" alt="image" src="https://github.com/user-attachments/assets/ff91cc9d-eb59-4cee-b06f-5758acd1bd5d" /> Given that our existing code checks first 1024 bytes to decide whether to bail on a binary file or not, this seems very wasteful — hence, adjusted the code to read the "header" and check that first, and only continue reading the entire file after the checks are successful. I suspect this should also help the project search, esp. the crashes and memory usage during that when many binary files are present? Release Notes: - Improved Zed's memory usage when attempting to open binary files
LivioGama
pushed a commit
to LivioGama/zed
that referenced
this pull request
Jan 20, 2026
Closes zed-industries#35894 Trace: https://drive.google.com/file/d/1pXDFzOg4ZS4p2SX8fk9Cjnyy1Qd1Pkrx/view?usp=sharing [Relevant part](https://github.com/user-attachments/files/24412472/image_leak.txt) Release Notes: - Fixed a memory leak when opening images
LivioGama
pushed a commit
to LivioGama/zed
that referenced
this pull request
Jan 20, 2026
…zed-industries#45971) When working on zed-industries#45969 I've noticed that whenever I try to open a binary file ``` ~/Desktop/svenska ❯ du -ha "Svenska А2B1. 07.09.2025 [u6qEIe9-COc].mkv" 456M Svenska А2B1. 07.09.2025 [u6qEIe9-COc].mkv ``` Zed allocates all its size <img width="214" height="104" alt="image" src="https://github.com/user-attachments/assets/e67ad522-b8a4-4e8e-9961-13030a34df1c" /> <img width="345" height="154" alt="image" src="https://github.com/user-attachments/assets/ea691020-0d02-4acc-88c3-476385f309bb" /> only to show me this: <img width="979" height="677" alt="image" src="https://github.com/user-attachments/assets/ff91cc9d-eb59-4cee-b06f-5758acd1bd5d" /> Given that our existing code checks first 1024 bytes to decide whether to bail on a binary file or not, this seems very wasteful — hence, adjusted the code to read the "header" and check that first, and only continue reading the entire file after the checks are successful. I suspect this should also help the project search, esp. the crashes and memory usage during that when many binary files are present? Release Notes: - Improved Zed's memory usage when attempting to open binary files
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #35894
Trace: https://drive.google.com/file/d/1pXDFzOg4ZS4p2SX8fk9Cjnyy1Qd1Pkrx/view?usp=sharing
Relevant part
Release Notes: