78132 2012-02-08 10:45:20 -0800 [WK2] Performance issue in FindController::findString 2012-07-12 01:38:36 -0700 1 1 1 Unclassified WebKit WebKit2 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 svillar svillar andersca cgarcia darin gustavo svillar webkit.review.bot oldest_to_newest 552283 0 svillar 2012-02-08 10:45:20 -0800 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. 552329 1 svillar 2012-02-08 11:37:53 -0800 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. 569375 2 129838 svillar 2012-03-02 00:27:48 -0800 Created attachment 129838 Tentative patch 569379 3 webkit.review.bot 2012-03-02 00:31:59 -0800 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 569380 4 svillar 2012-03-02 00:33:47 -0800 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 569657 5 129838 cgarcia 2012-03-02 08:05:31 -0800 Comment on attachment 129838 Tentative patch Looks good to me 572552 6 129838 thorton 2012-03-06 22:52:39 -0800 Comment on attachment 129838 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. 573901 7 svillar 2012-03-08 10:07:25 -0800 (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? 663920 8 svillar 2012-07-06 00:27:44 -0700 This patch is 4 months old. Any comment or review? 665570 9 129838 webkit.review.bot 2012-07-10 01:47:23 -0700 Comment on attachment 129838 Tentative patch Rejecting attachment 129838 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 667519 10 cgarcia 2012-07-12 01:38:36 -0700 Committed r122428: <http://trac.webkit.org/changeset/122428> 129838 2012-03-02 00:27:48 -0800 2012-07-10 01:47:23 -0700 Tentative patch bug-78132-20120302092746.patch text/plain 6322 svillar U3VidmVyc2lvbiBSZXZpc2lvbjogMTA5MzM4CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0Mi9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViS2l0Mi9DaGFuZ2VMb2cKaW5kZXggNmIyYTg1OGM3NjI4N2I5 NDE1ZGZmNWE0NDExZDllYTY2YjM3NGZhMC4uM2RmMzg3ZjE5ZGI2ZWNhNGQwNWY0ZTJiYmE2YWI3 ZTQ0NjYwZDM2NiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYktpdDIvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJLaXQyL0NoYW5nZUxvZwpAQCAtMSwzICsxLDIzIEBACisyMDEyLTAzLTAyICBTZXJn aW8gVmlsbGFyIFNlbmluICA8c3ZpbGxhckBpZ2FsaWEuY29tPgorCisgICAgICAgIFtXSzJdIFBl cmZvcm1hbmNlIGlzc3VlIGluIEZpbmRDb250cm9sbGVyOjpmaW5kU3RyaW5nCisgICAgICAgIGh0 dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD03ODEzMgorCisgICAgICAgIFJl dmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIEZpbmRDb250cm9sbGVyIHNob3Vs ZCBub3QgdW5tYXJrIGFsbCB0ZXh0IG1hdGNoZXMgYnkgZGVmYXVsdC4gSXQKKyAgICAgICAgd2ls bCBiZSBkb25lIG9ubHkgaWYgdGhlIHN0cmluZyBpcyBub3QgZm91bmQgb3IgaWYKKyAgICAgICAg bWFya0FsbFRleHRNYXRjaGVzKCkgaXMgY2FsbGVkLiBUaGlzIHdpbGwgYWxsb3cgY2xpZW50cyB0 byBsb29rCisgICAgICAgIGZvciB0aGUgbmV4dC9wcmV2aW91cyB3aXRob3V0IGhhdmluZyB0byB1 bm1hcmsoKSArIG1hcmsoKSBhbGwgdGhlCisgICAgICAgIHRleHQgbWF0Y2hlcyBmb3IgZXZlcnkg c2luZ2xlIHNlYXJjaCBvcGVyYXRpb24uCisKKyAgICAgICAgKiBVSVByb2Nlc3MvQVBJL2d0ay9X ZWJLaXRGaW5kQ29udHJvbGxlci5jcHA6CisgICAgICAgICh3ZWJLaXRGaW5kQ29udHJvbGxlclBl cmZvcm0pOgorICAgICAgICAod2Via2l0X2ZpbmRfY29udHJvbGxlcl9zZWFyY2hfbmV4dCk6Cisg ICAgICAgICh3ZWJraXRfZmluZF9jb250cm9sbGVyX3NlYXJjaF9wcmV2aW91cyk6CisgICAgICAg ICogV2ViUHJvY2Vzcy9XZWJQYWdlL0ZpbmRDb250cm9sbGVyLmNwcDoKKyAgICAgICAgKFdlYktp dDo6RmluZENvbnRyb2xsZXI6OmZpbmRTdHJpbmcpOgorCiAyMDEyLTAzLTAxICBTZXJnaW8gVmls bGFyIFNlbmluICA8c3ZpbGxhckBpZ2FsaWEuY29tPgogCiAgICAgICAgIFtXSzJdIFtHVEtdIFts aWJzb3VwXSBTb3VwU2Vzc2lvbiBzaG91bGQgdXNlIHN5c3RlbSBDQQpkaWZmIC0tZ2l0IGEvU291 cmNlL1dlYktpdDIvVUlQcm9jZXNzL0FQSS9ndGsvV2ViS2l0RmluZENvbnRyb2xsZXIuY3BwIGIv U291cmNlL1dlYktpdDIvVUlQcm9jZXNzL0FQSS9ndGsvV2ViS2l0RmluZENvbnRyb2xsZXIuY3Bw CmluZGV4IDM3ZjViMWYyMWZiYTJmYmRmYjQ5NjM4NjAyZTA0OTg4Mjc2OTRkMzQuLmYyMGRhYzU2 ZWI5Mzk5ZmQxN2ViNmE0MDljNDFlYzc0YmU4ZjcxZWQgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJL aXQyL1VJUHJvY2Vzcy9BUEkvZ3RrL1dlYktpdEZpbmRDb250cm9sbGVyLmNwcAorKysgYi9Tb3Vy Y2UvV2ViS2l0Mi9VSVByb2Nlc3MvQVBJL2d0ay9XZWJLaXRGaW5kQ29udHJvbGxlci5jcHAKQEAg LTQ4LDYgKzQ4LDcgQEAgZW51bSB7CiAKIHR5cGVkZWYgZW51bSB7CiAgICAgRmluZE9wZXJhdGlv biwKKyAgICBGaW5kTmV4dFByZXZPcGVyYXRpb24sCiAgICAgQ291bnRPcGVyYXRpb24KIH0gV2Vi S2l0RmluZENvbnRyb2xsZXJPcGVyYXRpb247CiAKQEAgLTM0NSwxNyArMzQ2LDIyIEBAIHN0YXRp YyB2b2lkIHdlYktpdEZpbmRDb250cm9sbGVyUGVyZm9ybShXZWJLaXRGaW5kQ29udHJvbGxlciog ZmluZENvbnRyb2xsZXIsIFdlCiAgICAgV0tSZXRhaW5QdHI8V0tTdHJpbmdSZWY+IHdrU2VhcmNo VGV4dChBZG9wdFdLLCBXS1N0cmluZ0NyZWF0ZVdpdGhVVEY4Q1N0cmluZyhmaW5kQ29udHJvbGxl ci0+cHJpdi0+c2VhcmNoVGV4dC5kYXRhKCkpKTsKICAgICBXS1BhZ2VSZWYgd2tQYWdlID0gZ2V0 V0tQYWdlRnJvbVdlYktpdFdlYlZpZXcoZmluZENvbnRyb2xsZXItPnByaXYtPndlYlZpZXcpOwog Ci0gICAgaWYgKG9wZXJhdGlvbiA9PSBGaW5kT3BlcmF0aW9uKSB7Ci0gICAgICAgIC8vIFVuY29u ZGl0aW9uYWxseSBoaWdobGlnaHQgdGV4dCBtYXRjaGVzLiBXSzEgQVBJIHdhcyBmb3JjaW5nCi0g ICAgICAgIC8vIGNsaWVudHMgdG8gZW5hYmxlL2Rpc2FibGUgaGlnaGxpZ2h0aW5nLiBTaW5jZSBt b3N0IG9mIHRoZW0KLSAgICAgICAgLy8gKGFsbD8pIHdoZXJlIHVzaW5nIGhpZ2hsaWdodGluZyB3 ZSBkZWNpZGVkIHRvIHNpbXBsaWZ5IHRoZQotICAgICAgICAvLyBXSzIgQVBJIGFuZCB1bmNvbmRp dGlvbmFsbHkgc2hvdyBoaWdobGlnaHRzLgotICAgICAgICB3a0ZpbmRPcHRpb25zID0gc3RhdGlj X2Nhc3Q8V0tGaW5kT3B0aW9ucz4oZmluZENvbnRyb2xsZXItPnByaXYtPmZpbmRPcHRpb25zIHwg a1dLRmluZE9wdGlvbnNTaG93SGlnaGxpZ2h0KTsKLSAgICAgICAgV0tQYWdlRmluZFN0cmluZyh3 a1BhZ2UsIHdrU2VhcmNoVGV4dC5nZXQoKSwgd2tGaW5kT3B0aW9ucywgZmluZENvbnRyb2xsZXIt PnByaXYtPm1heE1hdGNoQ291bnQpOworICAgIGlmIChvcGVyYXRpb24gPT0gQ291bnRPcGVyYXRp b24pIHsKKyAgICAgICAgV0tQYWdlQ291bnRTdHJpbmdNYXRjaGVzKHdrUGFnZSwgd2tTZWFyY2hU ZXh0LmdldCgpLCB3a0ZpbmRPcHRpb25zLCBmaW5kQ29udHJvbGxlci0+cHJpdi0+bWF4TWF0Y2hD b3VudCk7CiAgICAgICAgIHJldHVybjsKICAgICB9CiAKLSAgICBXS1BhZ2VDb3VudFN0cmluZ01h dGNoZXMod2tQYWdlLCB3a1NlYXJjaFRleHQuZ2V0KCksIHdrRmluZE9wdGlvbnMsIGZpbmRDb250 cm9sbGVyLT5wcml2LT5tYXhNYXRjaENvdW50KTsKKyAgICBpZiAob3BlcmF0aW9uID09IEZpbmRP cGVyYXRpb24pCisgICAgICAgIC8vIFVuY29uZGl0aW9uYWxseSBoaWdobGlnaHQgdGV4dCBtYXRj aGVzIHdoZW4gdGhlIHNlYXJjaAorICAgICAgICAvLyBzdGFydHMuIFdLMSBBUEkgd2FzIGZvcmNp bmcgY2xpZW50cyB0byBlbmFibGUvZGlzYWJsZQorICAgICAgICAvLyBoaWdobGlnaHRpbmcuIFNp bmNlIG1vc3Qgb2YgdGhlbSAoYWxsPykgd2hlcmUgdXNpbmcgdGhhdAorICAgICAgICAvLyBmZWF0 dXJlIHdlIGRlY2lkZWQgdG8gc2ltcGxpZnkgdGhlIFdLMiBBUEkgYW5kCisgICAgICAgIC8vIHVu Y29uZGl0aW9uYWxseSBzaG93IGhpZ2hsaWdodHMuIEJvdGggc2VhcmNoX25leHQoKSBhbmQKKyAg ICAgICAgLy8gc2VhcmNoX3ByZXYoKSBzaG91bGQgbm90IGVuYWJsZSBoaWdobGlnaHRpbmcgdG8g YXZvaWQgYW4KKyAgICAgICAgLy8gZXh0cmEgdW5tYXJrQWxsVGV4dE1hdGNoZXMoKSArIG1hcmtB bGxUZXh0TWF0Y2hlcygpCisgICAgICAgIHdrRmluZE9wdGlvbnMgPSBzdGF0aWNfY2FzdDxXS0Zp bmRPcHRpb25zPihmaW5kQ29udHJvbGxlci0+cHJpdi0+ZmluZE9wdGlvbnMgfCBrV0tGaW5kT3B0 aW9uc1Nob3dIaWdobGlnaHQpOworCisgICAgV0tQYWdlRmluZFN0cmluZyh3a1BhZ2UsIHdrU2Vh cmNoVGV4dC5nZXQoKSwgd2tGaW5kT3B0aW9ucywgZmluZENvbnRyb2xsZXItPnByaXYtPm1heE1h dGNoQ291bnQpOwogfQogCiBzdGF0aWMgaW5saW5lIHZvaWQgd2ViS2l0RmluZENvbnRyb2xsZXJT ZXRTZWFyY2hEYXRhKFdlYktpdEZpbmRDb250cm9sbGVyKiBmaW5kQ29udHJvbGxlciwgY29uc3Qg Z2NoYXIqIHNlYXJjaFRleHQsIGd1aW50MzIgZmluZE9wdGlvbnMsIGd1aW50IG1heE1hdGNoQ291 bnQpCkBAIC00MTUsNyArNDIxLDggQEAgdm9pZCB3ZWJraXRfZmluZF9jb250cm9sbGVyX3NlYXJj aF9uZXh0KFdlYktpdEZpbmRDb250cm9sbGVyKiBmaW5kQ29udHJvbGxlcikKICAgICBnX3JldHVy bl9pZl9mYWlsKFdFQktJVF9JU19GSU5EX0NPTlRST0xMRVIoZmluZENvbnRyb2xsZXIpKTsKIAog ICAgIGZpbmRDb250cm9sbGVyLT5wcml2LT5maW5kT3B0aW9ucyA9IGZpbmRDb250cm9sbGVyLT5w cml2LT5maW5kT3B0aW9ucyAmIH5XRUJLSVRfRklORF9PUFRJT05TX0JBQ0tXQVJEUzsKLSAgICB3 ZWJLaXRGaW5kQ29udHJvbGxlclBlcmZvcm0oZmluZENvbnRyb2xsZXIsIEZpbmRPcGVyYXRpb24p OworICAgIGZpbmRDb250cm9sbGVyLT5wcml2LT5maW5kT3B0aW9ucyA9IGZpbmRDb250cm9sbGVy LT5wcml2LT5maW5kT3B0aW9ucyAmIH5rV0tGaW5kT3B0aW9uc1Nob3dIaWdobGlnaHQ7CisgICAg d2ViS2l0RmluZENvbnRyb2xsZXJQZXJmb3JtKGZpbmRDb250cm9sbGVyLCBGaW5kTmV4dFByZXZP cGVyYXRpb24pOwogfQogCiAvKioKQEAgLTQzMiw3ICs0MzksOCBAQCB2b2lkIHdlYmtpdF9maW5k X2NvbnRyb2xsZXJfc2VhcmNoX3ByZXZpb3VzKFdlYktpdEZpbmRDb250cm9sbGVyKiBmaW5kQ29u dHJvbGxlcgogICAgIGdfcmV0dXJuX2lmX2ZhaWwoV0VCS0lUX0lTX0ZJTkRfQ09OVFJPTExFUihm aW5kQ29udHJvbGxlcikpOwogCiAgICAgZmluZENvbnRyb2xsZXItPnByaXYtPmZpbmRPcHRpb25z ID0gZmluZENvbnRyb2xsZXItPnByaXYtPmZpbmRPcHRpb25zIHwgV0VCS0lUX0ZJTkRfT1BUSU9O U19CQUNLV0FSRFM7Ci0gICAgd2ViS2l0RmluZENvbnRyb2xsZXJQZXJmb3JtKGZpbmRDb250cm9s bGVyLCBGaW5kT3BlcmF0aW9uKTsKKyAgICBmaW5kQ29udHJvbGxlci0+cHJpdi0+ZmluZE9wdGlv bnMgPSBmaW5kQ29udHJvbGxlci0+cHJpdi0+ZmluZE9wdGlvbnMgJiB+a1dLRmluZE9wdGlvbnNT aG93SGlnaGxpZ2h0OworICAgIHdlYktpdEZpbmRDb250cm9sbGVyUGVyZm9ybShmaW5kQ29udHJv bGxlciwgRmluZE5leHRQcmV2T3BlcmF0aW9uKTsKIH0KIAogLyoqCmRpZmYgLS1naXQgYS9Tb3Vy Y2UvV2ViS2l0Mi9XZWJQcm9jZXNzL1dlYlBhZ2UvRmluZENvbnRyb2xsZXIuY3BwIGIvU291cmNl L1dlYktpdDIvV2ViUHJvY2Vzcy9XZWJQYWdlL0ZpbmRDb250cm9sbGVyLmNwcAppbmRleCA2YjYx NTY5MzRjY2IzZjFiMWNjNjFjYzlkOGNlYzY5NWMwNmQ5MDBlLi4xZjc0YWE2ZTIwZDM4NWM1NDU1 ZGRiOWEzMmI2Mjg1Y2ExMTdmODg4IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViS2l0Mi9XZWJQcm9j ZXNzL1dlYlBhZ2UvRmluZENvbnRyb2xsZXIuY3BwCisrKyBiL1NvdXJjZS9XZWJLaXQyL1dlYlBy b2Nlc3MvV2ViUGFnZS9GaW5kQ29udHJvbGxlci5jcHAKQEAgLTkxLDggKzkxLDYgQEAgc3RhdGlj IEZyYW1lKiBmcmFtZVdpdGhTZWxlY3Rpb24oUGFnZSogcGFnZSkKIAogdm9pZCBGaW5kQ29udHJv bGxlcjo6ZmluZFN0cmluZyhjb25zdCBTdHJpbmcmIHN0cmluZywgRmluZE9wdGlvbnMgb3B0aW9u cywgdW5zaWduZWQgbWF4TWF0Y2hDb3VudCkKIHsKLSAgICBtX3dlYlBhZ2UtPmNvcmVQYWdlKCkt PnVubWFya0FsbFRleHRNYXRjaGVzKCk7Ci0KICAgICBib29sIGZvdW5kID0gbV93ZWJQYWdlLT5j b3JlUGFnZSgpLT5maW5kU3RyaW5nKHN0cmluZywgY29yZShvcHRpb25zKSk7CiAKICAgICBGcmFt ZSogc2VsZWN0ZWRGcmFtZSA9IGZyYW1lV2l0aFNlbGVjdGlvbihtX3dlYlBhZ2UtPmNvcmVQYWdl KCkpOwpAQCAtMTAwLDYgKzk4LDggQEAgdm9pZCBGaW5kQ29udHJvbGxlcjo6ZmluZFN0cmluZyhj b25zdCBTdHJpbmcmIHN0cmluZywgRmluZE9wdGlvbnMgb3B0aW9ucywgdW5zaWcKICAgICBib29s IHNob3VsZFNob3dPdmVybGF5ID0gZmFsc2U7CiAKICAgICBpZiAoIWZvdW5kKSB7CisgICAgICAg IG1fd2ViUGFnZS0+Y29yZVBhZ2UoKS0+dW5tYXJrQWxsVGV4dE1hdGNoZXMoKTsKKwogICAgICAg ICAvLyBDbGVhciB0aGUgc2VsZWN0aW9uLgogICAgICAgICBpZiAoc2VsZWN0ZWRGcmFtZSkKICAg ICAgICAgICAgIHNlbGVjdGVkRnJhbWUtPnNlbGVjdGlvbigpLT5jbGVhcigpOwpAQCAtMTE3LDYg KzExNyw3IEBAIHZvaWQgRmluZENvbnRyb2xsZXI6OmZpbmRTdHJpbmcoY29uc3QgU3RyaW5nJiBz dHJpbmcsIEZpbmRPcHRpb25zIG9wdGlvbnMsIHVuc2lnCiAgICAgICAgICAgICBpZiAobWF4TWF0 Y2hDb3VudCA9PSBudW1lcmljX2xpbWl0czx1bnNpZ25lZD46Om1heCgpKQogICAgICAgICAgICAg ICAgIC0tbWF4TWF0Y2hDb3VudDsKIAorICAgICAgICAgICAgbV93ZWJQYWdlLT5jb3JlUGFnZSgp LT51bm1hcmtBbGxUZXh0TWF0Y2hlcygpOwogICAgICAgICAgICAgbWF0Y2hDb3VudCA9IG1fd2Vi UGFnZS0+Y29yZVBhZ2UoKS0+bWFya0FsbE1hdGNoZXNGb3JUZXh0KHN0cmluZywgY29yZShvcHRp b25zKSwgc2hvdWxkU2hvd0hpZ2hsaWdodCwgbWF4TWF0Y2hDb3VudCArIDEpOwogCiAgICAgICAg ICAgICAvLyBDaGVjayBpZiB3ZSBoYXZlIG1vcmUgbWF0Y2hlcyB0aGFuIGFsbG93ZWQuCg==