PowerToys icon indicating copy to clipboard operation
PowerToys copied to clipboard

[Peek] Fix memory leak caused by unmanaged bitmaps not being freed

Open daverayment opened this issue 1 year ago • 4 comments

Summary of the Pull Request

Fixes the Peek PowerToy leaking unmanaged resources associated with bitmaps when using the ImagePreviewer. Also ensures any Previewer implementing IDisposable will be correctly disposed.

PR Checklist

  • [x] Closes: #33593
  • [ ] Communication: I've discussed this with core contributors already. If work hasn't been agreed, this work might be rejected
  • [ ] Tests: Added/updated and all pass
  • [ ] Localization: All end user facing strings can be localized
  • [ ] Dev docs: Added/updated
  • [ ] New binaries: Added on the required places
  • [ ] Documentation updated: If checked, please file a pull request on our docs repo and link it here: #xxx

Detailed Description of the Pull Request / Additional comments

The Peek PowerToy creates a Previewer for each item as the user navigates a folder. For the ImagePreviewer, a Clear method is provided which cleans up unmanaged resources, including bitmaps created for preview thumbnails. Unfortunately, Clear is not called on the old Previewer before a new instance is created, leading to unreachable resources which are never freed. This PR ensures Dispose is called on a Previewer if it implements IDisposable. ImagePreviewer calls Clear within its Dispose, which fixes the leak.

Validation Steps Performed

Manual validation:

  1. Attached to the PowerToys.Peek.UI exe in VS, where the memory leak manifests.
  2. Created a Heap Snapshot.
  3. Manually navigated through a test folder, which contains 90% image files, and also zip files for several minutes.
  4. Created another Heap Snapshot.
  5. Confirmed that the Process Memory used was stable.
  6. Confirmed that only a small number of pinned Byte[] instances were present in the heap snapshot.
  7. Left the program running for more than an hour as I did other work. Confirmed that it still ran without issue and memory issues had not resurfaced.

Before Fix

Despite garbage collection, unmanaged resources are not freed, leading to an ever-increasing amount of memory used - nearly 2GB in this example.

Screenshot 2024-08-28 172800

After Fix

image

daverayment avatar Aug 29 '24 15:08 daverayment

@microsoft-github-policy-service agree

daverayment avatar Aug 29 '24 15:08 daverayment

Thank you for approving the merge, @drawbyperpetual !

I can speak to the strange behaviour you were seeing previously, where the increase in private bytes used was not as prevalent when the same files were previewed again: this could be because of the thumbnail retrieval code in Peek and the relationship with the Windows thumbnail cache. You are 'priming' the cache by previewing the images, as Peek always calls the Shell thumbnail retrieval/creation routine for every image, even when the full size image loads correctly. This was the fundamental 'tell' of the memory leak issue, as the thumbnail bitmaps are unmanaged and never freed. I've recently raised an issue about the unnecessary thumbnail generation as #34490 . In the majority of cases in my testing, the bitmaps are never used, so the #34490 fix actually saves a considerable amount of memory use and subsequent garbage collection (75%+ GC reduction).

I totally agree about the existing Dispose code, too. I'm happy to do that update in a separate PR to match the recommended pattern and ensure idempotency.

daverayment avatar Aug 30 '24 17:08 daverayment

@daverayment as FYI, we've been heads down trying to get ship .84 and stabilize based on feedback. We appreciate the PRs here!

crutkas avatar Sep 07 '24 05:09 crutkas

@crutkas Thanks! It's a privilege to be able to contribute, and there's certainly no rush. Best of luck with the upcoming release 😀

daverayment avatar Sep 08 '24 16:09 daverayment

Thanks a lot for the contribution! Merging this in!

jaimecbernardo avatar Sep 16 '24 20:09 jaimecbernardo