Closed Bug 1046477 Opened 5 years ago Closed 5 years ago

Separate the reporting of the main and static atoms tables

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: njn, Assigned: njn)

Details

Attachments

(2 files, 3 obsolete files)

Currently we're left guessing as to the relative sizes of these two tables.
Might as well report them separately.
Comment on attachment 8465116 [details] [diff] [review]
Separate the reporting of the dynamic and static atoms tables

Cancelling review; I just realized the static atoms table has its atoms measured as if they are heap-allocated, when they're actually static. I'm surprised this doesn't causes crashes. Anyway, I'll fix and upload the new patch.
Attachment #8465116 - Flags: review?(nfroyd)
This is the same as the earlier version; I've done the statics fix-up in a
follow-up patch.
Attachment #8465133 - Flags: review?(nfroyd)
Attachment #8465116 - Attachment is obsolete: true
Attachment #8465134 - Flags: review?(nfroyd)
> I'm surprised this doesn't causes crashes.

Oh, I know why. These static atoms are always shared, and so our existing "don't measure anything that's shared" guard kicks in, preventing us from actually measuring them. But it's pointless to even try.
After discovering that the contents of gStaticAtomTable is a subset of the
gAtomTable, I'm now calling gAtomTable the "main" atom table instead of the
"dynamic" one.
Attachment #8465213 - Flags: review?(nfroyd)
Attachment #8465133 - Attachment is obsolete: true
Attachment #8465133 - Flags: review?(nfroyd)
Attachment #8465134 - Attachment is obsolete: true
Attachment #8465134 - Flags: review?(nfroyd)
Attachment #8465213 - Flags: review?(nfroyd) → review+
Comment on attachment 8465214 [details] [diff] [review]
(part 2) - Don't measure the heap size of static atoms

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

::: xpcom/ds/nsAtomTable.cpp
@@ +105,5 @@
>  
>    // for |#ifdef NS_BUILD_REFCNT_LOGGING| access to reference count
>    nsrefcnt GetRefCount() { return mRefCnt; }
>  
> +  size_t SizeOfIncludingThis(MallocSizeOf aMallocSizeOf);

It is unfortunate that we had to lose const-ness on this, but I can see why. :(
Attachment #8465214 - Flags: review?(nfroyd) → review+
Summary: Separate the reporting of the dynamic and static atoms tables → Separate the reporting of the main and static atoms tables
https://hg.mozilla.org/mozilla-central/rev/4ce92d6fa22b
https://hg.mozilla.org/mozilla-central/rev/e4f7ab0a0b30
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.