GNOME Bugzilla – Bug 667569
GUI progress update during text search causes slowdown
Last modified: 2013-03-04 19:46:03 UTC
during text search, every time a page has been searched, a signal is emitted. this signal causes the GUI to update the search progress. this is slow. the attached patch changes this behavior, to only emit the signal for every 25th frame. for this document http://www.itk.org/ItkSoftwareGuide.pdf the search duration has been decreased from 12.5 to 5.5 seconds.
Created attachment 204869 [details] [review] increase search speed by updating the GUI less often
If you want to update the GUI less often move the code to ev_window_find_job_updated_cb. Others views using EvJobFind might require more precision.
*** Bug 672900 has been marked as a duplicate of this bug. ***
Created attachment 237577 [details] [review] shell: increase search performance. Updated patch, this is applied to ev-window.c. The outcome is the same than the previous patch, but this applies only to the GUI.
*** Bug 694836 has been marked as a duplicate of this bug. ***
Review of attachment 237577 [details] [review]: Thanks! ::: shell/ev-window.c @@ +5238,3 @@ EvWindow *ev_window) { + if(job->current_page % (gint)((job->n_pages/100)+1) == 0) { This looks a bit cryptic to me, could you add a coment explaining what it does, or even create a inline function to give it a descriptive name? Also, fix the coding style, please: if (job->current_page % (gint)((job->n_pages / 100) + 1) == 0)
(In reply to comment #6) > [...] > This looks a bit cryptic to me, could you add a coment explaining what it does, > or even create a inline function to give it a descriptive name? Also, fix the > coding style, please: > > if (job->current_page % (gint)((job->n_pages / 100) + 1) == 0) Before update the patch, I will explain the line and I will add some context: The line gets a factor relative to the document size and update the GUI every certain number of pages. For instance, the PDF reference 1.7 has 1310 pages. So, the factor will be 14 and the GUI will be updated on pages multiple of 14. The GUI should be updated 93 times (1310 / 14) For documents with 100 pages or less, the GUI is updated for every page. For documents between 101 and 200 pages, the GUI will be updated every other page, between 201 and 300, the GUI will be updated every 3 pages, and so on. FWIW, the signal 'updated' is triggered several times in pages lower than 30. This is exacerbated when the GUI is updated every time. The patch is not deterministic, but it improves the performance. In a quick measured I got: With the patch: 108 updates of 93 expected 109 updates of 91 expected Without the patch: 1395 updates of 1310 expected 1417 updates of 1310 expected Also, the GUI only updates sensitivity and percentage of remaining pages to search. The latter is used as integer, therefore, it does not makes sense to update it for decimals produced in documents with more than 100 pages. Even more, it can be argues that is does not need to be updated per every 1% of increment (only if the find process becomes too slow).
(In reply to comment #7) > (In reply to comment #6) > The patch is not deterministic, but it improves the performance. In a quick > measured I got: > > With the patch: > 108 updates of 93 expected > 109 updates of 91 expected > > Without the patch: > 1395 updates of 1310 expected > 1417 updates of 1310 expected This part is wrong. I did not realize it happens because evince restarted the search per character I was entering the entry. To minimize this, I copied the text from another place to the entry, and it is done per page (93, and 1310).
Created attachment 237684 [details] [review] shell: increase search performance. Updated patch. This addresses the comment, except that did not define the function as inline.
Created attachment 237685 [details] [review] shell: update eggfindbar label only if the text changes This is an extra patch, that checks if the text of the label in eggfindbar is changed before updating it. This would prevent (or reduce) future performance issues if other function makes burst updates.
I forgot to mention, egg_find_bar_set_status_text() turned out to be the expensive operation, in particular, gtk_label_set_text(). The constant progress status makes the search almost 2x slower, which start to be notorious in big documents. I do think the first patch goes in the right direction, because it does not try to update when there is no meaningful data to update (i.e. calling to update for 1, 1.1, 1.2, 1.3, etc. percent when in all those case it will be shown '1%' anyway). In other big documents, the process can be slowed down by the rendering (the search could work, but evince will not show progress until the render of the current page finishes), like the documents pointed in https://bugzilla.gnome.org/show_bug.cgi?id=635449. For less heavy documents, this patch works well.
Review of attachment 237685 [details] [review]: LGTM ::: shell/eggfindbar.c @@ +857,3 @@ + if (g_strcmp0 (current_text, text) != 0) { + gtk_label_set_text (GTK_LABEL (priv->status_label), text); + } Please don't use braces for single statement ifs
Review of attachment 237684 [details] [review]: Thanks, feel free to push, but please consider my minor comments below. ::: shell/ev-window.c @@ +5238,3 @@ + */ +static gboolean +find_bar_check_refresh_rate (EvJobFind *job, gint page_rate) This is in ev-window and also the window ui is updated, so I would avoid suing find_bar, maybe just find_check_refresh_rate? @@ +5251,3 @@ + to the document size in pages. For documents smaller (or equal) + than 100 pages, it will be updated in every page. 100 also + is enough to update the find bar every 1%. */ Please, use the following format for multiline comments: /* * */ @@ +5252,3 @@ + than 100 pages, it will be updated in every page. 100 also + is enough to update the find bar every 1%. */ + if (find_bar_check_refresh_rate (job, 100)) { Don't use magic numbers, give a name to the 100 using a macro
(In reply to comment #9) > Created an attachment (id=237684) [details] [review] > shell: increase search performance. > > Updated patch. This addresses the comment, except that did not define the > function as inline. why?
(In reply to comment #14) > (In reply to comment #9) > > Created an attachment (id=237684) [details] [review] [details] [review] > > shell: increase search performance. > > > > Updated patch. This addresses the comment, except that did not define the > > function as inline. > > why? A couple of reasons: 1. I did not find any other inline definition in evince. 2. I did not find any difference in performance. I just re-checked for (1) and there are some 7. It seems I did not pay enough attention. I am going to change it.
Created attachment 238037 [details] [review] shell: increase search performance.
Created attachment 238038 [details] [review] shell: update eggfindbar label only if the text changes