921923 - Make multi-output sizeOfFoo() functions more consistent

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

Just some more clean-ups to help pave the way for bug 918207.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Reorder StatsZoneCallback and StatsCompartmentCallback.

This makes them appear in the same order in which they are passed to
IterateZonesCompartmentsArenasCells().

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Make multi-output sizeOfFoo() functions more consistent.

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.

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Make multi-output sizeOfFoo() functions more consistent in content/, dom/ and layout/.

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.

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: (part 4) - Make multi-output sizeOfFoo() functions more consistent in gfx/thebes/.

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.

Comment 5

[Jonathan Kew [:jfkthame]]

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 6

[Till Schneidereit [:till]]

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

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

sure

Comment 7

[Till Schneidereit [:till]]

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.

Comment 8

[Till Schneidereit [:till]]

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

Comment 9

[Nicholas Nethercote [inactive]]

Attachment: (part 4b) - fix-ups

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.

Comment 10

[Nicholas Nethercote [inactive]]

Parts 1--3:
https://hg.mozilla.org/integration/mozilla-inbound/rev/71c3fc082038
https://hg.mozilla.org/integration/mozilla-inbound/rev/bfcf75eac943
https://hg.mozilla.org/integration/mozilla-inbound/rev/d1e2767da13f

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/71c3fc082038
https://hg.mozilla.org/mozilla-central/rev/bfcf75eac943
https://hg.mozilla.org/mozilla-central/rev/d1e2767da13f

Comment 12

[Jonathan Kew [:jfkthame]]

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.

Comment 13

[Jonathan Kew [:jfkthame]]

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)

Comment 14

[Nicholas Nethercote [inactive]]

Part 4:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b75e10dac5d

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3b75e10dac5d