Last Comment Bug 751227 - Remove nsPresShell::mStackArena, make it a separate stack class
: Remove nsPresShell::mStackArena, make it a separate stack class
Product: Core
Classification: Components
Component: Layout (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla15
Assigned To: Mats Palmgren (:mats)
: Jet Villegas (:jet)
Depends on:
  Show dependency treegraph
Reported: 2012-05-02 09:51 PDT by Mats Palmgren (:mats)
Modified: 2012-05-10 19:16 PDT (History)
5 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

fix (26.43 KB, patch)
2012-05-02 10:06 PDT, Mats Palmgren (:mats)
no flags Details | Diff | Splinter Review
fix, v2 (26.79 KB, patch)
2012-05-03 10:30 PDT, Mats Palmgren (:mats)
roc: review+
Details | Diff | Splinter Review

Description Mats Palmgren (:mats) 2012-05-02 09:51:23 PDT
nsPresShell::mStackArena takes up a lot of space permanently even though
it's only used as temporary storage in a couple of XUL layout methods.

Before this patch, sizeof(nsPresShell) is 552 making jemalloc allocate
a 1024 byte block for it.  Additionally, mStackArena is a 4096 block
even when it's not actually used.  (This is on 64-bit Linux)

With the patch, sizeof(nsPresShell) is 512, fitting exactly into a 512
jemalloc chunk.  mStackArena is removed, so the total saving is 4.6kB.
Comment 1 Mats Palmgren (:mats) 2012-05-02 10:06:41 PDT
Created attachment 620361 [details] [diff] [review]

The StackArena/StackBlock/StackMark code is verbatim copies, except that
I made StackArena completely private and AutoStackArena a friend to forbid
any direct use of StackArena.
Comment 2 Mats Palmgren (:mats) 2012-05-02 10:07:20 PDT
I'm not sure the two places where this code is used really justifies its
existence.  The only benefit I see is that it coalesces the alloc/free
of (presumably) many small objects, and it avoids the realloc copying in
nsTArray f.e. should we use that instead.  Do we have some collection type
that use "roped" multiple buffers internally to avoid copying?
Comment 3 Mats Palmgren (:mats) 2012-05-02 10:09:35 PDT
> in a couple of XUL layout methods

One XUL and one Table layout method, actually.
Comment 4 Mats Palmgren (:mats) 2012-05-02 10:13:24 PDT
Try results pending, together with bug 750745 patches:
Comment 5 Mats Palmgren (:mats) 2012-05-02 13:53:25 PDT
> Before this patch, sizeof(nsPresShell) is 552 ... (This is on 64-bit Linux)

Uhm, a Linux64 debug build to be exact, so the clown shoes on nsPresShell
is likely less of a problem in release builds, I'll check what the real
numbers are.  The 4096 bytes mStackArena size should be accurate though --
those structs does not have DEBUG-only members.
Comment 6 Mats Palmgren (:mats) 2012-05-02 14:25:29 PDT
FYI, I found bug 486066 with discussion about removing the StackArena class.
Comment 7 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-05-02 15:50:58 PDT
Comment on attachment 620361 [details] [diff] [review]

Review of attachment 620361 [details] [diff] [review]:

Looks good!

::: layout/base/StackArena.h
@@ +87,5 @@
> +    return mStackArena->Allocate(aSize);
> +  }
> +
> +private:
> +  static StackArena* mStackArena;


::: layout/tables/SpanningCellSorter.h
@@ +76,5 @@
>       */
>      Item* GetNext(PRInt32 *aColSpan);
>  private:
>      nsIPresShell *mPresShell;
> +    mozilla::AutoStackArena mArena;

Putting this in the class is a bit dangerous because it permits non-stack behavior. How about moving it to a local variable in some function?
Comment 8 Mats Palmgren (:mats) 2012-05-02 16:00:58 PDT
Doesn't NS_STACK_CLASS on SpanningCellSorter take care of that?
AutoStackArena should probably have NS_STACK_CLASS too, but I wasn't sure if all
compilers would grok that.
Comment 9 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-05-02 17:05:52 PDT
I think I'd rather have NS_STACK_CLASS on AutoStackArena and put it explicitly in a method's stack frame.
Comment 10 Mats Palmgren (:mats) 2012-05-03 10:30:34 PDT
Created attachment 620762 [details] [diff] [review]
fix, v2

Nits fixed.
Comment 11 Mats Palmgren (:mats) 2012-05-03 15:18:38 PDT
The size of nsPresShell in an Opt build on Linux64:
  before: sizeof=496 moz_malloc_size_of=504
  after:  sizeof=456 moz_malloc_size_of=456
so the total saving is (with the 4096 of mStackArena) 4144 bytes.

After shuffling fields around, and narrowing the size of some, I get:
  sizeof=408 moz_malloc_size_of=408
I'll file a separate bug for that.
Comment 12 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-05-03 15:56:06 PDT
Comment on attachment 620762 [details] [diff] [review]
fix, v2

Review of attachment 620762 [details] [diff] [review]:

::: layout/tables/BasicTableLayoutStrategy.cpp
@@ +264,5 @@
>  {
>      nsTableFrame *tableFrame = mTableFrame;
>      nsTableCellMap *cellMap = tableFrame->GetCellMap();
> +    mozilla::AutoStackArena mArena;

Just call this "arena" since it's not a member.
Comment 13 Mats Palmgren (:mats) 2012-05-03 17:16:50 PDT
> Just call this "arena" since it's not a member.

Thanks, fixed.
Comment 14 Ed Morley [:emorley] 2012-05-04 10:57:11 PDT
Comment 15 Nicholas Nethercote [:njn] 2012-05-10 16:06:31 PDT
How many nsPresShells do we have live at a time?
Comment 16 Boris Zbarsky [:bz] (still a bit busy) 2012-05-10 17:42:35 PDT
One per document being rendered.
Comment 17 Mats Palmgren (:mats) 2012-05-10 18:27:07 PDT
It seems we keep a few in the bfcache as well.  Here's the results I get:
start up with 1 tab (about:home)
  3 shells
load in that tab --, about:home, ...
 +1 for each page up to 6
create a new tab (about:home):
 +1 (total 7)
loading in tab 2 --, about:home, ...
 +1 for each page up to 10
Comment 18 Boris Zbarsky [:bz] (still a bit busy) 2012-05-10 18:34:37 PDT
Oh, right.  One per document being shown or in bfcache.  ;)
Comment 19 Mats Palmgren (:mats) 2012-05-10 19:16:44 PDT
Yeah, and not to forget, each window has a XUL document.

I was quite impressed to see that "Minimize memory usage" in about:memory
reclaims the ones in bfcache.

Note You need to log in before you can comment on or make changes to this bug.