Closed Bug 857690 Opened 8 years ago Closed 8 years ago

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

Categories

(Toolkit :: about:memory, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: nmaier, Assigned: nmaier)

References

Details

Attachments

(2 files, 1 obsolete file)

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.
Attachment #732944 - Flags: review?(n.nethercote)
Greenish try: https://tbpl.mozilla.org/?tree=Try&rev=47a1086a21dc
The test failures are due to the amIAddonManager patch, not this one.
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.
Attachment #732944 - Flags: review?(n.nethercote) → review+
(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?
(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
> > 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.
(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.
Nits addressed, except for the c'tor one.
However, I added parameterless c'tors and made the *Extras non-copyable.
Attachment #732944 - Attachment is obsolete: true
Attachment #733170 - Flags: review?(n.nethercote)
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.
Attachment #733170 - Flags: review?(n.nethercote) → review+
(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.
Attached patch For checkinSplinter Review
Last comment nits addressed.
Keywords: checkin-needed
> >   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.
(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.
https://hg.mozilla.org/mozilla-central/rev/9be5069796d7
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.