754480 - MemShrink nsIPresShell/PresShell more

Status: RESOLVED FIXED

(Core :: Layout, enhancement)

Comment 1

[Mats Palmgren (inactive)]

Attachment: part 1, remove unused mDragService member

Comment 2

[Mats Palmgren (inactive)]

Attachment: 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 (inactive)]

Attachment: 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 (inactive)]

Attachment: 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 (inactive)]

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 [:bzbarsky]]

Comment on attachment 623349 [details] [diff] [review]
part 1, remove unused mDragService member

r=me

Comment 7

[Boris Zbarsky [:bzbarsky]]

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 [:bzbarsky]]

Comment on attachment 623352 [details] [diff] [review]
part 3, remove PresShell members associated with mContentToScrollTo

r=me

Comment 9

[Boris Zbarsky [:bzbarsky]]

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 (inactive)]

Attachment: part 4, as wdiff

Comment 11

[Boris Zbarsky [:bzbarsky]]

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)]

Yeah, I wouldn't add any complexity to save a word or two in PresShell.

Comment 13

[Mats Palmgren (inactive)]

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.

Comment 14

[Mats Palmgren (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/91c4d2dc47d2
https://hg.mozilla.org/integration/mozilla-inbound/rev/f8d21660ac1d
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b6bafa382f6

Comment 15

[Ryan VanderMeulen [:RyanVM]]

(In reply to Mats Palmgren [:mats] from comment #14)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/91c4d2dc47d2
> https://hg.mozilla.org/integration/mozilla-inbound/rev/f8d21660ac1d
> https://hg.mozilla.org/integration/mozilla-inbound/rev/3b6bafa382f6

https://hg.mozilla.org/mozilla-central/rev/91c4d2dc47d2
https://hg.mozilla.org/mozilla-central/rev/f8d21660ac1d
https://hg.mozilla.org/mozilla-central/rev/3b6bafa382f6