Bug 78132 - [WK2] Performance issue in FindController::findString
Summary: [WK2] Performance issue in FindController::findString
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sergio Villar Senin
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-02-08 10:45 PST by Sergio Villar Senin
Modified: 2012-07-12 01:38 PDT (History)
6 users (show)

See Also:


Attachments
Tentative patch (6.17 KB, patch)
2012-03-02 00:27 PST, Sergio Villar Senin
andersca: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sergio Villar Senin 2012-02-08 10:45:20 PST
Search for the previous or next occurrence of a given string is a quite common operation in a Web browser (or any other WebKit embedder). If I'm not wrong this operation is currently heavier than it should be in WK2. That's because FindController::findString unconditionally calls unmarkAllTextMatches(). So, if FindOptionsShowOverlay is passed (or FindOptionsShowHighlight once http://webkit.org/b/76522 lands), the ::findString() method will _always_ unmarkAllTextMatches() and markAllTextMatchesForText(). That's fine if the text to search is different, but if we're doing search_next(), search_previous() kind of operations, i.e. the string to search is the same, we will be unnecessary unmarking and marking all text matches everytime we look for the next/previous result.
Comment 1 Sergio Villar Senin 2012-02-08 11:37:53 PST
As far as I see this, if it's considered an issue, the only way to fix this would be to save the last find operation parameters in the FindController to avoid unmarkAll+markAll if the following find operation has the same string, options and matchCount. Obviously that state will have to be cleared in ::hideFindUI.
Comment 2 Sergio Villar Senin 2012-03-02 00:27:48 PST
Created attachment 129838 [details]
Tentative patch
Comment 3 WebKit Review Bot 2012-03-02 00:31:59 PST
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Comment 4 Sergio Villar Senin 2012-03-02 00:33:47 PST
So the approach I followed is:

1) unmarkAllTextMatches() is not called by default
2) it'd be called from FindController whenever:
   a) the string is not found
   b) we're going to call markAllTextMatches()
3) clients could perform searchNext()/searchPrevious() kind of operations by just calling FindString with the same arguments BUT removing the showOverlay & showHighlight options from the findOptions.

PS: I know it might require a test but just wanted to confirm that the approach is correct.
PS2: didn't find any usage of WebPageProxy::findString in the other ports... sounds weird
Comment 5 Carlos Garcia Campos 2012-03-02 08:05:31 PST
Comment on attachment 129838 [details]
Tentative patch

Looks good to me
Comment 6 Tim Horton 2012-03-06 22:52:39 PST
Comment on attachment 129838 [details]
Tentative patch

View in context: https://bugs.webkit.org/attachment.cgi?id=129838&action=review

> Source/WebKit2/UIProcess/API/gtk/WebKitFindController.cpp:357
> +        // highlighting. Since most of them (all?) where using that

s/where/were/? Though I see you're just moving the comment, might as well fix it while you're here.
Comment 7 Sergio Villar Senin 2012-03-08 10:07:25 PST
(In reply to comment #6)
> (From update of attachment 129838 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=129838&action=review
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitFindController.cpp:357
> > +        // highlighting. Since most of them (all?) where using that
> 
> s/where/were/? Though I see you're just moving the comment, might as well fix it while you're here.

Sure I will. Anyone willing to review the patch? Or any comment to the approach I followed?
Comment 8 Sergio Villar Senin 2012-07-06 00:27:44 PDT
This patch is 4 months old. Any comment or review?
Comment 9 WebKit Review Bot 2012-07-10 01:47:23 PDT
Comment on attachment 129838 [details]
Tentative patch

Rejecting attachment 129838 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
 Source/WebKit2/UIProcess/API/gtk/WebKitFindController.cpp
patching file Source/WebKit2/WebProcess/WebPage/FindController.cpp
Hunk #1 succeeded at 152 with fuzz 1 (offset 61 lines).
Hunk #2 FAILED at 98.
Hunk #3 FAILED at 115.
2 out of 3 hunks FAILED -- saving rejects to file Source/WebKit2/WebProcess/WebPage/FindController.cpp.rej

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Anders Car..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue/

Full output: http://queues.webkit.org/results/13180043
Comment 10 Carlos Garcia Campos 2012-07-12 01:38:36 PDT
Committed r122428: <http://trac.webkit.org/changeset/122428>