227458 - Very slow rendering of thousands of overflow:hidden divs

Status: RESOLVED FIXED

(Core :: Web Painting, defect, P1)

Description

[Hish]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.5) Gecko/20031007
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.6b) Gecko/20031203

http://212.23.139.138/moz16bug/ is a big DIV list. 
IE6 and Moz 1.5 render this page in less than a second
Moz 1.6 needs almost 60 secs (Sony Vaio 1GHz, 512MB)
When the page is renderen and you open a menu item, than it takes again 60 secs
to show the menu.

Reproducible: Always

Steps to Reproduce:
open th link
Actual Results:  
moz 16b freezes

Expected Results:  
less than 1 second rendering time

It's a blocker because we deal with big address lists in the browser. So we
can't work with 16b anymore.

Comment 1

[Jon Henry]

Hish, only Mozilla.org drivers should set the blocking flags to +. You can use ?
to request they review the bug, but only they should be actually setting the
blocking status.

Did this work for you in previous versions? If so, could you narrow down at all
when the regression happened?

Comment 2

[Isaac Hwak Han]

Removing all 'overflow: hidden' occurrences cures the problem. Actually there is
no point in using them in the testcase page. (MSHTML-generated) I tried
different overflow: property values.

overflow: visible (default, OK)
overflow: hidden/scroll/auto (extremely slow)

It may be a dup of many overflow:-related bugs (possibly bug 221614 and the
16-bit coordinate bug, bug 126592).

Comment 3

[Jon Henry]

I'm not sure it's the 16-bit issue since that supposedly does not affect WinXP
(and I can't reproduce any of the testcases on it). I've been searching for a
dupe for a while now, and haven't come up with anything. This might be new.

Comment 4

[Mats Palmgren (inactive)]

Confirming bug, regression occured somwhere between 1.5 and 1.6a.
Changing 'overflow:hidden' to 'overflow:-moz-hidden-unscrollable' cures the
problem so I suspect bug 69355.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Well, -moz-hidden-unscrollable is much more lightweight than hidden -- it
doesn't need a scrollframe and all that jazz.  Here are the fun profile results
(non-realtime profile):

Total hits:  2588
Hits under PresShell::ProcessReflowCommands:  169
Hits under nsCSSFrameConstructor::ConstructFrame: 806 (a lot of this under
   nsScrollPortView::CreateScrollControls)
Hits under nsWindow::Update: 1510 (1501 of these under nsWindow::DoPaint)

Tracking that DoPaint path down some more, I see 1439 hits under
nsViewManager::RenderViews -- of these 1122 under BuildDisplayList, 236 under
AddCoveringWidgetsToOpaqueRegion and 61 under nsViewManager::OptimizeDisplayList).

The flat profile looks like:

Total hit count: 2588
Count %Total  Function Name
1081   41.8     ComputePlaceholderContainment(nsView*)
223   8.6     gtk_signal_handler_insert
 93   3.6     nsSupportsArray::QueryElementAt(unsigned, nsID const&, void**)
 84   3.2     nsBaseWidget::GetZIndex(int*)
 79   3.1     nsView::IsZPlaceholderView()
 63   2.4     nsRegion::SubRect(nsRectFast const&, nsRegion&, nsRegion&) const
 52   2.0     nsRegion::SetToElements(unsigned)
 44   1.7     nsRegion::Copy(nsRegion const&)
 42   1.6     RgnRectMemoryAllocator::Alloc()
 35   1.4     nsBaseWidget::SetZIndex(int)

So it looks like the basic problem is that now every single overflow:hidden
thingie has a widget.  Which means that painting has to z-order every single 
overflow:hidden element on that page a whole bunch of times...

I've filed bug 227491 on the nsSupportsArray thing.

All the ComputePlaceholderContainment are under BuildDisplayList, of course.

I did a little test.  If I use "overflow: -moz-hidden-unscrollable", I get 25
calls to BuildDisplayList and 24854 calls to ComputePlaceholderContainment. If I
use "overflow: hidden", I run out of disk space for the log file before the load
completes, but for the partial load I got data for I get 1565 calls to
BuildDisplayList and 24962342 calls to ComputePlaceholderContainment.

So two obvious questions:

1) Why are we calling BuildDisplayList so much more?  Is it because we do it on
   every widget insertion?

2) Why is ComputePlaceholderContainment called so much more per BuildDisplayList
   in the "hidden" case?  There should only be about 3 times as many views, not
   15 times more... (the ratios are about 1000 for the -moz-hidden-unscrollable
   case and 15000 for the hidden case).

Comment 6

[Boris Zbarsky [:bzbarsky]]

More data.  In the fast case, when nsWindow::UpdateIdle is called it has 7 or 8
windows in update_queue.  In the slow (overflow:hidden) case, it's called at
some point when it has 5000 or so windows in update_queue.  Those are the
windows corresponding to the widgets in the scrolling stuff.

Now for each of those 5000 windows, a paint event is synchronously dispatched to
that window's event callback, which is the HandleEvent callback in nsView.  That
passes along the event to the viewmanager.

The viewmanager starts building the display list with displayRoot pointing to
the widget's view (5000 or so distinct values there).  But then, "aCaptured" is
false, so it walks up the view tree to the root view and renders from there
down.  So we paint the whole page 5000 times or so.  Oops.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Oh, and the reason we have all those widgets is that they're what we use for
clipping of the overflowing content...

Comment 8

[Boris Zbarsky [:bzbarsky]]

Reporter, the page has gone all unavailable... ("Connection Refused").  Would
you mind attaching it to this bug using
http://bugzilla.mozilla.org/attachment.cgi?bugid=227458&action=enter so that it
can be used to test patches?

Comment 9

[Mats Palmgren (inactive)]

Attachment: Testcase

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

> So we paint the whole page 5000 times or so.

Well, hopefully the display list builder only actually generates a display list
containing the elements that intersect the widget in question.

> Oh, and the reason we have all those widgets is that they're what we use for
> clipping of the overflowing content...

They're used to scroll the content, mostly. They're also used to clip plugins.
Normal frames would clip themselves whether there's a widget there or not.

It would certainly be nice to have at least some scrollframes not need widgets.
Even without per-platform work to make nsIWidget::Scroll methods honour the rect
parameter, we could just fully repaint them when they scroll and this would not
be a perf problem if we stick to rarely scrolled frames. The main problem is
what happens when there are other native widgets, especially plugins, present as
children. Or we could say that we don't care if plugins are clipped in this
situation. Or we could actually go and fix plugins the way I want to, by adding
a per-platform way to specify a native clip region on each plugin and then
having layout set this region to the plugin's actual visible area taking into
account layout's clipping and drawing demands.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Yeah, ok.  I don't understand this code that well.  s/paint/build display list
from the top level/.

Did plugins in overflow:hidden divs that were overflowin use to just not get
clipped?  If so, it may make sense to go back to that for now (and investigate
the other way you're proposing for handling that clipping)...  Are there any
other drawbacks to not giving most scrollframes widgets?

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

> Did plugins in overflow:hidden divs that were overflowin use to just not get
> clipped?

Right

> Are there any other drawbacks to not giving most scrollframes widgets?

Slower scrolling, obviously. Other than that, I can't think of any, but I'd be a
little worried about the risk of regressions especially at this stage.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Right.  So I guess the two questions are:

1) What do we do for 1.6?
2) What do we do for later?

I guess overflow:hidden is used a lot more than scroll/auto, which is why we
never so all these issues before... :(

Comment 14

[Hish]

can somebody tell me in short why it worked with mozilla 1.5?

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I could probably live with making SCROLLBARS_NONE nsGfxScrollFrames not create a
widget for 1.6. I'd have to run some tests and get the fix in soon to make sure
there's adequate widespread testing for 1.6.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Hish, it worked in 1.5 because overflow:hidden elements did not create a
scrollframe.  This made them not be scrollable either programmatically or via
user action (eg by drag-selecting).  That behavior was changed in 1.6a -- now
they have no scrollbars but can be scrolled via JS, etc.  See bug 69355 as
pointed out in comment 4.

Your page would have shown the same performance problem in 1.5 if it used
overflow:auto....

Comment 17

[Hish]

I tested a bit with other browsers and 1.6b.
"overflow: hidden;  overflow:-moz-hidden-unscrollable;" cures the bug and lets
all other browsers behave as wanted.
It's important to have overflow:-moz-hidden-unscrollable behind overflow:hidden

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

OK, the good news is that (and I should have known this already) scrollports in
form elements already don't have a widget, so the no-widget scrolling code
paths are already heavily tested and my estimation of the risk of this patch
goes way down.

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: oops

The previous patch was incorrect. This is the correct patch. It seems to
work...

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 137121 [details] [diff] [review]
oops

>Index: layout/html/base/src/nsGfxScrollFrame.cpp
>+  nsIScrollableFrame* scFrame =
>+    (nsCOMPtr<nsIScrollableFrame>(do_QueryInterface(parent)));

That may look less weird as:

nsIScrollbaleFrame* scFrame;
CallQueryInterface(parent, &scFrame);

>+  return NS_STATIC_CAST(nsGfxScrollFrame*, scFrame);

Do we have a bug on eliminating nsIScrollableFrame, by any chance?  We only
have a single impl of it, and are not likely to have more....

>Index: layout/html/base/src/nsScrollPortFrame.cpp

>     if ((NS_SUCCEEDED(parentFrame->QueryInterface(NS_GET_IID(nsIFormControlFrame), (void**)&fcFrame)))) {

Not youre code, but 

  if (NS_SUCEEDED(CallQueryInterface(parentFrame, &fcFrame))) {

is nicer.  ;)

Apart from those two nits, looks good.	r+sr=bzbarsky

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 137121 [details] [diff] [review]
oops

plussing as per bz's comment

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

> That may look less weird as:

Yeah, I don't know what I was doing

> Do we have a bug on eliminating nsIScrollableFrame, by any chance?

Eliminating it in favour of nsGfxScrollFrame and deCOMtaminating+devirtualizing
its methods ... no, no Bugzilla bug, but it's in my head

Thanks! I'll check it in tonight, hopefully.

Comment 23

[Boris Zbarsky [:bzbarsky]]

> no, no Bugzilla bug, but it's in my head

File the bug.  Just in case.  ;)

And sorry for not setting the flags... that should teach me to do reviews before
breakfast.

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 137121 [details] [diff] [review]
oops

Requesting 1.6 approval for this 1.6 blocker bug

Comment 25

[Richard Lloyd]

Just wondering if this is another (good) example of the overflow: hidden problem:

http://download.fedora.redhat.com/pub/fedora/linux/core/development/i386/Fedora/RPMS/

Yep, this is where all the Fedora Core development is being done and it's
virtually unlistable in Mozilla (I tried Mozilla 1.5 in Linux and Mozilla 1.6b
in Windows - both struggle badly with it. Got MSIE 6.0 SP1 and Opera 7.23 to
render it in about 30 seconds, but gave up after minutes with Mozilla). I'm
posting here just to make sure it is indeed the same bug...

Comment 26

[Boris Zbarsky [:bzbarsky]]

If it's a problem in 1.5, it's not this bug (which is obvious from _reading_
this bug)... Could you file a separate bug on that site and CC me?

Comment 27

[chris hofmann]

let get in in quickly for 1.6

Comment 28

[eberry]

bz - At least one incarnation of the Fedora download page bug from comment 25 is
in bugzilla as #226938.  The page is a bit slow loading still in in 1.6b/XP, but
not terrible given the size of the page.  The more serious problem now might be
the the coordinate overflow you hinted at in the related bug.

Comment 29

[Boris Zbarsky [:bzbarsky]]

Oh, right.  That was the page we were having that perf issue on (bug 225848).  
Richard Lloyd, are you sure you're seeing a major performance problem there in a
1.6b build?  It should have been fixed by the checkin for bug 225848...  If you
are, please file a new bug and cc me.

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Fix checked in last night, by the way.

Comment 31

[Boris Zbarsky [:bzbarsky]]

This caused bug 232951