bayleeadamoss / zazu-file-finder

A file finder for Zazu.
18 stars 11 forks source link

Cache mdfind apps for faster searches. #31

Closed bayleedev closed 7 years ago

bayleedev commented 7 years ago

Locally I can get results in ~100ms, but others have reported results around ~2000ms. This resolves that issue by caching the applications for faster lookups.

bayleedev commented 7 years ago

@twang2218 can you have time to take a look at this?

amjith commented 7 years ago

🎖

amjith commented 7 years ago

So regardless of the cache, it is still slow. 😢

I just tried it on my box. The cache had no effect.

amjith commented 7 years ago

Nevermind, it took a few minutes to cache. Now everything is fast. 😃

twang2218 commented 7 years ago

I'm considering moving the cache to higher layer, so all adapters will be benefited the caching mechanism.

About the fuzzyfind, should we replace the filterSort in fallback with fuzzyfind as well?

And about the cacheIcons():

  cacheIcons (apps) {
    return Promise.all(apps
      .filter((file) => !file.hasIcon())
      .slice(0, 20)
      .map(file => file.generateIcon()))
  }

Why only generate 20 icons at a time? It will take several minutes to generate all the icons if the system have a lot apps, which will cost dozens cycles, and each cycle is 30s now.

afaur commented 7 years ago

I'm considering moving the cache to higher layer, so all adapters will be benefited the caching mechanism.

This is a great idea!

About the fuzzyfind, should we replace the filterSort in fallback with fuzzyfind as well?

Ideally yes. It would make it more consistent.

Why only generate 20 icons at a time?

The speed of the file finder is the most common complaint about Zazu. This was just to make sure we didn't overdo somebody's computer. It might be fun to try and make batches for ~20s, but since it's async it would be hard to limit.

It only took ~5s to generate all the icons on my computer. One possibility is to time the first 20 icons, and change the next batch based on that time.

each cycle is 30s now.

It was 30s before, but it was incorrectly comitted

twang2218 commented 7 years ago

To limit the performance impact, how about using async.queue(), just like my previous PR (https://github.com/tinytacoteam/zazu-file-finder/pull/24): https://github.com/twang2218/zazu-file-finder/blob/8e36d824dc6b3a17fec0517bf543cb2da696c58d/lib/appInfo.js#L43

const iconutilQueue = async.queue(iconutil.toIconset, 20)
...
iconutilQueue.push(icnsPath, (err, icons) => { ... })

It will limit the concurrent async tasks to a certain number, but also generate all the icons in one batch.

And about the AppCache cycle period, it is 30 seconds now, https://github.com/tinytacoteam/zazu-file-finder/blob/master/zazu.json#L11:

      {
        "id": "AppCache",
        "type": "ServiceScript",
        "script": "appCache.js",
        "interval": 30000
      }

And I think 30 seconds is a reasonable time period, as apps list will not be changed that often, even a little bit longer should be ok. It was 1 second once, which makes me a little bit worrying the CPU impact.

bayleedev commented 7 years ago

The 1s interval was a mistake that got in on accident ]:

What the advantage of doing ~20 concurrent vs 20 per interval? I assume the concurrent ones with async would be faster, but ideally we want to be less cpu intensive. Maybe we should try and do a few tests.

twang2218 commented 7 years ago

Without the concurrency limitation, such as Promise.all(), the async tasks will be started as many as it can, say we have 200 tasks, all of them will be started at almost the same time, which will have a significant impact on the CPU/performance, and it can also cause race condition problems sometimes.

With the concurrency limitation, only limited tasks can be executed at the same time, which will reduce the CPU impact, and it will also guarantee all the tasks will be executed finally.

20 per interval will also make sure it will not execute more than 20 tasks at a time, however, it will only execute 20 tasks at this cycle, and left the rest time of the interval to be idle. If we have 200 apps, it will take 10 cycles to fully generate the icons, which means we don't have many app icons when we search the apps until 5 minutes later.

Massively generating apps icons should be almost one-time job, only happened when run this plugin in the first time, it should be ok to generate them in one batch. And, to limit the performance impact, I think applying concurrency limitation should be good enough. After first time icons generation cycle, the rest cycles should be quite lightweight.