913881 - More misc. cycle collector cleanups

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Andrew McCreight [:mccr8]]

More misc. cycle collector cleanups I came across while working on ICC, but not directly related to it.

Comment 1

[Andrew McCreight [:mccr8]]

single platform try run: https://tbpl.mozilla.org/?tree=Try&rev=633297ed2418

Comment 2

[Andrew McCreight [:mccr8]]

Attachment: part 1 - Remove unused method nsCycleCollector::Runtime.

Comment 3

[Andrew McCreight [:mccr8]]

Attachment: part 2 - Make a bunch of nsCycleCollector methods private.

Comment 4

[Andrew McCreight [:mccr8]]

Attachment: part 3 - Fix argument name for GCGraphBuilder::AddNode.

Comment 5

[Andrew McCreight [:mccr8]]

Attachment: part 4 - Change nsCycleCollector::ClearGraph to GCGraph::Clear.

Comment 6

[Andrew McCreight [:mccr8]]

Attachment: part 5 - Assert for freeing snow white during cycle collector scanning. r=smaug

Comment 7

[Andrew McCreight [:mccr8]]

Attachment: part 6 - Measure weak map representations in the cycle collector graph. r=njn

Comment 8

[Nicholas Nethercote [inactive]]

Comment on attachment 801670 [details] [diff] [review]
part 6 - Measure weak map representations in the cycle collector graph. r=njn

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

::: xpcom/base/nsCycleCollector.cpp
@@ -611,5 @@
>          *aNodesSize = mNodes.SizeOfExcludingThis(aMallocSizeOf);
>          *aEdgesSize = mEdges.SizeOfExcludingThis(aMallocSizeOf);
> -
> -        // These fields are deliberately not measured:
> -        // - mWeakMaps entries, because the pointers are non-owning

This comment still holds, right?  So the comment could be kept, maybe with modification, something like:  "We don't measure what the mWeakMaps entries point to, because the pointers are non-owning."

@@ +2819,2 @@
>                                        size_t *aWhiteNodeSize,
>                                        size_t *aPurpleBufferSize) const

We should make a struct for this.  But it doesn't have to be now.

Comment 9

[Olli Pettay [:smaug]]

Comment on attachment 801669 [details] [diff] [review]
part 5 - Assert for freeing snow white during cycle collector scanning. r=smaug

Could you change the comment. It is not right in case
aAsyncSnowWhiteFreeing is false.
Maybe, "Don't forget skippable or free snow-white while scan is in progress."

Comment 10

[Andrew McCreight [:mccr8]]

Thanks, I addressed the review comments.  I filed bug 914698 for using structs for the memory reporter arguments.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/431aa2871160
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/50f93200b176
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8ac551efe4e1
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d722282fdc4d
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a36c944c929e
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/fb46afe7f48b

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/431aa2871160
https://hg.mozilla.org/mozilla-central/rev/50f93200b176
https://hg.mozilla.org/mozilla-central/rev/8ac551efe4e1
https://hg.mozilla.org/mozilla-central/rev/d722282fdc4d
https://hg.mozilla.org/mozilla-central/rev/a36c944c929e
https://hg.mozilla.org/mozilla-central/rev/fb46afe7f48b