1109928 - Clean up CCGraphBuilder

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Andrew McCreight [:mccr8]]

There are a few clean ups that can be done here. The most important is moving all of the graph building logic into a method here, which let's us hide a lot of the internal state of the CCGraphBuilder.  There are also a few methods that can be made private or inlined because they don't do much any more.

Comment 1

[Andrew McCreight [:mccr8]]

Encapsulating CCGraphBuilder will make it easier to change how things are done, like bug 1109816 which may add a second NodePool.

Comment 2

[Andrew McCreight [:mccr8]]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=f889e7dd8689

Comment 3

[Andrew McCreight [:mccr8]]

Attachment: part 1 - Move graph building inside CCGraphBuilder.

This lets us hide a lot of the internal state of graph building from other classes.

Comment 4

[Andrew McCreight [:mccr8]]

Attachment: part 2 - Inline some CCGraphBuilder methods now that the builder is doing the building.

Comment 5

[Andrew McCreight [:mccr8]]

Attachment: part 3 - Make CCGraphBuilder::AddWeakMapNode and ::NoteJSChild methods private.

Comment 6

[Andrew McCreight [:mccr8]]

Attachment: part 4 - Make CCBuilder::AddNode private by adding a new AddPurpleRoot method.

Comment 7

[Andrew McCreight [:mccr8]]

Attachment: part 5 - Inline CCGraphBuilder::DescribeNode because it is silly.

Comment 8

[Andrew McCreight [:mccr8]]

I think this isn't going to be needed for bug 1109816, because the approach that would rely on that is too complex.

Comment 9

[Andrew McCreight [:mccr8]]

These patches are pretty simple, but there's no hurry to get them into the tree either.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8535012 [details] [diff] [review]
part 2 - Inline some CCGraphBuilder methods now that the builder is doing the building.

Don't inline SetLastChild. It is a bit odd method.
But feel free to move its definition to declaration.

Comment 11

[Andrew McCreight [:mccr8]]

(In reply to Olli Pettay [:smaug] from comment #10)
> Don't inline SetLastChild. It is a bit odd method.
> But feel free to move its definition to declaration.

That's a good point.  I wasn't entirely sure that was a good idea.  I think I'll also define a SetFirstChild().

Comment 12

[Andrew McCreight [:mccr8]]

Attachment: part 6 - Define CCGraphBuilder::SetFirstChild() for consistency.

Comment 13

[Andrew McCreight [:mccr8]]

The typed GC API stuff has settled out a bit so I can go ahead and land this.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5a5a932d817b
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/70e929ee0ec6
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/452cc6e826f3
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b30467b66cbb
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/9997dbfbc68a
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/3f56c00d214f

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5a5a932d817b
https://hg.mozilla.org/mozilla-central/rev/70e929ee0ec6
https://hg.mozilla.org/mozilla-central/rev/452cc6e826f3
https://hg.mozilla.org/mozilla-central/rev/b30467b66cbb
https://hg.mozilla.org/mozilla-central/rev/9997dbfbc68a
https://hg.mozilla.org/mozilla-central/rev/3f56c00d214f