70211 - nsContainerFrame::PositionChildViews does a recursive walk of the frame tree when positioning views

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Kevin McCluskey (gone)]

The logic in nsContainerFrame::PositionChildViews does a recursive walk of the
frame tree to position the views of descendants. It should be much more efficent
to get the child views and their associated frames and determine whether the
frames are descendants of the container frame instead. There are typically just
a few views per page, while there can be a large number of frames.

I commented out the logic in nsContainerFrame::PositionChildViews and saw a 4%
page load improvement running jrgm's perf test. This is definitely worth
investigating.

Comment 1

[Kevin McCluskey (gone)]

Added topperf keyword and mozilla0.9 milestone

Comment 2

[Kevin McCluskey (gone)]

Attachment: Attempted fix. Doesn't position XUL menus correctly

Comment 3

[rbs]

the so-called 'secret' XBL frames somewhere in-between?

Comment 4

[Kevin McCluskey (gone)]

Updated status whiteboard

Comment 5

[Eugene Savitsky]

ETA (2/30) for this bug? So it will never happens?

PS We have max the 29 february in grigorian calender... :)

Comment 6

[Kevin McCluskey (gone)]

Darn!, almost got away with it :)

Comment 7

[Kevin McCluskey (gone)]

Setting milestone to mozilla0.8.1

Comment 8

[Kevin McCluskey (gone)]

Attachment: patch which eliminates the frame tree walk and positions XUL popup's correctly

Comment 9

[Kevin McCluskey (gone)]

Updated status.

Comment 10

[David Hyatt]

I would prefer it if you put the code that tells menu popups not to sync up in
the nsMenuPopupFrame constructor instead of in the CSS frame constructor.

Also, test the heck out of this before landing.

sr=hyatt

Comment 11

[Kevin McCluskey (gone)]

Ok. I'll move that code to the popup's constructor. (I had it there to begin
with, but it looked like CSSFrameConstructor was setting frame state for other
frames. I agree it's better to move as much as possible out of
CSSFrameConstructor, since it has too much in it to begin with ;-)).

I won't be landing this for several days. I'll test on WIN32, Linux, and Mac
before checking this in. I've been running for two days with it on WIN32 and so
far I haven't seen any problems.

Comment 12

[Marc Attinasi]

r=attinasi - I will be applying and testing this starting Thursday as well, but
the code looks fine to me.

Comment 13

[David Hyatt]

Test cases where menus need to be pulled back onscreen by moving your window to
the edge of your monitor.

Comment 14

[Kevin McCluskey (gone)]

Attachment: patch with change to set the frame's state in the ctor

Comment 15

[Tomi Leppikangas]

Just for info. When i did profile with eazel profiler in page
http://www.w3.org/TR/css3-roadmap/  (bug #68821) i see
nsContainerFrame::PositionChildViews called 1013136 times and taken about
4sec when just mousing over page once (enter/leave page).

Maybe that is good test page? Maybe add depency to that bug?

Comment 16

[Kevin McCluskey (gone)]

I tested the XUL Menus as Dave suggested. I have not seen any regressions as the
result of my patch, but it should be noted that there are two "pre existing"
problems with popup menus that cause the popup's submenus to be positioned
incorrectly
bug 71521 
bug 61662


I have tested WIN32, Linux, and Mac and I have not seen any regressions. I'm
planning on checking in the patch this weekend.

Comment 17

[Kevin McCluskey (gone)]

Patch checked in.
Marking fixed.
peterson: This can only be verified by looking at the code.

Comment 18

[Aidas Kasparas]

Not sure if this checkin caused that but on Linux-2001031408 Ctrl+/Crtl- started
to work incorectly - when I press those key combinations text is drawn in bigger
or smaller font but at the same place that it originally was. This causes text
drawn on another text or forming blank spaces.

Comment 19

[Kevin McCluskey (gone)]

I have a new patch, which should be provide better performance than the last
past that was checked in.

I am reopening this bug until we determine whether the new patch has any impact
on page load times. 

Comment 20

[Kevin McCluskey (gone)]

Attachment: new patch which sets and uses a frame bit to indicate whether any of the frame's descendants have views

Comment 21

[rbs]

Simpler and much better design in this patch...
Some minor typos:

+// If this bit is set the frame does not have a descendant with a view
+#define NS_FRAME_HAS_CHILD_WITH_VIEW                  0x00040000

It think you meant _does have_ ?

+  PRBool isSet= ((frameState & NS_FRAME_HAS_CHILD_WITH_VIEW) == 
NS_FRAME_HAS_CHILD_WITH_VIEW);
+  if (! isSet) {

Why bother with the temp. variable. Just testing the stuff is ok (and readable).

       // Remember our view
       aFrame->SetView(aPresContext, view);
 
+      if (view) {
+         // Let all of the containing frames know they have a child with a view

The test doesn't seem necessary here since earlier in the code 'view->' is used.

Searched and saw the above mentioned Ctrl+/- bug 72181 which seems to be a
separate issue.

Comment 22

[Kevin McCluskey (gone)]

Attachment: Cleaned up patch which sets the bit in nsFrame::SetView instead of nsHTMLContainer

Comment 23

[Kevin McCluskey (gone)]

jrgm ran his performance tests on all three platforms with the equivalent of the
new patch, but he did not see any performance improvement. 

This patch should improve performance when loading large documents however.
[e-mail] and I saw that 50% of the time when reflowing a large text
document is spent in PositionChildViews. The new patch should cut out 80% of
that time.

Comment 24

[Kevin McCluskey (gone)]

Updated status

Comment 25

[John Morrison]

(tiny note: I ran on win2k. I didn't run on mac or linux).

Comment 26

[karnaze (gone)]

r=karnaze

Comment 27

[Kevin McCluskey (gone)]

Fix checked in.

Comment 28

[Marc Attinasi]

I had approved this prior to the checkin - sr=attinasi

Comment 29

[Chris Petersen]

Marking verified per last comments.