Closed Bug 817493 Opened 8 years ago Closed 8 years ago

Use templates for compartment group finder

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: billm, Assigned: billm)

Details

Attachments

(1 file)

Attached patch patchSplinter Review
Using a virtual function inside JSCompartment is causing problems in bug 747066. This is my fault, and Jon originally wrote the patch using a template. This patch reverts back to using templates and removes the virtual.

I also did some refactoring of the compartment group finder. Now it constructs a single list of all the compartments. The list is ordered so that each group is contiguous. Each compartment has a link to the next compartment and to the first compartment in the next group. This seems easier to understand to me, since the list doesn't get updated after it's first constructed.

Finally, I moved the stack overflow handling to happen at the end of the algorithm rather than in the middle. This seemed easier to reason about.
Attachment #687630 - Flags: review?(jcoppeard)
Comment on attachment 687630 [details] [diff] [review]
patch

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

Looks good!

One comment: in the processNode method, in the loop that adds nodes to the list - all nodes added are in the same component, so I think gcNextGraphComponent can always be set to whatever firstComponent was at the start of the loop.  Also, we don't need to update gcLowLink here any more, that was to record the components before you added gcNextGraphComponent.

You've improved the handling of stack overflow too I see.
Attachment #687630 - Flags: review?(jcoppeard) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef74c1df0080

Just noticed I forgot about the comments. I'll fix that tomorrow.
Flags: needinfo?(wmccloskey)
Whiteboard: [leave open]
https://hg.mozilla.org/mozilla-central/rev/d0b061a3f719
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
You need to log in before you can comment on or make changes to this bug.