729519 - Allocate heap nsFrameLists from the pres shell arena

Status: RESOLVED FIXED

(Core :: Layout: Block and Inline, enhancement, P5)

Description

[Mats Palmgren (inactive)]

Attachment: exploration

I'm not sure if this is worth doing for the relatively few heap
nsFrameLists that we have.

pros:
1. pres arena LRU cache (is it really better than jemalloc though?)
2. avoids freeing memory for each one separately when destroying the shell
   (many of these lists are transient and don't live that long though)
3. leak detection by default (when the pres shell is destroyed)

cons:
1. slightly more heavy handed to allocate/destroy since you have to
   provide the pres shell

PresArena also gives us poisoning but since the two frame members of
nsFrameList are already poisoned I don't think it adds much value.

Also, the FramePropertyTable changes in the patch to support a destructor
is probably overkill, we could just free these properties manually in the
respective frame that owns them.

What do you think, is it worth doing this?

Comment 1

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

I don't know.

Comment 3

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

OK.

Comment 4

[Mats Palmgren (inactive)]

Attachment: Use SafelyDestroyFrameListProp also for the OverflowList

Don't use DestroyOverflowList() to destroy the overflow list in
nsContainerFrame::DestroyFrom -- use SafelyDestroyFrameListProp instead,
because it's more robust (bug 606642) and it makes the next patch possible.

Comment 5

[Mats Palmgren (inactive)]

Attachment: Simplify DestroyOverflowList() by requiring that the list is empty

Comment 6

[Mats Palmgren (inactive)]

Attachment: Allocate nsFrameList::sEmptyList from the .rodata segment, not the heap

Comment 7

[Mats Palmgren (inactive)]

Attachment: Make frames destroy all child frames and remove their nsFrameList properties when destroyed

Specifically, this wasn't done for the OutsideBullet list which was
destroyed by the property destructor, potentially after the entire
frame tree was gone.  Now we'll assert that the property destructor
isn't called.

Use SafelyDestroyFrameListProp when destroying PushedFloat and
OverflowOutOfFlows frame lists since it's more robust.

Destroy [Excess]OverflowContainers frame lists if they exist, regardless
of the IsFrameOfType(nsIFrame::eCanContainOverflowContainers) bit
(since the oveflow continuation tracker doesn't check that before
creating these lists -- this was the source of crash bugs before
that bit was added to ColumnSetFrame).

Assert in SetPropTableFrames that the property doesn't exist, because
if it does the property destructor will run.

Comment 8

[Mats Palmgren (inactive)]

Attachment: Allocate heap nsFrameLists from the shell arena

The main patch, which is fairly straight-forward now:

"new nsFrameList()" becomes "new (shell) nsFrameList()".
"delete list" becomes "if (list) list->Delete(shell)" - note also that
an additional assertion was added that list is empty when deleted.

"nsAutoPtr<nsFrameList> list(StealSomeFrames())" becomes 
"AutoFrameListPtr list(aPresContext, StealSomeFrames())"

Comment 9

[Mats Palmgren (inactive)]

In summary, the main changes are:
1. allocate (heap) nsFrameLists from the shell arena
2. frame property values that are nsFrameLists are destroyed by the
   frame that owns them, in DestroyFrom, thus the property destructor
   function is never called (this was actually already the case except
   for the OutsideBullet list)

https://tbpl.mozilla.org/?usebuildbot=1&tree=Try&rev=313fcb091ba9

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 730922 [details] [diff] [review]
Use SafelyDestroyFrameListProp also for the OverflowList

r=me

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 730923 [details] [diff] [review]
Simplify DestroyOverflowList() by requiring that the list is empty

r=me

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 730925 [details] [diff] [review]
Allocate nsFrameList::sEmptyList from the .rodata segment, not the heap

The assertion messages still mention sEmptyList.  Fix that?

>+  return *(nsFrameList*)&mozilla::layout::detail::gEmptyFrameListBytes;

reinterpret_cast.

r=me

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 730927 [details] [diff] [review]
Make frames destroy all child frames and remove their nsFrameList properties when destroyed

Hmm.  A bit worried about the perf impact of regetting the prop repeatedly for overflow out-of-flows, but r=me....

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 730930 [details] [diff] [review]
Allocate heap nsFrameLists from the shell arena

>+  void operator delete(void*) MOZ_DELETE;

Shouldn't this have a size_t argument too?

r=me either way, as long as deleting a framelist via delete doesn't compile.

Comment 16

[Mats Palmgren (inactive)]

(In reply to Boris Zbarsky (:bz) from comment #13)
> The assertion messages still mention sEmptyList.  Fix that?

fixed

> reinterpret_cast.

fixed 

(In reply to Boris Zbarsky (:bz) from comment #14)
> Hmm.  A bit worried about the perf impact of regetting the prop repeatedly
> for overflow out-of-flows, but r=me....

Yeah, it's a temporary compromise.  I want to create a frame bit for the
remaining few lists that don't have it, and then gate all lookups on it.
I figured it would help to make frame list handling more uniform.
Filed bug 856630 about it.

(In reply to Boris Zbarsky (:bz) from comment #15)
> >+  void operator delete(void*) MOZ_DELETE;
> 
> Shouldn't this have a size_t argument too?

Either way is fine.  The C++11 Standard says that 'operator delete(void*)'
is the (non-placement) deallocator if it exists, otherwise it's
'operator delete(void*, size_t)' if it exists.

I checked that it fails to compile if used.

Comment 17

[Mats Palmgren (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/37bebc3a719e
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba8a10a35c18
https://hg.mozilla.org/integration/mozilla-inbound/rev/c320c5821f98
https://hg.mozilla.org/integration/mozilla-inbound/rev/489955a7d759
https://hg.mozilla.org/integration/mozilla-inbound/rev/d1f113402481

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/37bebc3a719e
https://hg.mozilla.org/mozilla-central/rev/ba8a10a35c18
https://hg.mozilla.org/mozilla-central/rev/c320c5821f98
https://hg.mozilla.org/mozilla-central/rev/489955a7d759
https://hg.mozilla.org/mozilla-central/rev/d1f113402481