Skip to content

Conversation

@allenhumphreys
Copy link
Collaborator

@allenhumphreys allenhumphreys commented Aug 19, 2025

I've pulled these out of #729 to make it easier to focus on them. They're very high priority because the state of the master branch right now is very bad with the memory leaks in particular.

ImageDownloadCenter:

  • Sendable
  • async API
  • Cache retrievals are now non-blocking by default
  • Fixed a problem with URLSession misuse

Related Sessions:

  • De-duplication by id (SwiftUI warnings from multiple items with the same id)
  • Retain view models correctly (Better UI performance, not incorrectly displayed info)

Various:

  • Use weakAssign to avoid retain cycles on cells (Currently, in master this causes a MASSIVE memory leak when scrolling the sessions)

PUIButtonView:

  • Fix State vs StateObject oopsie (Leaking PUIButton instances currently, which are fairly small, but still)
Screen.Recording.2025-08-21.at.10.16.01.AM.mov

@allenhumphreys allenhumphreys force-pushed the async-image-download branch 3 times, most recently from e023ac8 to cf796ae Compare August 20, 2025 17:54
@allenhumphreys allenhumphreys marked this pull request as ready for review August 20, 2025 17:57
@allenhumphreys
Copy link
Collaborator Author

Depends on #732 being merged first.

@allenhumphreys allenhumphreys marked this pull request as draft August 20, 2025 19:49
@allenhumphreys allenhumphreys force-pushed the async-image-download branch 2 times, most recently from 85b6dde to 2b59a7f Compare August 21, 2025 14:15
if let thumbnailImage = cache.cachedImage(for: url, thumbnailOnly: true).thumbnail {
completion(url, (nil, thumbnailImage))
return nil
func downloadImage(from url: URL, thumbnailHeight: CGFloat, thumbnailOnly: Bool = false, completion: @escaping ImageDownloadCompletionBlock) -> Operation {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Re-structuring this to always return an Operation is motivated by filesystem accesses for cached images blocking the main thread. This has been causing the very choppy scrolling of the sessions list.

let operation = ImageRetrievalOperation(url: url, cache: cache, cacheArguments: .init(thumbnailHeight: thumbnailHeight, thumbnailOnly: thumbnailOnly), completion: completion)

pendingOperation.addCompletionHandler(with: completion)
operation.addDependency(pendingOperation)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Switching from addCompletionHandler to using Operation dependencies is all about correctness.

In the context of cached images, 2 images requests may have different cache arguments. Specifically, thumbnail size and thumbnail only. When cache retrieval was moved into the Operation, this fact became even more problematic. Although technically the thumbnail size was already an input to the operation, which means the addCompletionHandler was already incorrect.

The dependency system also simplifies the logic around image loading from remote vs cache, etc. As long as you guarantee the dependencies are applied in the order the requests for images come in, there will only ever be a single web load, followed by dependents retrieving it from cache.

@allenhumphreys allenhumphreys marked this pull request as ready for review August 21, 2025 16:11
@allenhumphreys allenhumphreys merged commit 4c291be into master Aug 21, 2025
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.

3 participants