Closed Bug 1441430 Opened 2 years ago Closed 2 years ago

Provide more detail about atoms memory usage

Categories

(Core :: XPCOM, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: njn, Assigned: njn)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

The old output had a single value "atoms-table". The new output looks like
this:

> 681,264 B (00.37%) -- atoms
> +--332,432 B (00.18%) -- dynamic-atoms
> +--262,176 B (00.14%) -- table
> +---86,656 B (00.05%) -- static-atoms

MozReview-Commit-ID: 924vUmxHAlh
Attachment #8954271 - Flags: review?(nfroyd)
Comment on attachment 8954271 [details] [diff] [review]
Provide more detail about atoms memory usage

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

I trust that you can update this appropriately over bholley's sharded atom table patches.

::: xpcom/ds/nsAtomTable.h
@@ +28,5 @@
> +};
> +} // namespace mozilla
> +
> +void NS_AddSizeOfAtoms(mozilla::MallocSizeOf aMallocSizeOf,
> +                       mozilla::AtomsSizes& aSizes);

With discussions previously about returning values as values, rather than via outparams, should this just return mozilla::AtomsSizes?  Makes for slightly nicer code, and the actual assembly should be just as efficient.
Attachment #8954271 - Flags: review?(nfroyd) → review+
> With discussions previously about returning values as values, rather than
> via outparams, should this just return mozilla::AtomsSizes?  Makes for
> slightly nicer code, and the actual assembly should be just as efficient.

The standard approach for memory reporting functions is to use the return value when measuring a single value, and a by-ref struct for multiple values. I'd prefer to stick to that... returning structs still feels weird.
> The standard approach for memory reporting functions is to use the return
> value when measuring a single value, and a by-ref struct for multiple
> values. I'd prefer to stick to that... returning structs still feels weird.

Also, after rebasing over bholley's patch, the measurement gets split up across multiple functions in a way that returning a struct would be awkward.
Attachment #8954271 - Attachment is obsolete: true
The new version has more detail.
Comment on attachment 8954581 [details]
Bug 1441430 - Provide more detail about atoms memory usage.

https://reviewboard.mozilla.org/r/223672/#review229738
Attachment #8954581 - Flags: review?(nfroyd) → review+
https://hg.mozilla.org/mozilla-central/rev/2e2934676a8b
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.