Closed Bug 1246855 Opened 4 years ago Closed 4 years ago

Fix up CompartmentPrivate memory reporting

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: njn, Assigned: njn)

Details

Attachments

(2 files)

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!
Oh, also:

- The "private-data" measurement in about:memory was put in the "dom" sub-tree instead of the "js-compartment" sub-tree.
Measuring in advance confuses DMD. It's better to measure during reporting.
The next patch will reinstate the measurement, but during reporting.
Attachment #8717304 - Flags: review?(terrence)
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
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.
Attachment #8717305 - Flags: review?(terrence)
I have confirmed that this causes a bunch of heap blocks to move from "Unreported" to "Once-reported" in DMD.
Attachment #8717304 - Flags: review?(terrence) → review+
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.
Attachment #8717305 - Flags: review?(terrence) → review+
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().
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.
https://hg.mozilla.org/mozilla-central/rev/24529b78e2eb
https://hg.mozilla.org/mozilla-central/rev/99bef2f7a333
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.