Skip to content

Conversation

@jjw24
Copy link
Member

@jjw24 jjw24 commented Aug 5, 2020

Problem:
Memory usage can run up quite high for Flow, and can hit 300 mbs for myself if constantly performing searches. It does reduce back down to around 130 when querying is stopped.

Note:
This is a partial fix to the memory issue, it does not address it complete, and other factors that contribute to it are still been investigated.

This PR does not mean memory will reduce significantly, it is more focused on addressing the unnecessary high memory caused by improperly handling of image caching. So reduce image cache (ImageSources)'s memory's footprint is the main focus here.

How to reproduce:
Open query, type 'a' (especially if Explorer hotkey is set as global '*') and you should get numerous results. Keep pressing down arrow on the keyboard and should see Flow.Launcher.exe in task manager grow to as high as 300mbs for myself. It may vary depending on the user and the application images that need to be displayed on the machine.

If unable to repro, change the power mode on Windows to 'Recommended' then try.

Partial solution:
Limiting the number of ImageSources cached to reduced the memory usage going over too high.

  • Set maximum number of images cached to 50 from 5000.

  • Once cached images go over permissible factor (2) X number of images (50), perform clean up by removing infrequently used ones from dictionary.

Testing

  1. Take snapshots of the memory usage by using the Diagnostic tools of Visual studio (Debug -> Windows -> Diagnostic tools).
  2. Take the first snapshot after all the dlls are loaded and the memory utilization is stabilized.
  3. Take the next memory snapshot after many queries (don't have to execute an application). The aim is to just load many icons.
  4. Compare the second snapshot by setting the first one as the baseline.
  5. Search for memory size diff in bytes for Concurrent Dictionary with value ImageSource.
  6. Compare this value to that obtained without the changes in this PR, to get the results as shown in the following images.

Result

  • Before this PR change:
    image

  • With this PR:
    image

Related: #128

@jjw24 jjw24 added the bug Something isn't working label Aug 5, 2020
@jjw24 jjw24 self-assigned this Aug 5, 2020
@jjw24 jjw24 marked this pull request as draft August 5, 2020 10:01
@jjw24 jjw24 marked this pull request as ready for review August 11, 2020 04:03
@jjw24 jjw24 requested a review from taooceros November 10, 2020 21:31
@taooceros
Copy link
Member

taooceros commented Nov 11, 2020

@bao-qian has introduced a way to lazy load image for resultlistview, which is a quite simple change, should I add that to this pr?

Comment on lines 14 to 15
public ConcurrentDictionary<string, int> Usage = new ConcurrentDictionary<string, int>();
private readonly ConcurrentDictionary<string, ImageSource> _data = new ConcurrentDictionary<string, ImageSource>();
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should merge this two dictionary to one by creating another utility class, which will allow us to remove element easier?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, let me have a look

Copy link
Member

Choose a reason for hiding this comment

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

I have finished that, if you want, I can add that to the pr.

Copy link
Member Author

Choose a reason for hiding this comment

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

haha ok cool, yes please

@jjw24
Copy link
Member Author

jjw24 commented Nov 11, 2020

@bao-qian has introduced a way to lazy load image for resultlistview, which is a quite simple change, should I add that to this pr?

Are you referring to the change in the other PR ?

@taooceros
Copy link
Member

@bao-qian has introduced a way to lazy load image for resultlistview, which is a quite simple change, should I add that to this pr?

Are you referring to the change in the other PR ?

Yes, it is part of commit in #195

@jjw24
Copy link
Member Author

jjw24 commented Nov 11, 2020

@bao-qian has introduced a way to lazy load image for resultlistview, which is a quite simple change, should I add that to this pr?

Are you referring to the change in the other PR ?

Yes, it is part of commit in #195

I think we can keep them separate and then just resolve any conflict depending which goes in first

@taooceros
Copy link
Member

Sure

@jjw24
Copy link
Member Author

jjw24 commented Nov 12, 2020

@taooceros new changes look good. Do you want to approve and merge it in?

@taooceros
Copy link
Member

Sure!

@taooceros taooceros merged commit 0ab7dc6 into dev Nov 12, 2020
@taooceros taooceros deleted the address_high_memory_issue branch November 12, 2020 03:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants