Closed Bug 914402 Opened 6 years ago Closed 5 years ago

GC: remove the conservative stack scanner and associated machinery

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: terrence, Assigned: terrence)

References

Details

Attachments

(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
RIP
Assignee: nobody → terrence
Status: NEW → ASSIGNED
Attachment #8486758 - Flags: review?(sphink)
Comment on attachment 8486758 [details] [diff] [review]
rip_out_conservative_stack_scanning-v0.diff

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+
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1f12964fe26

Try run at:
https://tbpl.mozilla.org/?tree=Try&rev=fb9db399ea10

It's as green as what's under it:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=b7d2c160e1f0

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
https://hg.mozilla.org/mozilla-central/rev/f1f12964fe26
Status: ASSIGNED → RESOLVED
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.