Closed Bug 486072 Opened 12 years ago Closed 5 years ago

Selection can be slow in large file with lots of elements

Categories

(Core :: DOM: Selection, defect)

defect
Not set
normal

Tracking

()

RESOLVED WORKSFORME

People

(Reporter: smontagu, Unassigned)

References

(Depends on 2 open bugs)

Details

(Keywords: perf, regression)

Attachments

(3 files)

Attached file testcase
Steps to reproduce:

1) Click in the middle of the first line of the attached testcase
2) Scroll down and shift-click in the middle of the last line
3) Click somewhere else

After 2 and 3 the browser becomes unresponsive with high CPU usage for several seconds.

Regression range: http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2006-07-27+10%3A00&maxdate=2006-07-28+04%3A00&cvsroot=%2Fcvsroot

Bug 345099 looks like the most likely suspect in that range.
This is what I used to generate the testcase, in case someone wants to create a larger or smaller version.
I profiled the steps to reproduce, and we spend a bunch of time painting (on Mac).  In particular, at least 30% of the time is CoreGraphics stuff.  Also a bunch of time spent adding rects to native regions, etc.

That said, we do spend some time in IndexOf and whatnot; I suspect this comment in the patch for bug 345099 is relevant:

+//    PERFORMANCE: a common case is that we are doing a fast check with exactly
+//    one range in the selection. In this case, this function is slower than
+//    brute force because of the overhead of checking the tree. We can optimize
+//    this case to make it faster by doing the same thing the previous version
+//    of this function did in the case of 1 range. This would also mean that
+//    the aSlowCheck flag would have meaning again.
Isn't this test case (at least step 2 of the STR) the opposite extreme from that? Each character is in its own inline, so with 8K inlines each containing one text frame, aren't there 16K ranges?
Whoops, forgot to cc myself.

No, there is only one DOM range for the selection, since it's contiguous.  The only time we get more than one range around is if the selection is discontiguous in the DOM or if there are multiple selections.

The each character in its own inline certainly explains why there are so many textframes, though!  I'd been wondering.

So we end up doing thousands of calls into LookupSelection.  Each call does a bunch of comparing, etc...  Still, the main cost I see here is painting....
This isn't correct, but I was wondering whether it even helps...
With that, step 2 is visibly faster, but still a lot slower than in builds before bug 345099.

In step 3 there is hardly any hang, but on the down side the selection doesn't collapse as it should ;-)
Sure; like I said it's not correct.  I don't see why it would be slower than in builds before bug 345099...  I do you think you might be able to profile the hang, by any chance?
For what it's worth, I tried doing just that right now, and all the time is spent painting (borders, text, the selection itself).  Since you're comparing cairo to pre-cairo builds at this point, is this just a matter of cairo slowness?
OK.  The reason that patch doesn't work as it should is that the new-textframe landing made the GetSelectionDetails() call in the !aSelected case in SetSelected not pass true for aSlowCheck.  So we never detect that we're no longer in the selection with this patch.

So it seems like readding the fast path here depends on the general "improve selection setup in textframe" bug.  roc, do we have a bug for that yet?

I filed bug 486547 on improving the slow path performance here.
I've lost track of the selection bugs. May as well file a new one I guess.
I found bug 485446 and took the liberty of adding it to the layout spreadsheet.
Depends on: 485446
(In reply to comment #7)
> do you think you might be able to profile the
> hang, by any chance?

I tried to, but can no longer get jprof to work on my (Ubuntu 8.10) system.
gdb and ctrl-c might give you some ideas
I managed to get jprof to work with JP_RTC_HZ=64. Like that, in step 2 of the STR we spend 57.6% of the time in nsLineBox::IndexOf and 35.3% in calloc.

In step 3 we spend 86.5% in nsLineBox::IndexOf and 10% in nsAttrAndChildArray::IndexOfChild(nsINode*)
I saw the IndexOfChild stuff here, but what do the stacks to those others look like?
Depends on: 519657
Keywords: regression
Mozilla/5.0 (Windows NT 10.0; WOW64; rv:49.0) Gecko/20100101 Firefox/49.0

I have tested on the latest Firefox release (46.0.1, Build ID: 20160502172042) and on the latest Nightly(49.0a1, Build ID: 20160522030240) and I haven't managed to reproduce it. I've opened attachment 370152 [details], clicked in the middle of the first line and scrolled down and shift-clicked in the middle of the last line, after a few seconds it marked all the characters in the range as selected.
I have also tested this on Mac OS (10.11, 10.8), Ubuntu (12.04, 14.04)
and Windows (10, XP) with older versions of Firefox and It behaved the same, no crashes, hangs or high CPU usage.

Is this still reproducible on your end?
Flags: needinfo?(smontagu)
I can't reproduce this any more on either OSX or Linux
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: needinfo?(smontagu)
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.