Skip to content

Conversation

@taooceros
Copy link
Member

@taooceros taooceros commented Jan 19, 2021

fix #299
Also using EnumerationOption to avoid one access denied location breaking the whole searching similar to #209 .

Outcome:

  • Improved Explorer's handling of long running results
  • Enhanced result list update speed

@jjw24
Copy link
Member

jjw24 commented Jan 19, 2021

Ok so found the bottleneck that causes query to run very long from explorer:

image

when you have a lot of results like below, and it's trying to add new results (called from updateresultview) in after using except to exclude the same results currently display, this operation is very costly:
image

if you let it just display the new results, blazing fast

@taooceros
Copy link
Member Author

That seems what #195 is trying to do. You can take a look on the same method in that pr.

@jjw24
Copy link
Member

jjw24 commented Jan 19, 2021

Ok cool, yeah I haven't looked at the code yet, just being taking it for test drives, will do so very soon

@taooceros
Copy link
Member Author

I think enumerate all file including subdirectory for about 200000 results may take about 5s and 200MB memory, which is still costly. This should still be added with an cancellation token.

@taooceros
Copy link
Member Author

taooceros commented Jan 19, 2021

Ok so found the bottleneck that causes query to run very long from explorer:

if you let it just display the new results, blazing fast

image

Also, due to the lock strategy, by doing huge calculation within NewResutls method will hold the lock, which blocks ongoing results view update.

@jjw24
Copy link
Member

jjw24 commented Jan 19, 2021

agree and agree, wonder if there is a better way to do this. also maybe if we want to do it in separate pr so cancellation is not blocked by this other fix

@taooceros
Copy link
Member Author

agree and agree, wonder if there is a better way to do this. also maybe if we want to do it in separate pr so cancellation is not blocked by this other fix

That's already exist in #195, so no worry!

@taooceros taooceros marked this pull request as ready for review January 20, 2021 05:50
@taooceros
Copy link
Member Author

taooceros commented Jan 20, 2021

I also fix some code that is used for testing in #195, which isn't a big deal.

@taooceros
Copy link
Member Author

After merging the dev, the recursive search for 180000 result takes about 5s to finish, which is significantly faster than before.

@taooceros
Copy link
Member Author

From testing, seems that the large amount of result won't be collected by GC even after a few other queries. By changing the List capacity, it should show a memory lower down but seems no effect....
Not sure whether my testing is wrong.

@taooceros
Copy link
Member Author

Unintentionally add a test code in PublicAPIInstance due to #300, should remove it before merging.

@jjw24 jjw24 added the enhancement New feature or request label Jan 21, 2021
@jjw24
Copy link
Member

jjw24 commented Jan 21, 2021

Unintentionally add a test code in PublicAPIInstance due to #300, should remove it before merging.

this one?

  Application.Current.Dispatcher.Invoke(() =>
            {
                useMainWindowAsOwner = false; <=============
                var msg = useMainWindowAsOwner ? new Msg { Owner = Application.Current.MainWindow } : new Msg();
                msg.Show(title, subTitle, iconPath);
            });

@taooceros
Copy link
Member Author

Unintentionally add a test code in PublicAPIInstance due to #300, should remove it before merging.

this one?

  Application.Current.Dispatcher.Invoke(() =>
            {
                useMainWindowAsOwner = false; <=============
                var msg = useMainWindowAsOwner ? new Msg { Owner = Application.Current.MainWindow } : new Msg();
                msg.Show(title, subTitle, iconPath);
            });

Yes

@jjw24
Copy link
Member

jjw24 commented Jan 21, 2021

getting this error ,

repro:
set explorer hotkey to 'e', type 'e a'. first couple of characters gets this error

image

@jjw24
Copy link
Member

jjw24 commented Jan 21, 2021

open flow's settings get this:
image

@taooceros
Copy link
Member Author

open flow's settings get this:
image

Forget to add null check😅, will do now.

@taooceros
Copy link
Member Author

getting this error ,
repro: set explorer hotkey to 'e', type 'e a'. first couple of characters gets this error

I cannot reproduce the error....Could you please provide the stacktrace for that error?

@jjw24
Copy link
Member

jjw24 commented Jan 22, 2021

getting this error ,
repro: set explorer hotkey to 'e', type 'e a'. first couple of characters gets this error

I cannot reproduce the error....Could you please provide the stacktrace for that error?

This maybe related to the null check, will check again later

@taooceros
Copy link
Member Author

@jjw24 Should we try to fix #308 here or use a new pr to fix it?

@jjw24
Copy link
Member

jjw24 commented Jan 22, 2021

Separate pr please

@taooceros
Copy link
Member Author

Separate pr please

Sure

@taooceros taooceros added this to the 1.7.0 milestone Jan 23, 2021
@taooceros taooceros requested a review from jjw24 January 24, 2021 21:34
@jjw24
Copy link
Member

jjw24 commented Jan 25, 2021

getting this error ,

repro:
set explorer hotkey to 'e', type 'e a'. first couple of characters gets this error

image

yeah ok still getting this error. Set search activation hotkey to 'e' and just type 'e' i got it.

@jjw24
Copy link
Member

jjw24 commented Jan 25, 2021

sorted in ff5e369

@taooceros
Copy link
Member Author

sorted in ff5e369

Nice! I understand that why I am unable to reproduce, because I have a few quick access.

@jjw24
Copy link
Member

jjw24 commented Jan 25, 2021

yeah haha, i should have said remove roaming profile

@taooceros
Copy link
Member Author

yeah haha, i should have said remove roaming profile

Hahaha 🤣

@taooceros
Copy link
Member Author

9914124 remove the extra checking to make the code clear. We don't need to check whether the quick folder list is empty and don't need to check the action keyword as what we have talked about in Window Walker plugin.

@taooceros
Copy link
Member Author

@jjw24 Any further change required?

UpdateResults(newResults);
}
/// <summary>
/// To avoid deadlock, this method should not called from main thread
Copy link
Member

Choose a reason for hiding this comment

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

@taooceros do you know if this comment is still relevant?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think yes, because only results from main result set will be added via resultUpdateQueue, the one for context menu and history is still added directly.
However, I am not sure the deadlock meaning here. Maybe just avoiding potential stunt of main ui due to lock machanism.

Copy link
Member

Choose a reason for hiding this comment

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

cool, thank you

@jjw24 jjw24 merged commit 5dd12d2 into Flow-Launcher:dev Jan 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Use cancellation with Explorer's DirectorySearch

2 participants