Closed Bug 1246855 Opened 4 years ago Closed 4 years ago

Fix up CompartmentPrivate memory reporting


(Core :: JavaScript Engine, defect)

Not set



Tracking Status
firefox47 --- fixed


(Reporter: njn, Assigned: njn)



(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
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().
Bug 1246855 (part 1) - Remove the in-advance measurement of CompartmentPrivates. r=terrence.
Bug 1246855 (part 2) - Measure CompartmentPrivates during memory reporting. r=terrence.
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.