If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Clean up CCGraphBuilder

RESOLVED FIXED in mozilla37

Status

()

Core
XPCOM
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

(Blocks: 1 bug)

unspecified
mozilla37
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments)

(Assignee)

Description

3 years ago
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.
(Assignee)

Comment 1

3 years ago
Encapsulating CCGraphBuilder will make it easier to change how things are done, like bug 1109816 which may add a second NodePool.
(Assignee)

Comment 2

3 years ago
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=f889e7dd8689
(Assignee)

Comment 3

3 years ago
Created attachment 8535010 [details] [diff] [review]
part 1 - Move graph building inside CCGraphBuilder.

This lets us hide a lot of the internal state of graph building from other classes.
Attachment #8535010 - Flags: review?(bugs)
(Assignee)

Comment 4

3 years ago
Created attachment 8535012 [details] [diff] [review]
part 2 - Inline some CCGraphBuilder methods now that the builder is doing the building.
Attachment #8535012 - Flags: review?(bugs)
(Assignee)

Comment 5

3 years ago
Created attachment 8535013 [details] [diff] [review]
part 3 - Make CCGraphBuilder::AddWeakMapNode and ::NoteJSChild methods private.
Attachment #8535013 - Flags: review?(bugs)
(Assignee)

Comment 6

3 years ago
Created attachment 8535014 [details] [diff] [review]
part 4 - Make CCBuilder::AddNode private by adding a new AddPurpleRoot method.
Attachment #8535014 - Flags: review?(bugs)
(Assignee)

Comment 7

3 years ago
Created attachment 8535015 [details] [diff] [review]
part 5 - Inline CCGraphBuilder::DescribeNode because it is silly.
Attachment #8535015 - Flags: review?(bugs)
(Assignee)

Comment 8

3 years ago
I think this isn't going to be needed for bug 1109816, because the approach that would rely on that is too complex.
No longer blocks: 1109816
(Assignee)

Comment 9

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

Updated

3 years ago
Attachment #8535013 - Flags: review?(bugs) → review+

Updated

3 years ago
Attachment #8535015 - Flags: review?(bugs) → review+

Updated

3 years ago
Attachment #8535014 - Flags: review?(bugs) → review+
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.
Attachment #8535012 - Flags: review?(bugs) → review+

Updated

3 years ago
Attachment #8535010 - Flags: review?(bugs) → review+
(Assignee)

Comment 11

3 years ago
(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().
(Assignee)

Comment 12

3 years ago
Created attachment 8535057 [details] [diff] [review]
part 6 - Define CCGraphBuilder::SetFirstChild() for consistency.
Attachment #8535057 - Flags: review?(bugs)

Updated

3 years ago
Attachment #8535057 - Flags: review?(bugs) → review+
(Assignee)

Comment 13

3 years ago
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
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
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
You need to log in before you can comment on or make changes to this bug.