Make multi-output sizeOfFoo() functions more consistent

RESOLVED FIXED in mozilla27

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla27
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments)

Assignee

Description

6 years ago
Just some more clean-ups to help pave the way for bug 918207.
Assignee

Comment 1

6 years ago
This makes them appear in the same order in which they are passed to
IterateZonesCompartmentsArenasCells().
Attachment #811791 - Flags: review?(till)
Assignee

Updated

6 years ago
Blocks: 918207
Summary: (part 1) - Clean up MemoryMetrics some more → Clean up MemoryMetrics some more
Assignee

Comment 2

6 years ago
We have lots of sizeOfFoo() functions.  Some of them measure multiple things,
either via multiple outparams, or via a struct.  Of those functions, some of
them *assign* the measured values to the params/struct, and some of them
*increment* the params/struct.

This patch does the following.

- Converts them all to the increment form, because that's faster and more
  convenient in StatsCellCallback() (e.g. see the changes to the JSTRACE_OBJECT
  case, where we no longer need a temporary ObjectsExtraSizes class).  

- Changes all their names to addSizeOfFoo() to make the incrementing behaviour
  more obvious.
Attachment #811827 - Flags: review?(till)
Assignee

Comment 3

6 years ago
Till, this is not a JS engine patch but it's straightforward enough and similar
enough to the previous patch that I figure you might as well review it as well.

Note that there are a handful of assignments that I changed to increments.
Turns out this doesn't change any behaviour because those values are always
zero prior to that operation.  Prior to this patch that was unclear, though --
until I checked the values I thought there might be a bug in the summing.  The
new "always add" behaviour is clearer.
Attachment #811830 - Flags: review?(till)
Assignee

Comment 4

6 years ago
This mostly just renames lots of SizeOfFoo() functions as AddSizeOfFoo(), to
follow the new naming scheme.

One non-trivial change is that I'm ignoring the return value of
mFontTableCache->SizeOfExcludingThis(), because it's zero, because
FontTableHashEntry::AddSizeOfEntryExcludingThis() always returns zero.
Attachment #811832 - Flags: review?(jfkthame)
Assignee

Updated

6 years ago
Summary: Clean up MemoryMetrics some more → Make multi-output sizeOfFoo() functions more consistent
Comment on attachment 811832 [details] [diff] [review]
(part 4) - Make multi-output sizeOfFoo() functions more consistent in gfx/thebes/.

Review of attachment 811832 [details] [diff] [review]:
-----------------------------------------------------------------

Mostly this seems fine, but I don't understand the reasoning behind the one "non-trivial" change. OK, it's a long time since I looked at memory-reporting stuff, so perhaps I'm just confused, but if so please enlighten me...

::: gfx/thebes/gfxFT2Fonts.h
@@ +64,5 @@
>  
> +    virtual void AddSizeOfExcludingThis(mozilla::MallocSizeOf aMallocSizeOf,
> +                                        FontCacheSizes*   aSizes) const;
> +    virtual void AddSizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf,
> +                                        FontCacheSizes*   aSizes) const;

nit: might as well lose the extra spaces here, like you've done in other places, as the original alignment of the parameters has long since been lost among renamings.

::: gfx/thebes/gfxFont.cpp
@@ +736,5 @@
>              mCharacterMap->SizeOfIncludingThis(aMallocSizeOf);
>      }
>      if (mFontTableCache) {
> +        (void)mFontTableCache->SizeOfExcludingThis(
> +                FontTableHashEntry::AddSizeOfEntryExcludingThis,

This seems wrong to me... I realize FontTableHashEntry::AddSizeOfEntryExcludingThis itself always returns 0 (for now, at least), but isn't it likely that the hashtable's SizeOfExcludingThis() function still returns non-zero? Surely that would include the size of the entries themselves, not just their "excluding this" sizes, so it's likely to be > 0, and we should include it in the total here.

@@ +747,1 @@
>                                    FontListSizes*    aSizes) const

please fix indent

::: gfx/thebes/gfxFont.h
@@ +775,5 @@
>      // For memory reporter
> +    virtual void AddSizeOfExcludingThis(mozilla::MallocSizeOf aMallocSizeOf,
> +                                        FontListSizes*    aSizes) const;
> +    virtual void AddSizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf,
> +                                        FontListSizes*    aSizes) const;

spaces
Comment on attachment 811791 [details] [diff] [review]
(part 1) - Reorder StatsZoneCallback and StatsCompartmentCallback.

Review of attachment 811791 [details] [diff] [review]:
-----------------------------------------------------------------

sure
Attachment #811791 - Flags: review?(till) → review+
Comment on attachment 811827 [details] [diff] [review]
(part 2) - Make multi-output sizeOfFoo() functions more consistent.

Review of attachment 811827 [details] [diff] [review]:
-----------------------------------------------------------------

While I did try to make sure that changing all these functions to increment instead of assign is correct, I'm trusting you on this to some extent. It certainly does make some code (i.e., in Shape.h) much easier on the eye.
Attachment #811827 - Flags: review?(till) → review+
Comment on attachment 811830 [details] [diff] [review]
(part 3) - Make multi-output sizeOfFoo() functions more consistent in content/, dom/ and layout/.

Review of attachment 811830 [details] [diff] [review]:
-----------------------------------------------------------------

Yup. (Again with the trusting you on incrementing being ok in all of these cases.)
Attachment #811830 - Flags: review?(till) → review+
Assignee

Comment 9

6 years ago
You're right about the hashtable measurement --
mFontTableCache->SizeOfExcludingThis() returns the size of the hashtable's
storage.

This patch applies on top of the previous, and I will fold them together before
landing.  It does the following.

- Reinstates the addition of mFontTableCache->SizeOfExcludingThis().

- Changes gfxFontEntry::FontTableHashEntry::AddSizeOfEntryExcludingThis to
  gfxFontEntry::FontTableHashEntry::SizeOfEntryExcludingThis and makes it
  return a size rather than incrementing a FontListSizes argument.  I did this
  because all the increments were to the same field -- mFontTableCacheSize --
  and so returning a single value is simpler, and avoids the "return 0"
  business.

- Changes gfxFontCache::AddSizeOfExcludingThis() to not ignore the
  return value of mFonts.SizeOfExcludingThis().  I added this value to
  aSizes->mFontTableCache, I'm not sure if that's the best field for it.

- Other minor whitespace and comment changes.
Attachment #812342 - Flags: review?(jfkthame)
Assignee

Updated

6 years ago
Whiteboard: [leave open]
Comment on attachment 812342 [details] [diff] [review]
(part 4b) - fix-ups

Review of attachment 812342 [details] [diff] [review]:
-----------------------------------------------------------------

Looks reasonable, modulo correcting the field name in aSizes (see below).

::: gfx/thebes/gfxFont.cpp
@@ +741,5 @@
>  }
>  
>  void
>  gfxFontEntry::AddSizeOfIncludingThis(MallocSizeOf aMallocSizeOf,
> +                                  FontListSizes* aSizes) const

align indentation

@@ +1613,5 @@
>      // TODO: add the overhead of the expiration tracker (generation arrays)
>  
> +    aSizes->mFontTableCache +=
> +        mFonts.SizeOfExcludingThis(AddSizeOfFontEntryExcludingThis,
> +                                   aMallocSizeOf, aSizes);

aSizes->mFontTableCache doesn't sound right (is there even such a field?). This should be added to aSizes->mFontInstances, I think.
Attachment #812342 - Flags: review?(jfkthame) → review+
Comment on attachment 811832 [details] [diff] [review]
(part 4) - Make multi-output sizeOfFoo() functions more consistent in gfx/thebes/.

r=me (in conjunction with the fixups)
Attachment #811832 - Flags: review?(jfkthame) → review+
https://hg.mozilla.org/mozilla-central/rev/3b75e10dac5d
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.