Closed Bug 914402 Opened 6 years ago Closed 5 years ago

GC: remove the conservative stack scanner and associated machinery


(Core :: JavaScript Engine, defect)

Not set





(Reporter: terrence, Assigned: terrence)




(1 file)

There are several excellent reasons why we should remove the dynamic analysis:

1) The last several issues the dynamic rooting analysis has found have been false positives -- missing skiproots on stack integers, etc. This will be a recurring problem.

2) GGC with gczeal can catch the same problems and in practice has already caught several things that the dynamic root analysis should have caught ages ago. GGC+zeal runs ~2x faster than the dynamic rooting analysis, so it can try twice as many test cases in the same around of time.

3) Removing the dynamic rooting analysis will allow us to remove the conservative stack scanner, killing a bunch of really ugly code.

Arguments for keeping the analysis and counter-argument:

1) It catches rooting hazards on things we do not currently move.
  Reply: adding a configure flag that does move those things and then fuzzing against that configuration is trivial.

2) Root analysis crashes are easier to debug.
  Reply: It depends, but generally the difference is not significant.

3) GGC will re-use object data, potentially quite frequently when using gczeal, making crashes much harder to track down.
  Reply: Correct. This bug is about fixing that problem. The solution should be much less code and much less complexity than conservative scanning and the dynamic root analysis.

I'd like to move forward with making zeal+ggc a viable option right now so that it can help us with landing exact rooting. We can rip out the rest when we remove the conservative stack scanner.
Depends on: 923317
Spoke to Terrence over IRC and there is a chance this will remove the need for the --enable-root-analysis option.
Now that GC zeal mode 7 has landed and is actively finding bugs, I'm morphing this to be our bug for removing the conservative scanner, including the root analysis machinery.
Blocks: 795030
No longer blocks: 773746
Summary: Generational GC: Enhance gczeal in the Nursery and use it to replace the dynamic root analysis → GC: remove the conservative stack scanner and associated machinery
Assignee: terrence → nobody
Assignee: nobody → terrence
Attachment #8486758 - Flags: review?(sphink)
Comment on attachment 8486758 [details] [diff] [review]

Review of attachment 8486758 [details] [diff] [review]:

Sort of sad to see this go, since it's pretty cool to be able to query a random pointer and find out what it is. But I sure don't want to maintain it!
Attachment #8486758 - Flags: review?(sphink) → review+

Try run at:

It's as green as what's under it:

This patch only removes unused code, so the important thing here is that it still builds on all platforms -- I would be very surprised if it caused bustage other than missing symbols.
Duplicate of this bug: 795031
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.