Note: There are a few cases of duplicates in user autocompletion which are being worked on.

MemShrink nsIPresShell/PresShell more

RESOLVED FIXED in mozilla15

Status

()

Core
Layout
--
enhancement
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Mats Palmgren (vacation - back in August), Assigned: Mats Palmgren (vacation - back in August))

Tracking

Trunk
mozilla15
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink:P3])

Attachments

(5 attachments)

Comment hidden (empty)
Created attachment 623349 [details] [diff] [review]
part 1, remove unused mDragService member
Attachment #623349 - Flags: review?(bzbarsky)
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.
Attachment #623350 - Flags: review?(bzbarsky)
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.
Attachment #623352 - Flags: review?(bzbarsky)
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).
Attachment #623355 - Flags: review?(bzbarsky)
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

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

r=me
Attachment #623349 - Flags: review?(bzbarsky) → review+

Comment 7

5 years ago
Comment on attachment 623350 [details] [diff] [review]
part 2, sort members in size order.  Use narrower types where possible.

r=me
Attachment #623350 - Flags: review?(bzbarsky) → review+

Comment 8

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

r=me
Attachment #623352 - Flags: review?(bzbarsky) → review+

Comment 9

5 years ago
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.
Created attachment 623427 [details] [diff] [review]
part 4, as wdiff

Comment 11

5 years ago
I'd still like to see some actual numbers indicating that the one-word savings is worth the runtime perf hit....
Yeah, I wouldn't add any complexity to save a word or two in PresShell.
Whiteboard: [MemShrink] → [MemShrink:P3]
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.
Attachment #623355 - Flags: review?(bzbarsky)
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
Target Milestone: --- → mozilla15
(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
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.