857690 - Only use one extra member in JS::CompartmentStats/JS::ZoneStats

Status: RESOLVED FIXED

(Toolkit :: about:memory, defect)

Description

[Nils Maier [:nmaier]]

Having two members, extra1 and extra2, in JS::CompartmentStats only encourages users to add more members (extra3, ...) instead of creating a class holding the extra information and storing a pointer to an instance of it in one extra member.

Hopefully, this will make JS:CompartmentStats easier to use, and the extra information easier to amend to, as I intent to do in bug 846019.

Comment 1

[Nils Maier [:nmaier]]

Attachment: Introduce xpc::ZoneStatsExtras and xpc::CompartmentStatsExtras

Comment 2

[Nils Maier [:nmaier]]

Greenish try: https://tbpl.mozilla.org/?tree=Try&rev=47a1086a21dc
The test failures are due to the amIAddonManager patch, not this one.

Comment 3

[Nicholas Nethercote [inactive]]

Comment on attachment 732944 [details] [diff] [review]
Introduce xpc::ZoneStatsExtras and xpc::CompartmentStatsExtras

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

Lovely.  Thanks for cleaning this up.  Just some nits below.

::: dom/workers/WorkerPrivate.cpp
@@ +1431,3 @@
>  
>      // ReportJSRuntimeExplicitTreeStats expects that
> +    // aZoneStats->extra is a xpc::ZoneStatsExtras pointer

Micro-nit: full stop at end of sentence.

::: js/public/MemoryMetrics.h
@@ +148,5 @@
>  
>  struct ZoneStats
>  {
>      ZoneStats()
> +      : extra(0),

Use NULL instead of 0.

@@ +224,5 @@
>  
>  struct CompartmentStats
>  {
>      CompartmentStats()
> +      : extra(0),

Ditto.

@@ +278,5 @@
>          typeInference(other.typeInference)
>      {
>      }
>  
>      // These fields can be used by embedders.

s/These fields/This field/

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ -2279,5 @@
> -        // cJSPathPrefix is used for almost all the compartment-specific
> -        // reports.  At this point it has the form
> -        // "<something>/compartment/(<cname>)/".
> -        //
> -        // cDOMPathPrefix is used for DOM orphan nodes, which are counted by

You moved this comment, which makes the "At this point..." part untrue.  Can you move it back, and just change cJSPathPrefix to extras->jsPathPrefix, and cDOMPathPrefix to extras->cDOMPathPrefix?

While you're here, the comment's slightly out of date.  It should be "<something>compartment(<cname)/".  Can you fix?

@@ -2282,5 @@
> -        //
> -        // cDOMPathPrefix is used for DOM orphan nodes, which are counted by
> -        // the JS multi-reporter but reported as part of the DOM measurements.
> -        // At this point it has the form "<something>/dom/" if this compartment
> -        // belongs to an nsGlobalWindow, and "explicit/dom/?!/" otherwise (in

And this should be "explicit/dom/<something>?!/".

@@ +2329,5 @@
>  
>      // Report the sums of the compartment numbers.
> +    xpc::CompartmentStatsExtras cExtrasTotal;
> +    cExtrasTotal.jsPathPrefix.AssignLiteral("js-main-runtime/compartments/"),
> +    cExtrasTotal.domPathPrefix.AssignLiteral("window-objects/dom/"),

Can you just pass in the literals to the constructor and assign the fields within it?

@@ +2334,5 @@
> +    rv = ReportCompartmentStats(rtStats.cTotals, cExtrasTotal, cb, closure);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    xpc::ZoneStatsExtras zExtrasTotal;
> +    zExtrasTotal.pathPrefix.AssignLiteral("js-main-runtime/zones/");

Ditto.

::: js/xpconnect/src/xpcpublic.h
@@ +386,5 @@
>  DOM_DefineQuickStubs(JSContext *cx, JSObject *proto, uint32_t flags,
>                       uint32_t interfaceCount, const nsIID **interfaceArray);
>  
> +
> +// ReportJSRuntimeExplicitTreeStats will expect this in the extra1 member

s/extra1/|extra|/

@@ +393,5 @@
> +public:
> +    nsAutoCString pathPrefix;
> +};
> +
> +// ReportJSRuntimeExplicitTreeStats will expect this in the extra1 member

Ditto.

Comment 4

[Nils Maier [:nmaier]]

(In reply to Nicholas Nethercote [:njn] from comment #3)
> ::: js/public/MemoryMetrics.h
> @@ +148,5 @@
> >  
> >  struct ZoneStats
> >  {
> >      ZoneStats()
> > +      : extra(0),
> 
> Use NULL instead of 0.

Not very C++... How about nullptr instead?

> @@ +2329,5 @@
> >  
> >      // Report the sums of the compartment numbers.
> > +    xpc::CompartmentStatsExtras cExtrasTotal;
> > +    cExtrasTotal.jsPathPrefix.AssignLiteral("js-main-runtime/compartments/"),
> > +    cExtrasTotal.domPathPrefix.AssignLiteral("window-objects/dom/"),
> 
> Can you just pass in the literals to the constructor and assign the fields
> within it?

These *Extras classes do not have a custom constructor for now, just the default ones. I'd leave it at that...
Or do you want me to use some struct initializer syntax instead?

Comment 5

[Nils Maier [:nmaier]]

(In reply to Nils Maier [:nmaier] from comment #4)
> > Use NULL instead of 0.
> 
> Not very C++... How about nullptr instead?
> 

That would in turn require to use mfbt/NullPtr.h, which is a no-go for js IIRC?
I'd like to keep Bjarne happy and still use 0, as it was used in the code already.
http://www.stroustrup.com/bs_faq2.html#null

Comment 6

[Nicholas Nethercote [inactive]]

> > Use NULL instead of 0.
> 
> Not very C++... How about nullptr instead?

The JS engine uses NULL everywhere.  Even if you don't like it, it's the local custom.


> These *Extras classes do not have a custom constructor for now, just the
> default ones. I'd leave it at that...

Why not add a custom constructor?  I think it's nicer that way.

Comment 7

[Nils Maier [:nmaier]]

(In reply to Nicholas Nethercote [:njn] from comment #6)
> > These *Extras classes do not have a custom constructor for now, just the
> > default ones. I'd leave it at that...
> 
> Why not add a custom constructor?  I think it's nicer that way.

You'd then need to a) actually have values for all parameters and b) have the values before calling the c'tor and c) might have some unnecessary copying.
I'd just keep it more flexible and in-lieu with JS:CompartmentStats, which do not use parametrized constructors as well.

Comment 8

[Nils Maier [:nmaier]]

Attachment: Introduce xpc::ZoneStatsExtras and xpc::CompartmentStatsExtras

Nits addressed, except for the c'tor one.
However, I added parameterless c'tors and made the *Extras non-copyable.

Comment 9

[Nicholas Nethercote [inactive]]

Comment on attachment 733170 [details] [diff] [review]
Introduce xpc::ZoneStatsExtras and xpc::CompartmentStatsExtras

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

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +2293,5 @@
> +        extras->jsPathPrefix += NS_LITERAL_CSTRING("compartment(") + cName + NS_LITERAL_CSTRING(")/");
> +
> +        // extras->jsPathPrefix is used for almost all the compartment-specific
> +        // reports. At this point it has the form
> +        // "<something>/compartment(<cname>)/".

Remove the '/' after "<something>" -- we can have ".../js-compartment(...)...".

::: js/xpconnect/src/xpcpublic.h
@@ +386,5 @@
>  DOM_DefineQuickStubs(JSContext *cx, JSObject *proto, uint32_t flags,
>                       uint32_t interfaceCount, const nsIID **interfaceArray);
>  
> +
> +// ReportJSRuntimeExplicitTreeStats will expect this in the extra member

Please write "in the |extra| member", to make it clear it's a member name.

@@ +397,5 @@
> +    nsAutoCString pathPrefix;
> +
> +private:
> +    ZoneStatsExtras(const ZoneStatsExtras &other) MOZ_DELETE;
> +    ZoneStatsExtras& operator=(const ZoneStatsExtras &other) MOZ_DELETE;

Um, maybe we're miscommunicating.  I'm suggesting this:

  ZoneStatsExtras(const char *pathPrefix)
    : pathPrefix(pathPrefix)
  {}

(And deleting the copy ctor and operator= isn't necessary.)

Then you can change this code above:

  xpc::ZoneStatsExtras zExtrasTotal;
  zExtrasTotal.pathPrefix.AssignLiteral("js-main-runtime/zones/");

to this:

  xpc::ZoneStatsExtras zExtrasTotal("js-main-runtime/sonzes/");

And a similar thing can be done for CompartmentStatsExtras.

Comment 10

[Nils Maier [:nmaier]]

(In reply to Nicholas Nethercote [:njn] from comment #9)
> Um, maybe we're miscommunicating.  I'm suggesting this:
> 
>   ZoneStatsExtras(const char *pathPrefix)
>     : pathPrefix(pathPrefix)
>   {}

I am not convinced this would be better for this type of class.
This only encourages folks to add new c'tors for most member configurations.
Here alone it would make "sense" to also add ZoneStatsExtras(const nsAString& pathPrefix). Given enough members you would arrive at many initializer combinations and constructors.

Comment 11

[Nils Maier [:nmaier]]

Attachment: For checkin

Last comment nits addressed.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9be5069796d7

Comment 13

[Nicholas Nethercote [inactive]]

> >   ZoneStatsExtras(const char *pathPrefix)
> >     : pathPrefix(pathPrefix)
> >   {}
> 
> I am not convinced this would be better for this type of class.
> This only encourages folks to add new c'tors for most member configurations.
> Here alone it would make "sense" to also add ZoneStatsExtras(const
> nsAString& pathPrefix).

Why would that make sense?  We create objects of this class in a single place.  Therefore, we only need one constructor.  If some hypothetical future in which more than one constructor is needed we can reconsider then, but YAGNI.

Comment 14

[Nils Maier [:nmaier]]

(In reply to Nicholas Nethercote [:njn] from comment #13)
> We create objects of this class in a single place. 
No, we do not. We create instances of this class in three places, some of which are would require a nsAString c'tor.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9be5069796d7