407889 - Reduce allocations for nsDisplayListBuilder::mCaretStates and mFramesMarkedForDisplay

Status: RESOLVED FIXED

(Core :: Layout, defect, P1)

Description

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

We can use nsAutoTArray here to reduce heap allocations.

We should also change mFramesMarkedForDisplay so that instead of accumulating frames and only unmarking them at the end of the lifetime of the nsDisplayListBuilder, we unmark frames when we leave their presshell. This will reduce the max number of elements needed in mFramesMarkedForDisplay.

Comment 1

[Mike Schroepfer]

+ing to continue our assault on memory usage and allocations..

Comment 2

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

I'm also going to fix HitTest so that instead of allocating an array at each recursion level, we share single nsAutoTArray for the whole stack.

Comment 3

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

Attachment: fix mCaretStates/mFramesMarkedForDisplay

This patch reorganizes the per-presshell state in nsDisplayListBuilder. The main goal is to remove frames from mFramesMarkedForDisplay when we leave the presshell for those frames, which means mFramesMarkedForDisplay won't need to get so large. Then we make the relevant nsTArrays nsAutoTArray instead.

Comment 4

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

Attachment: fix HitTest

Using a single buffer for all nested nsDisplayList::HitTest calls, and make that buffer an nsAutoTArray. This should basically eliminate all allocations in this code.

Comment 5

[Mats Palmgren (inactive)]

Comment on attachment 293047 [details] [diff] [review]
fix HitTest

This patch seems to have a bug somewhere. Try drag-selecting text
on http://www.mozilla.org/projects/minefield/
I see a lot of flicker...  (Linux debug 64-bit)
I'll continue the review tomorrow but wanted to give you a heads up
in case you want to start investigate that problem...

Comment 6

[Mats Palmgren (inactive)]

Comment on attachment 292936 [details] [diff] [review]
fix mCaretStates/mFramesMarkedForDisplay

Looks good. r+sr=mats

The initial capacity numbers can perhaps be tuned?
I added a bit of tracing and looked at a few hundred pages from
Alexa Top 500 and a few other selected pages on Wikipedia etc.
I could not find any page that used more than 3 PresShellState, so
maybe we should start with 4 instead of 8?

Comment 7

[Mats Palmgren (inactive)]

Comment on attachment 293047 [details] [diff] [review]
fix HitTest

This patch doesn't work correctly for me. Drag-selecting text flickers,
nothing happens when clicking in the toolbar/urlbar etc.

Comment 8

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

These PresShellStates are small so I think leaving it at 8 is harmless.

I'll look into the problem in the other patch.

Comment 9

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

(In reply to comment #7)
> (From update of attachment 293047 [details] [diff] [review])
> This patch doesn't work correctly for me. Drag-selecting text flickers,
> nothing happens when clicking in the toolbar/urlbar etc.

I'm not seeing any of these problems :-(

Comment 10

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

Mats, do you have any other platforms available you could check on?

Comment 11

[Mats Palmgren (inactive)]

Comment on attachment 293047 [details] [diff] [review]
fix HitTest

My apologies, I must have missed that one of the hunks for
nsDisplayList.h was rejected by 'patch' -- with that fixed
it works fine.   r+sr=mats

Comment 12

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

Both patches checked in. Thanks!