1379406 - Store DisplayItemClipChains in AutoTArray with a larger initial size

Status: RESOLVED FIXED

(Core :: Web Painting, enhancement)

Description

[Markus Stange [:mstange]]

We currently add all DisplayItemClipChain objects to mClipChainsToDestroy so that we can call their destructors when the display list builder is destroyed. But the only thing that really needs destroying is the array of rounded rects in the clip chain's mClip field. So we could skip adding non-rounded clip chains to this array.

This is what we were doing before bug 1298218 - we only put DisplayItemClips with rounded corners into mDisplayItemClipsToDestroy:
https://hg.mozilla.org/mozilla-central/file/8eeb5e0b825b/layout/painting/nsDisplayList.cpp#l1235

Adding elements to mClipChainsToDestroy shows up as cache misses on https://people-mozilla.org/~jmuizelaar/implementation-tests/dl-test.html when the array's internal buffer is enlarged and its contents are moved.

Comment 1

[Jet Villegas (inactive)]

Hi Miko: Can you pick this one up? Thx!

Comment 2

[Miko Mynttinen]

Attachment: Bug 1379406 - Store DisplayItemClipChains in AutoTArray with a larger initial size

Review commit: https://reviewboard.mozilla.org/r/163386/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/163386/

Comment 3

[Miko Mynttinen]

Because we modify DisplayItemClips after allocation (for example with DisplayItemClip::SetTo()), there is no easy way to tell if they are going to have rounded corners. Furthermore, I think that nsRect in DisplayItemClip requires a destructor call for leak checking (http://searchfox.org/mozilla-central/source/gfx/src/nsRect.h#37). I do not know why the previous version worked.

I have attached a patch that hopefully fixes the underlying cache miss problem by pre-allocating a larger array. The size 128 was chosen based on Google image search (60-80 clips) and Facebook (80-100 clips).

Comment 4

[Miko Mynttinen]

Markus, could you please see if this helps with the cache misses?

Comment 5

[Markus Stange [:mstange]]

Comment on attachment 8892424 [details]
Bug 1379406 - Store DisplayItemClipChains in AutoTArray with a larger initial size

https://reviewboard.mozilla.org/r/163386/#review186652

I haven't profiled this patch yet but it seems reasonable enough.

Comment 6

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 1bf85bf00940 -d 6918dea06b7b: rebasing 421513:1bf85bf00940 "Bug 1379406 - Store DisplayItemClipChains in AutoTArray with a larger initial size r=mstange" (tip)
merging layout/painting/nsDisplayList.h
warning: conflicts while merging layout/painting/nsDisplayList.h! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 7

[Miko Mynttinen]

Comment on attachment 8892424 [details]
Bug 1379406 - Store DisplayItemClipChains in AutoTArray with a larger initial size

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/163386/diff/1-2/

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d817c91e1e56
Store DisplayItemClipChains in AutoTArray with a larger initial size r=mstange

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d817c91e1e56