1246855 - Fix up CompartmentPrivate memory reporting

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

Bug 1214961 added some memory reporting of CompartmentPrivate. But there are two shortcomings:

- CompartmentPrivate::SizeOfIncludingThis() only measures part of mWrappedJS.

- The measurement happens upon compartment creation. This confuses DMD, which assumes that measurement occurs during reporting, rather than in advance.

Please r? me in the future for patches that involve non-trivial memory reporting changes!

Comment 1

[Nicholas Nethercote [inactive]]

Oh, also:

- The "private-data" measurement in about:memory was put in the "dom" sub-tree instead of the "js-compartment" sub-tree.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Remove the in-advance measurement of CompartmentPrivates

Measuring in advance confuses DMD. It's better to measure during reporting.
The next patch will reinstate the measurement, but during reporting.

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Measure CompartmentPrivates during memory reporting

terrence: "private-data" has this description:

> Extra data attached to the compartment by XPConnect, including
> wrapped-js that is local to a single compartment.

I find this awkward and unclear, for two reasons:

- Is "wrapped-js" an uncountable noun? Or should it be "a wrapped-js" or "the
  wrapped-js" or "each wrapped-js" or something else?

- "that is local to a single compartment" -- this private data belongs to a
  single compartment. Is phrase this redundant?

Alternative ways of writing it would be welcome. Thank you.

Comment 4

[Nicholas Nethercote [inactive]]

I have confirmed that this causes a bunch of heap blocks to move from "Unreported" to "Once-reported" in DMD.

Comment 5

[Terrence Cole [:terrence]]

Comment on attachment 8717305 [details] [diff] [review]
(part 2) - Measure CompartmentPrivates during memory reporting

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

I think either of the new phrasings you proposed is better than what's there now.

Comment 6

[Nicholas Nethercote [inactive]]

In JSCompartment::addSizeOfIncludingThis() I had to change this:

>    JSRuntime* rt = runtimeFromMainThread();
>    auto callback = rt->sizeOfIncludingThisCompartmentCallback;

to this:

>    auto callback = runtime_->sizeOfIncludingThisCompartmentCallback;

to avoid some assertion failures in js::CurrentThreadCanAccessRuntime().

Comment 7

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/24529b78e2eb5ba8f4deb0b61810f248dc1705aa
Bug 1246855 (part 1) - Remove the in-advance measurement of CompartmentPrivates. r=terrence.

https://hg.mozilla.org/integration/mozilla-inbound/rev/99bef2f7a333546e90de4a11094b1ab3f507dcfe
Bug 1246855 (part 2) - Measure CompartmentPrivates during memory reporting. r=terrence.

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/24529b78e2eb
https://hg.mozilla.org/mozilla-central/rev/99bef2f7a333