Closed Bug 1324143 Opened 8 years ago Closed 8 years ago

Find toolbar is very slow and uses 100% CPU on large page

Categories

(Toolkit :: Find Toolbar, defect)

Product:
Toolkit
Component:
Find Toolbar
Platform:
x86_64
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: botond, Assigned: mikedeboer)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

Bug 1324143 - part 1 - remove the deprecated '_getRangeContentArray' utility method and its uses, the unused '_maybeHighlightAll' findbar method and cleanup range outline highlighting code.
58 bytes, text/x-review-board-request
jaws
: review+
Details
Bug 1324143 - part 2 - stop doing expensive operations when we don't have to.
58 bytes, text/x-review-board-request
jaws
: review+
Details
Bug 1324143 - part 3 - disable CPU intensive tasks when the page is too big to bother.
58 bytes, text/x-review-board-request
jaws
: review+
Details
STR: 1. Load https://public-artifacts.taskcluster.net/B597Ys5nTu6qTYNIm2jgbg/0/public/logs/live_backing.log 2. Activate the find toolbar 3. Type "clang" Actual results: It takes 10+ seconds for the gray find highlight to appear. Then several more seconds for the first occurrence of "clang" to be highlighted in yellow. It you then click the "Find Next" button, it takes several more seconds for the highlight to be updated to the next match. All the while, the content process is using 100% CPU, even after the correct match has been highlighted and you haven't pressed Find Next again. Expected results: Response time is well under a second, and CPU utilizations becomes low after the match has been highlighted.
Yeah, this is a horrible regression. I've been working on this yesterday - this would be a good place to stick patches in.
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
The last patch in the series here disables dynamic updates of the find matches if the page is over 500,000px in height or width. I think we are at the point where we can't make this implementation any faster and we should approach the layout team to see if they can implement some extra CSS properties on ::-moz-selection so we don't need to reimplement layout updating/invalidation etc in the frontend.
Comment on attachment 8820269 [details] Bug 1324143 - part 1 - remove the deprecated '_getRangeContentArray' utility method and its uses, the unused '_maybeHighlightAll' findbar method and cleanup range outline highlighting code. https://reviewboard.mozilla.org/r/99804/#review102580 I don't want to hold this up any longer but I do believe that we should begin conversations about not shipping the "modal UI" and going back to the ::-moz-selection implementation, with improved styling capabilities for ::-moz-selection and no dimming of the page. ::: toolkit/modules/FinderHighlighter.jsm:810 (Diff revision 1) > + // When the range is invisible, we can return early. > + // if (!this.finder._fastFind.isRangeVisible(range, false)) > + // return { rectList: [], textList: [] }; This commented out code should probably just get deleted.
Attachment #8820269 - Flags: review?(jaws) → review+
Comment on attachment 8820270 [details] Bug 1324143 - part 2 - stop doing expensive operations when we don't have to. https://reviewboard.mozilla.org/r/99806/#review102582
Attachment #8820270 - Flags: review?(jaws) → review+
Comment on attachment 8820271 [details] Bug 1324143 - part 3 - disable CPU intensive tasks when the page is too big to bother. https://reviewboard.mozilla.org/r/99808/#review102584 ::: toolkit/modules/FinderHighlighter.jsm:24 (Diff revision 1) > return Services.prefs.getPrefType(kDebugPref) && Services.prefs.getBoolPref(kDebugPref); > }); > > const kContentChangeThresholdPx = 5; > const kBrightTextSampleSize = 5; > +const kPageIsTooBigPx = 500000; 500,000 is arbitrary and doesn't scale for low-powered machines or high-powered machines. Netbooks will probably need a much lower limit, for example. Though getting something out there is better than nothing.
Attachment #8820271 - Flags: review?(jaws) → review+
Comment on attachment 8820271 [details] Bug 1324143 - part 3 - disable CPU intensive tasks when the page is too big to bother. https://reviewboard.mozilla.org/r/99808/#review102584 > 500,000 is arbitrary and doesn't scale for low-powered machines or high-powered machines. Netbooks will probably need a much lower limit, for example. Though getting something out there is better than nothing. I added this exact comment above the constant declaration. I fully agree.
Pushed by mdeboer@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/e5624c0290e4 part 1 - remove the deprecated '_getRangeContentArray' utility method and its uses, the unused '_maybeHighlightAll' findbar method and cleanup range outline highlighting code. r=jaws https://hg.mozilla.org/integration/autoland/rev/ff105eaa2e08 part 2 - stop doing expensive operations when we don't have to. r=jaws https://hg.mozilla.org/integration/autoland/rev/b757748690ad part 3 - disable CPU intensive tasks when the page is too big to bother. r=jaws
Blocks: 1291278
https://hg.mozilla.org/mozilla-central/rev/e5624c0290e4 https://hg.mozilla.org/mozilla-central/rev/ff105eaa2e08 https://hg.mozilla.org/mozilla-central/rev/b757748690ad
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox53: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Still unusable on that page for me even after the patches.
(In reply to 6lobe from comment #19) > Still unusable on that page for me even after the patches. That's bug 1280978.
See Also: → 1280978
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: