kiwix / kiwix-android

Kiwix for Android
https://android.kiwix.org
GNU General Public License v3.0
861 stars 443 forks source link

Very slow search function #2082

Closed roydok closed 3 years ago

roydok commented 4 years ago

Describe the bug Slow search function. Takesna minute or 2 and sometimes never for the search results to come out.

Expected behavior Quick search suggestion and results come out lile in the older 2.5 version

Steps to reproduce the behavior:

  1. Upgrade to the latest version 3.0 and above
  2. Use search
  3. Slow to no results

Environment

kelson42 commented 4 years ago

@macgills Thank you for the feedback. Once you will have completed the CPU trace analysis, let us know here about the next steps and if you need anything. This is a serious problem.

kelson42 commented 4 years ago

@mgautierfr Might that be that https://github.com/kiwix/kiwix-lib/pull/374 is somehow related to this problem?

mgautierfr commented 4 years ago

Yes and no :)

The search api exposed by kiwix-lib to android is not thread safe indeed. The searchSuggestions use a internal vector to store the result and if you do several search in the same time you will get wrong (mixed) results. We may even have crash because of two thread modified the vector at the same time and scrambling the memory.

But it will not help to have a async search api nor to speed up the search process.

kelson42 commented 4 years ago

@mgautierfr AFAIK, the only way to provide a good experience of suggestion search is to use multithreading? You confirm?

mgautierfr commented 4 years ago

Yes. We may have good surprise on the search code itself, but from what I know, we should try to move this way. This should not be made on android however. As @macgills suggests, it would be to kiwixlib/libzim to provide a async api. And user code (anrdoid, desktop, serve..) will have to adapt to take advantage of this api (incremental displaying of the results...)

kelson42 commented 4 years ago

@mgautierfr Do we have a ticket for that already?

mgautierfr commented 4 years ago

No

kelson42 commented 4 years ago

@mgautierfr @macgills OK, so we need one and I don't want to release any new version of Kiwix Android without having this bug fixed and the suggestion working properly. Beside the basic ZIM opening and rendering, I hardly see something more important than the suggestions system. As this problem/strong limitation seems to have been understood earlier I would have appreciate to see a ticket already open. It is not optimal to wait to get full users impact before doing something.

stesee commented 4 years ago

An other thing that would be a enhancemant would be ui feedback that something happens while searching. Even when it takes some time, this would tell the user that there should be search result anytime.... in the first run I thought, that kiwix had no search hit, before I found this issue... would be a quick win too.

kelson42 commented 4 years ago

@stesee Indeed, I can not reproduce the problem, but another user impacted by the problem reported that it is written "no results" whereas it shouldl be maybe written "please wait" or "searching..." as long a the search is not over.

mgautierfr commented 4 years ago

OK, so we need one and I don't want to release any new version of Kiwix Android without having this bug fixed and the suggestion working properly.

I don't know what you're calling "suggestion working properly" but if you think about having a async api I hope you are not hurry to make a new release of kiwix-android. There is nothing async at all in libzim/kiwix-lib for now.

macgills commented 3 years ago

@dbedrenko it has been a long time coming but can you test on this https://drive.google.com/file/d/10LPnhWsW11lwTQGbLTKBeJ4HkZXwSxrn/view?usp=sharing ?

I am about to open a PR after a complete overhaul of the internals of this screen

kelson42 commented 3 years ago

@macgills It is a bit unfortunate that we don't have had any transparent exchange here between @mgautierfr and you on the technical approach to solve the problem. I hope at least you made a clear analysis of the problem together and came on a plan about what to do in a private chat.

On my side I discussed the topic in depth with @mgautierfr and clearly came to the conclusion that as long as the Kiwix for Android suggestion system is not able to handle multiple suggestion threads then the usability of the whole suggestion system usability will be challenged. The reason being that you have only one thread and you can not kill it to start a new search, so you have to wait it finishes before starting over a new one (so the user as well... waits).

macgills commented 3 years ago

So this ticket introduces a loading UI and speed increases, it isn't going to "fix" the problem but it is as much as we can do

kelson42 commented 3 years ago

@macgills What is missing from your point of view to fix the problem? Do you agree with my conclusion that Kiwix Android needs to open many threads to makes suggestions (if not why)? Have you introduced a delay beetween search requests so a user typing quickly 5 letters does not make 5 suggestions searches?

macgills commented 3 years ago

We are now using coroutines, the concurrency primitive of kotlin, which are way more easily cancellable, when a load is in progress we display a spinner (we weren't previously because I was under the impression loads happened in milliseconds). Right now I believe it to be a good usability improvement. If @dbedrenko can confirm that I would be grateful as I cannot replicate their scenario locally with my devices but they have done enough over the course of this ticket.

To really fix this problem then kiwixlib would have to have an interruptable API but that doesn't seem likely as that would mean a rewrite of an external API. Even then I don't think it would be very effective as the majority of time is spent locating the xapian index which must be done anyways?

We could try empty searches whenever a user opens a book and hope to warm it up ahead of a search?

I can't remember everything we have tried over the past 4 months

kelson42 commented 3 years ago

We are now using coroutines, the concurrency primitive of kotlin, which are way more easily cancellable, when a load is in progress we display a spinner (we weren't previously because I was under the impression loads happened in milliseconds). Right now I believe it to be a good usability improvement. If @dbedrenko can confirm that I would be grateful as I cannot replicate their scenario locally with my devices but they have done enough over the course of this ticket.

The spinner display is good, but this is only a communication thing... it does not solve the search speeed problem.

The Kotlin coroutines might solve the problem, I can not judge that as I don't know the technical background of them.

That said, if the user is happy, this is all good to me.

To really fix this problem then kiwixlib would have to have an interruptable API but that doesn't seem likely as that would mean a rewrite of an external API. Even then I don't think it would be very effective as the majority of time is spent locating the xapian index which must be done anyways?

@mgautierfr You agree with that? To me I remember you said, that thread being created at Kotlin level should be interrupted at Kotling level. If you disagree, please do the necessary with @macgills To have a full written agreement of what needs to be done to solve properly this ticket.

We could try empty searches whenever a user opens a book and hope to warm it up ahead of a search?

It seems indeed something has to be done here, this is one of the conclusions of https://github.com/openzim/libzim/issues/418 as well. Pretty sure nothing should be done at Kiwix-Android level (at least not now). This is more something at a lower level IMO.

mgautierfr commented 3 years ago

To really fix this problem then kiwixlib would have to have an interruptable API but that doesn't seem likely as that would mean a rewrite of an external API. Even then I don't think it would be very effective as the majority of time is spent locating the xapian index which must be done anyways?

I agree here. It will not really be possible to interrupt the search usefully. There is only one xapian function taking a lot of time. Libzim don't have the hand on this and cannot interrupt the long step. The only real thing to do is to do the search in a different thread. This way we can still answer ui event (and tell user a search is going one). If we don't want the search result (because user enter another query) we have to launch another thread and discard result of the previous search. (Ideally we would kill the previous search and launch another one, but killing a thread could lead to memory leak and inconsistent state).

The question is where we create this thread. For now it is to the user to create this thread and handle this correctly. Creating it on the libzim level is pretty complex has we have to adapt the whole api to have async api. (Not speaking about the wrapping). It may come in the future but I don't thing it will come soon.

Coroutine works pretty well with cooperative "tasks" (where the tasks regularly give back control to the "tasks mananger" and allow other tasks to run). This is not specific to kotlin, this is specific to coroutine. Without a async api, it will not be possible to have cooperative search task.

To me I remember you said, that thread being created at Kotlin level should be interrupted at Kotling level

I don't remember I said that but I totally agree. Thread created by one "system" but be handle by this "system".


To conclude, I think there will be no real solution coming from libzim/kiwix-lib soon. The best way to solve this is to use (several) thread to do the search.

macgills commented 3 years ago

The best way to solve this is to use (several) thread to do the search.

Which is what the updated solution does and in fairness what the original solution did but much less cooperatively because of the nature of RxJava's implementation of Reactive Streams. We will always have problems on device with low thread counts that search on an sd card

The idea of the "interuptable" xapian function is quite unpalatable but would work from the app side like so

searchSuggestions(etc) { //this function gets executed at intervals in a task and would cancel my coroutine }

but the xapian function would need to be rewritten and I don't think you can pass lambdas across JNI for starters? It is not what we want to do

mgautierfr commented 3 years ago

Which is what the updated solution does and in fairness what the original solution did but much less cooperatively because of the nature of RxJava's implementation of Reactive Streams

This is something I cannot comment :)

We will always have problems on device with low thread counts that search on an sd card

I agree here. If the device need time to get the data on the sdcard, it will takes time. What we can do is to ensure the application do not freeze. And maybe do a bit of warmup to start loading data before the user needs it.

The idea of the "interuptable" xapian function is quite unpalatable but would work from the app side like so

searchSuggestions(etc) { //this function gets executed at intervals in a task and would cancel my coroutine }

but the xapian function would need to be rewritten and I don't think you can pass lambdas across JNI for starters? It is not what we want to do

I don't really understand that. But there we(I) will not rewrite xapian methods. This is an external library we are using. A cooperative task/api means that we give back control when we are waiting for data (io). Here we are waiting for xapian to run. On higher level everything run on the same thread but on lower level it means that we use thread to do the search while we give back control on main thread. I suppose the low thread counts is low whatever the code using threads. If this is too low for kotlin, it will be too low for cpp also.

Beater34 commented 3 years ago

Thank you all for doing so much work to fix the slow search problem. I had slow searches with Kiwix 3.3.3 on a Motorola G6 Plus phone with wikipedia_en_all_maxi_2020-06.zim on an SD card (see my post of Jul 8, above). I now have Kiwix 3.4.0 build 6230400, but searches still take 1-2 minutes, and sometimes selecting one of the search results gets no response and I have to wait another minute or so until the selected article opens (or I restart Kiwix). Please let me know if I can provide any more information that would be helpful.

macgills commented 3 years ago

Please await Kiwix 3.4.1's release and if you still experience these issues then opening a new ticket would be best

Wikinaut commented 3 years ago

I built and installed the current develop branch be91e6ba6 and found that the search time in the big enwiki file (80 GB) went down from 1..2 minutes (beta release 3.3.4 or so) to 10..20 seconds. ("external" SD Card 256GB on HUAWEI P Smart 2019)

Wikinaut commented 3 years ago

So from my side: Go on!

kelson42 commented 3 years ago

@Wikinaut It is a good news, but still 10 times too slow. Hopefullz 3.4.1 will solve the problem.

Wikinaut commented 3 years ago

Yes, I was still disappointed, because I thought, the develop has that built in, hasn't it?

kelson42 commented 3 years ago

@Wikinaut Yes it does, you are right. We will release 3.4.1 and then probably have to start a new round of investigation/fix to come down to reasonable times even in external SD cards worse scenarios.