Last Comment Bug 754480 - MemShrink nsIPresShell/PresShell more
: MemShrink nsIPresShell/PresShell more
Status: RESOLVED FIXED
[MemShrink:P3]
:
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: mozilla15
Assigned To: Mats Palmgren (:mats)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-11 15:36 PDT by Mats Palmgren (:mats)
Modified: 2012-05-17 04:00 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
part 1, remove unused mDragService member (1.74 KB, patch)
2012-05-11 16:13 PDT, Mats Palmgren (:mats)
bzbarsky: review+
Details | Diff | Splinter Review
part 2, sort members in size order. Use narrower types where possible. (23.75 KB, patch)
2012-05-11 16:15 PDT, Mats Palmgren (:mats)
bzbarsky: review+
Details | Diff | Splinter Review
part 3, remove PresShell members associated with mContentToScrollTo (11.40 KB, patch)
2012-05-11 16:19 PDT, Mats Palmgren (:mats)
bzbarsky: review+
Details | Diff | Splinter Review
part 4, remove PresShell::mPrefStyleSheet (18.81 KB, patch)
2012-05-11 16:22 PDT, Mats Palmgren (:mats)
no flags Details | Diff | Splinter Review
part 4, as wdiff (15.89 KB, patch)
2012-05-12 07:56 PDT, Mats Palmgren (:mats)
no flags Details | Diff | Splinter Review

Description Mats Palmgren (:mats) 2012-05-11 15:36:39 PDT

    
Comment 1 Mats Palmgren (:mats) 2012-05-11 16:13:27 PDT
Created attachment 623349 [details] [diff] [review]
part 1, remove unused mDragService member
Comment 2 Mats Palmgren (:mats) 2012-05-11 16:15:52 PDT
Created attachment 623350 [details] [diff] [review]
part 2, sort members in size order.  Use narrower types where possible.

Sort nsIPresShell/PresShell members in size order.

Make all bool fields "bool : 1"

Narrow a few other types as well:
  ScrollAxis::WhenToScroll 32-bit => 16-bit
  mRenderFlags 32-bit => 8-bit
  mChangeNestCount 32-bit => 16-bit  (this counts mutual recursive
   invocations from the frame constructor, and there's usually 4-5
   stack frames per increment so I think we would crash before
   exhausting 16 bits anyway)

Move all the member fields to the end of the class for code readability.
Comment 3 Mats Palmgren (:mats) 2012-05-11 16:19:58 PDT
Created attachment 623352 [details] [diff] [review]
part 3, remove PresShell members associated with mContentToScrollTo

Remove these PresShell members:
  ScrollAxis  mContentScrollVAxis;
  ScrollAxis  mContentScrollHAxis;
  PRUint32    mContentToScrollToFlags;
this is data associated with mContentToScrollTo for a delayed
ScrollContentIntoView.  Put the data in a content property on
mContentToScrollTo instead, and remove it after the scroll happens.
Comment 4 Mats Palmgren (:mats) 2012-05-11 16:22:29 PDT
Created attachment 623355 [details] [diff] [review]
part 4, remove PresShell::mPrefStyleSheet

Remove the PresShell::mPrefStyleSheet member, look it up in mStyleSet when needed instead (the StyleSet owns it).
Comment 5 Mats Palmgren (:mats) 2012-05-11 16:26:12 PDT
With the above changes the size of a PresShell instance goes from:
    sizeof=456 moz_malloc_size_of=456
to
    sizeof=376 moz_malloc_size_of=384

https://tbpl.mozilla.org/?usebuildbot=1&tree=Try&rev=b5bb7ff4e629
Comment 6 Boris Zbarsky [:bz] (TPAC) 2012-05-11 22:04:36 PDT
Comment on attachment 623349 [details] [diff] [review]
part 1, remove unused mDragService member

r=me
Comment 7 Boris Zbarsky [:bz] (TPAC) 2012-05-11 22:09:25 PDT
Comment on attachment 623350 [details] [diff] [review]
part 2, sort members in size order.  Use narrower types where possible.

r=me
Comment 8 Boris Zbarsky [:bz] (TPAC) 2012-05-11 22:15:32 PDT
Comment on attachment 623352 [details] [diff] [review]
part 3, remove PresShell members associated with mContentToScrollTo

r=me
Comment 9 Boris Zbarsky [:bz] (TPAC) 2012-05-11 22:19:29 PDT
Comment on attachment 623355 [details] [diff] [review]
part 4, remove PresShell::mPrefStyleSheet

It's not obvious to me that saving this one word is worth having to grovel through the style set and depending on no extension using a URI like that for the sheet.

If we _do_ want to do this, I'd like to see a -w diff, though.
Comment 10 Mats Palmgren (:mats) 2012-05-12 07:56:40 PDT
Created attachment 623427 [details] [diff] [review]
part 4, as wdiff
Comment 11 Boris Zbarsky [:bz] (TPAC) 2012-05-14 09:26:44 PDT
I'd still like to see some actual numbers indicating that the one-word savings is worth the runtime perf hit....
Comment 12 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-05-14 15:09:27 PDT
Yeah, I wouldn't add any complexity to save a word or two in PresShell.
Comment 13 Mats Palmgren (:mats) 2012-05-16 09:40:45 PDT
Comment on attachment 623355 [details] [diff] [review]
part 4, remove PresShell::mPrefStyleSheet

I don't feel strongly about this one word, so let's skip
this patch for now...

(In reply to Boris Zbarsky (:bz) from comment #9)
> depending on no extension using a URI like that
> for the sheet.

GetUserPrefSheet would return the first sheet with that URI -
I don't think extensions can add sheets before it (AddSheet does
AppendElement).  (We could make sure this sheet is always the first
if necessary.)

(In reply to Boris Zbarsky (:bz) from comment #11)
> I'd still like to see some actual numbers indicating that the one-word
> savings is worth the runtime perf hit....

In a default profile, SheetCount(nsStyleSet::eUserSheet) is 1,
so GetUserPrefSheet would do a single URI string check per call.
The methods involved are called when creating/deleting a new
pres shell making 5 or so calls.  So the perf hit is negligible.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #12)
> Yeah, I wouldn't add any complexity to save a word or two in PresShell.

I think the patch reduces complexity.  The StyleSet owns all the sheets,
so I regard the mPrefStyleSheet alias as added complexity.

I think these pref sheet methods does not belong in pres shell and
should move somewhere else in the long term, at least to a separate file.

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