1029830 - Integrate UCE into GVN

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement)

Description

[Dan Gohman [:sunfish]]

Attachment: gvn-uce.patch

This is the patch split from bug 1004363 which implements UCE functionality within the GVN pass.

It largely works, but it currently still causes unexplained regressions on mochitest browser chrome:

https://tbpl.mozilla.org/?tree=Try&rev=34d9b6627f3a

Comment 1

[Dan Gohman [:sunfish]]

Attachment: gvn-fold-more-control.patch

This is a supplement patch which implements several more control instruction folding rules.

Comment 3

[Dan Gohman [:sunfish]]

Comment on attachment 8445481 [details] [diff] [review]
gvn-uce.patch

Now that the dependencies have landed, I would like to request fuzzing for this patch.

Comment 4

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

I'm behind on stuff, but hope to get to this soon. :-/

Comment 5

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Comment on attachment 8445481 [details] [diff] [review]
gvn-uce.patch

This patch didn't immediately blow up anything, but I'll keep watch.

Comment 6

[Christian Holler (:decoder)]

Comment on attachment 8445481 [details] [diff] [review]
gvn-uce.patch

Found nothing after a day of fuzzing, feedback+ :)

Comment 7

[Dan Gohman [:sunfish]]

I did some investigation of the browser-chrome mochitest failures, and the basic problem seems to be that ValueNumberer::removePredecessor is assuming that if the loopPredecessor() of a loop header is being removed, then the entire loop is unreachable. This assumption is broken in this case by the presence of an OSR entry into a nested loop, inside the first loop.

In order for this to case a problem, there has to be some branch which is constant folded in Ion to go one way, and which actually branches the other way in baseline/interpreter without invalidating the Ion code, and which then does an OSR into the IonCode past the folded branch.

Comment 8

[Dan Gohman [:sunfish]]

Attachment: gvn-asserts.patch

This patch asserts for a bunch of GVN-related things, including asserting that operands are dominated by their producers.

Comment 9

[Dan Gohman [:sunfish]]

Attachment: gvn-rename-discard.patch

This patch is the first in a series which start renaming some things in order to establish a more consistent terminology. "Discard" is used to describe when all references to an instruction or other construct are dropped from the graph. When just one operand is dropped, the producer is not necessarily discarded.

Comment 10

[Dan Gohman [:sunfish]]

Attachment: gvn-rename-insert.patch

This is just a minor renaming for consistency.

Comment 11

[Dan Gohman [:sunfish]]

Attachment: gvn-rename-delete.patch

I previously used "delete" in a lot of the code in this area, but that's actually a misnomer. Instructions and other constructs aren't actually deleted in the C++ sense; they're merely discarded from the graph.

Comment 12

[Dan Gohman [:sunfish]]

Attachment: gvn-misc.patch

This patch is a catch-all for misc cleanups and minor fixes that I collected while debugging the mochitest failures mentioned above.

Comment 13

[Dan Gohman [:sunfish]]

Attachment: gvn-clear-dependencies.patch

Load instructions are now using their dependency() to do store-to-load forwarding, so in preparation for having GVN discard unreachable basic blocks, we need to make sure that instructions which have dependencies on discarded things get handled properly. This patch makes such instructions temporarily depend on themselves, which is admittedly a hack, but it's simple and effective.

Comment 14

[Dan Gohman [:sunfish]]

Attachment: gvn-justgoto-uninteresting.patch

IsDominatorRefined is a function that checks whether a block will get a new dominator parent as a result of graph simplifications. This is to determine whether it will be worthwhile to re-run GVN. If the block contains just a goto, and it does not itself dominate its destination, then updates to its dominator parent are uninteresting.

Comment 15

[Dan Gohman [:sunfish]]

Attachment: gvn-spew.patch

This patch adds a line to spew output mentioning the current block being visited, which is handy context to have, and updates indentation to keep everything pretty.

Comment 16

[Dan Gohman [:sunfish]]

Attachment: gvn-notnot.patch

This patch just adds folds for Test(Not(Not(x))) and Not(Not(Not(x))). This is fun, but there's a practical reason too, which is that previously GVN would fold Test(Not(Not(x))) to Test(Not(x)) with the test successors reversed, and it would miss the opportunity to perform another fold on the test.

Comment 17

[Dan Gohman [:sunfish]]

Attachment: gvn-uce.patch

This is the main updated uce-via-gvn patch. It contains a fix for the browser-chrome mochitest bug mentioned above (see the included testcases and the fixupOSROnlyLoop function for details), as well as several other fixes for bugs exposed by actually folding basic blocks away.

Comment 18

[Dan Gohman [:sunfish]]

Attachment: gvn-fold-more-control.patch

This patch just adds a few more foldsTo rules for folding control instructions. This is roughly the same set of control optimizations that Lowering.cpp currently does, except as noted in a comment.

Comment 21

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8476252 [details] [diff] [review]
gvn-asserts.patch

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

::: js/src/jit/IonAnalysis.cpp
@@ +1786,5 @@
>      for (MBasicBlockIterator block(graph.begin()); block != graph.end(); block++) {
>          count++;
>  
>          JS_ASSERT(&block->graph() == &graph);
> +        JS_ASSERT(!block->isDead());

nit: use MOZ_ASSERT for new assertions.

@@ +1841,5 @@
> +        MOZ_ASSERT(control->block() == *block);
> +        MOZ_ASSERT(!control->hasUses());
> +        MOZ_ASSERT(control->type() == MIRType_None);
> +        MOZ_ASSERT(!control->isDiscarded());
> +        MOZ_ASSERT(!control->isRecoveredOnBailout());

Can you also assert that there is no resume point?
This is not a garanteed fact, but a convenient one for the MNodeIterator.

@@ +1985,5 @@
>  
>          JS_ASSERT(successorWithPhis <= 1);
>          JS_ASSERT((successorWithPhis != 0) == (block->successorWithPhis() != nullptr));
> +
> +        // Verify that phis are dominated by their operands.

This comment sounds wrong to me.  I think you meant that phi's operands are dominating the corresponding predecessor.

Comment 22

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8476266 [details] [diff] [review]
gvn-clear-dependencies.patch

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

::: js/src/jit/ValueNumbering.cpp
@@ +469,5 @@
> +            IonSpew(IonSpew_GVN, "      Will recompute!");
> +            dependenciesBroken_ = true;
> +        }
> +        // Clear its dependency for now, to protect foldsTo.
> +        def->setDependency(def->toInstruction());

Nice catch :)

Comment 23

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8476283 [details] [diff] [review]
gvn-uce.patch

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

I'll continue the reviews in a week, sorry for that.

Comment 24

[Dan Gohman [:sunfish]]

Attachment: gvn-uce.patch

This updates the patch with new resume-point handling code, to account for recent changes on top-of-tree.

Comment 25

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8477924 [details] [diff] [review]
gvn-uce.patch

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

(ongoing review)

::: js/src/jit/MIRGraph.cpp
@@ +1199,5 @@
> +    MOZ_ASSERT(!isLoopHeader());
> +    kind_ = LOOP_HEADER;
> +
> +    size_t numPreds = numPredecessors();
> +    size_t lastIndex = numPreds - 1;

nit: MOZ_ASSERT(numPreds);

::: js/src/jit/ValueNumbering.cpp
@@ +136,5 @@
>  DeadIfUnused(const MDefinition *def)
>  {
> +    return (!def->isEffectful() && !def->isGuard() && !def->isControlInstruction() &&
> +            (!def->isInstruction() || !def->toInstruction()->resumePoint())) ||
> +           def->block()->isMarked();

It sounds that this 'or' belongs to a new function, and this way we would be able to share the code of DeadIfUnused.

@@ +207,5 @@
> +    }
> +    return false;
> +}
> +
> +// Walk up the dominator tree from block to now and test for any defs which look

nit: Add "|" around identifier names, such as |block| and |now|.

@@ +248,1 @@
>      MOZ_ASSERT(old->dominates(now), "Refined dominator not dominated by old dominator");

Move this assertion inside ScanDominatorForDefs(old, now)

@@ +286,5 @@
> +        // (bug 1055690).
> +        if (op->isDiscarded())
> +            continue;
> +        resume->releaseOperand(i);
> +        if (!handleUseReleased(op, DontSetUseRemoved))

We do want to SetUseRemoved in such case.

A ResumePoint is a location where the value could have been observed.  Removing a resume point without setting the SetUseRemoved, implies that we are not aware that there is a path which depends on the original value of of this operand.

This matters for destructive optimizations such as Range Analysis.  If we forget to set the SetUseRemoved flag when removing a resume point, then the branch that we thought to be unreachable might be able to observe a truncated result which is unexpected.

I wonder if the UseRemove flag is badly used, the goal of it was to prevent RangeAnalysis truncation.  It seems to be better suited as a way to prevent truncations by flagging the operands of resume points … I will investigate.

::: js/src/jit/ValueNumbering.h
@@ +82,2 @@
>      bool discardDefsRecursively(MDefinition *def);
> +    bool releaseResumePointOperands(MResumePoint *resume);

I would prefer if all this logic get merged to prepareForDiscard.

Comment 26

[Dan Gohman [:sunfish]]

Attachment: gvn-uce.patch

Thanks for the ongoing review! Attached is an updated and rebased patch.

(In reply to Nicolas B. Pierron [:nbp] from comment #25)
> @@ +286,5 @@
> > +        // (bug 1055690).
> > +        if (op->isDiscarded())
> > +            continue;
> > +        resume->releaseOperand(i);
> > +        if (!handleUseReleased(op, DontSetUseRemoved))
> 
> We do want to SetUseRemoved in such case.
> 
> A ResumePoint is a location where the value could have been observed. 
> Removing a resume point without setting the SetUseRemoved, implies that we
> are not aware that there is a path which depends on the original value of of
> this operand.
> 
> This matters for destructive optimizations such as Range Analysis.  If we
> forget to set the SetUseRemoved flag when removing a resume point, then the
> branch that we thought to be unreachable might be able to observe a
> truncated result which is unexpected.
> 
> I wonder if the UseRemove flag is badly used, the goal of it was to prevent
> RangeAnalysis truncation.  It seems to be better suited as a way to prevent
> truncations by flagging the operands of resume points … I will investigate.

releaseResumePointOperands is only called on resume points in unreachable blocks, and resume points attached to dead instructions. My understanding is that since such resume points will be discarded, they no longer represent places where values are observed. I added a comment mentioning this. Of course, if I've misunderstood something, please correct me.

> ::: js/src/jit/ValueNumbering.h
> @@ +82,2 @@
> >      bool discardDefsRecursively(MDefinition *def);
> > +    bool releaseResumePointOperands(MResumePoint *resume);
> 
> I would prefer if all this logic get merged to prepareForDiscard.

GVN wants to know whether any of these operands are the last use of their producers, so it can add them to the list for discarding. My intuition is that prepareForDiscard isn't yet ready to take on this new functionality, and we can re-evaluate this as the code evolves. However, if you have an idea of what you want the interface to look like, I'll try it out to see how it works.

Comment 27

[Nicolas B. Pierron [:nbp]]

(In reply to Dan Gohman [:sunfish] from comment #26)
> (In reply to Nicolas B. Pierron [:nbp] from comment #25)
> > We do want to SetUseRemoved in such case.
> > 
> > A ResumePoint is a location where the value could have been observed. 
> > Removing a resume point without setting the SetUseRemoved, implies that we
> > are not aware that there is a path which depends on the original value of of
> > this operand.
> 
> releaseResumePointOperands is only called on resume points in unreachable
> blocks, and resume points attached to dead instructions. My understanding is
> that since such resume points will be discarded, they no longer represent
> places where values are observed. I added a comment mentioning this. Of
> course, if I've misunderstood something, please correct me.

This is not true.  We can remove a branch because we are certain that it would not be taken, but the type information might be incomplete and let UCE believe that the branch would never be taken.  As a matter of fact all Recover Instructions tests[1] are based on this trick.

Without the UseRemoved flag, the following test case would fail because of the truncation of the addition and of the truncation of the multiplication which are done before the uceFault branches.  The branch being removed by uceFault cause the entry resume point and the MCompare to disappear, and thus get rid of the only use case which does not expect any truncation.


var uceFault_ra_add = eval(uneval(uceFault).replace('uceFault', 'uceFault_ra_add'));
function ra_add(i) {
    var x = (1 << 16) * (i >>> 1) + 1;
    if (uceFault_ra_add(i) || uceFault_ra_add(i)) {
        assertEq(x > 0, true);
    }
    return x | 0;
}

for (var i = 0; i < 32; i++)
  ra_add(Math.pow(2, i));


I think that the UseRemoved only matters for the removal of entry resume points operands, as they do capture uses which might depends on the plain values.  I am currently testing code to clone the truncated operations such as we can replace the resume point operands by a clone which is recovered on bailouts.

[1] http://dxr.mozilla.org/mozilla-central/source/js/src/jit-test/tests/ion/dce-with-rinstructions.js

Comment 28

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8483522 [details] [diff] [review]
gvn-uce.patch

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

Ok, sorry for the delay.
Still the issue with the UseRemove flag (see previous comment), otherwise, this looks good except for a few nits.

::: js/src/jit/ValueNumbering.cpp
@@ +135,5 @@
>  static bool
>  DeadIfUnused(const MDefinition *def)
>  {
> +    return (!def->isEffectful() && !def->isGuard() && !def->isControlInstruction() &&
> +            (!def->isInstruction() || !def->toInstruction()->resumePoint()));

style-nit: Remove surrounding brackets.

@@ +257,5 @@
>      // newly-dominating definitions which look interesting.
> +    if (block == old)
> +        return block != now && ScanDominatorsForDefs(now);
> +    MOZ_ASSERT(block != now,
> +               "Non-self-dominating block became self-dominating");

style-nit: join this line with the line above. the code is wrapped at 100 columns in SM.

@@ +303,5 @@
> +        // We don't set UseRemoved when discarding the operands because |resume|
> +        // is going to be discarded, so its observation of its operands is no
> +        // longer important to preserve.
> +        if (!handleUseReleased(op, DontSetUseRemoved))
> +            return false;

On the contrary, that's because we removed it that we need the UseRemoved flag.
This means that a branch might have been taken with this result.  The only case where it is fine is if no other resume point captures this definition.

@@ +333,5 @@
>  {
> +    for (size_t o = 0, e = def->numOperands(); o < e; ++o) {
> +        MDefinition *op = def->getOperand(o);
> +        def->releaseOperand(o);
> +        if (!handleUseReleased(op, useRemovedOption))

I think it would be safe to use DontSetUseRemoved here, as the main concern is resume points.

@@ +342,5 @@
>  
>  // Discard |def| and mine its operands for any subsequently dead defs.
>  bool
>  ValueNumberer::discardDef(MDefinition *def,
> +                          UseRemovedOption useRemovedOption)

Does the UseRemoved matters here?  I would think that it does not.

@@ +414,5 @@
>  
> +// If |block| has any predecessors other than |loopPred| which it doesn't
> +// dominate, return the one with the greatest id().
> +static bool
> +hasNonDominatingPredecessor(MBasicBlock *block, MBasicBlock *loopPred)

nit: Update the comment above this function.

@@ +449,5 @@
> +    fake->addNumDominated(1);
> +
> +    // Create zero-input phis to use as inputs for any phis in |block|.
> +    // Again, this is a little odd, but it's the least-odd thing we can do
> +    // without significant complexity.

So, if I understand correctly this is a substitute for a MNop with an unknown return value.  So in terms of generated code, we would be geenerating an unreachable "jump" to a block which is effectively only reachable through OSR?

@@ +504,5 @@
> +    block->removePredecessor(pred);
> +    return true;
> +}
> +
> +// Remove the CFG edge between |pred| and |block, and if this makes |block|

nit: and |block|,

@@ +564,5 @@
> +        if (block->isLoopHeader())
> +            block->clearLoopHeader();
> +        for (size_t i = 0, e = block->numPredecessors(); i < e; ++i) {
> +            if (!removePredecessorAndDoDCE(block, block->getPredecessor(i)))
> +                return false;

Any reason to not delegate this to visitUnreachableBlock?

@@ +570,4 @@
>  
> +        // Clear out the resume point operands, as they can hold things that
> +        // don't appear to dominate them live.
> +        if (MResumePoint *resume = block->entryResumePoint()) {

Can you move this inside, and check if this is an Ion compilation, entryResumePoints might become optional.

@@ +587,5 @@
> +            }
> +#ifdef DEBUG
> +        } else {
> +            MOZ_ASSERT(block->outerResumePoint() == nullptr,
> +                       "Outer resume point in block without an entry resume point");

This might no longer hold for long.  I have a patch which will allow us to get rid of most entryResumePoint.

::: js/src/jit/ValueNumbering.h
@@ +68,2 @@
>      BlockWorklist remainingBlocks_;   // Blocks remaining with fewer preds
> +    MDefinition *nextDef_;            // The next definition; don't discard

nit: Wrap this field between some #ifdef DEBUG

Comment 29

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8476286 [details] [diff] [review]
gvn-fold-more-control.patch

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

::: js/src/jit/MIR.cpp
@@ +289,5 @@
>      if (op->isConstant())
>          return MGoto::New(alloc, op->toConstant()->valueToBoolean() ? ifTrue() : ifFalse());
>  
> +    // It'd be nice to use operandMightEmulateUndefined() in the MIRType_Object
> +    // case, but that flag isn't set yet.

Isn't that provisioned IonBuilder ?

@@ +3418,5 @@
> +MTableSwitch::foldsTo(TempAllocator &alloc)
> +{
> +    MDefinition *op = getOperand(0);
> +
> +    // If we only have one case, convert to a plain goto to the default case.

nit: s/case/successor/; s/default/only/;  , case might be miss-interpreted knowing  that MTableSwitch are produced after a switch-case which can have "case" and "default" keyword.

@@ +3419,5 @@
> +{
> +    MDefinition *op = getOperand(0);
> +
> +    // If we only have one case, convert to a plain goto to the default case.
> +    // Case indices are numeric; other types will always go to the default case.

nit: s/Case/TableSwitch/, switch case can have anything as argument of the case, but we optimized it for integers, by producing a MTableSwitch.

Comment 30

[Dan Gohman [:sunfish]]

Attachment: gvn-uce.patch

> @@ +303,5 @@
> > +        // We don't set UseRemoved when discarding the operands because |resume|
> > +        // is going to be discarded, so its observation of its operands is no
> > +        // longer important to preserve.
> > +        if (!handleUseReleased(op, DontSetUseRemoved))
> > +            return false;
> 
> On the contrary, that's because we removed it that we need the UseRemoved
> flag.
> This means that a branch might have been taken with this result.  The only
> case where it is fine is if no other resume point captures this definition.

Ok, I changed this to use SetUseRemoved.

> @@ +333,5 @@
> >  {
> > +    for (size_t o = 0, e = def->numOperands(); o < e; ++o) {
> > +        MDefinition *op = def->getOperand(o);
> > +        def->releaseOperand(o);
> > +        if (!handleUseReleased(op, useRemovedOption))
> 
> I think it would be safe to use DontSetUseRemoved here, as the main concern
> is resume points.

Done.

> @@ +342,5 @@
> >  
> >  // Discard |def| and mine its operands for any subsequently dead defs.
> >  bool
> >  ValueNumberer::discardDef(MDefinition *def,
> > +                          UseRemovedOption useRemovedOption)
> 
> Does the UseRemoved matters here?  I would think that it does not.

I removed the argument accordingly.

> @@ +449,5 @@
> > +    fake->addNumDominated(1);
> > +
> > +    // Create zero-input phis to use as inputs for any phis in |block|.
> > +    // Again, this is a little odd, but it's the least-odd thing we can do
> > +    // without significant complexity.
> 
> So, if I understand correctly this is a substitute for a MNop with an
> unknown return value.  So in terms of generated code, we would be
> geenerating an unreachable "jump" to a block which is effectively only
> reachable through OSR?

Yes. And we use MPhi instead of MNop because it needs to have a result type.

> @@ +564,5 @@
> > +        if (block->isLoopHeader())
> > +            block->clearLoopHeader();
> > +        for (size_t i = 0, e = block->numPredecessors(); i < e; ++i) {
> > +            if (!removePredecessorAndDoDCE(block, block->getPredecessor(i)))
> > +                return false;
> 
> Any reason to not delegate this to visitUnreachableBlock?

Yeah, we don't want to leave partially-valid loops sitting around. I added a comment.

> @@ +570,4 @@
> >  
> > +        // Clear out the resume point operands, as they can hold things that
> > +        // don't appear to dominate them live.
> > +        if (MResumePoint *resume = block->entryResumePoint()) {
> 
> Can you move this inside, and check if this is an Ion compilation,
> entryResumePoints might become optional.

I can't quite tell what you want me to move this inside of.

> @@ +587,5 @@
> > +            }
> > +#ifdef DEBUG
> > +        } else {
> > +            MOZ_ASSERT(block->outerResumePoint() == nullptr,
> > +                       "Outer resume point in block without an entry resume point");
> 
> This might no longer hold for long.  I have a patch which will allow us to
> get rid of most entryResumePoint.

I assume we can just fix this when your patch lands.

> ::: js/src/jit/ValueNumbering.h
> @@ +68,2 @@
> >      BlockWorklist remainingBlocks_;   // Blocks remaining with fewer preds
> > +    MDefinition *nextDef_;            // The next definition; don't discard
> 
> nit: Wrap this field between some #ifdef DEBUG

nextDef_ is needed outside of DEBUG mode. It's the way we prevent recursive DCE from deleting our iterator out from underneath our traversal.

Comment 31

[Dan Gohman [:sunfish]]

Attachment: gvn-uce.patch

Comment 32

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8489424 [details] [diff] [review]
gvn-uce.patch

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

::: js/src/jit/ValueNumbering.cpp
@@ +351,3 @@
>  
> +    MOZ_ASSERT(def != nextDef_, "Invalidating the MDefinition iterator");
> +    if (def->block()->isMarked()) {

nit: add #ifdef DEBUG around this if.

@@ +592,5 @@
> +                        return false;
> +                }
> +            }
> +#ifdef DEBUG
> +        } else {

nit: Swap these two lines, this would avoid editors' nightmares ;)

@@ +597,5 @@
> +            MOZ_ASSERT(block->outerResumePoint() == nullptr,
> +                       "Outer resume point in block without an entry resume point");
> +            for (MInstructionIterator iter(block->begin()), end(block->end()); iter != end; ++iter)
> +                MOZ_ASSERT(iter->resumePoint() == nullptr,
> +                           "Instruction with resume point in block without entry resume point");

nit: add curly braces around the "for" body.

Comment 33

[Dan Gohman [:sunfish]]

Nits addressed.

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=ce0a75f9481b

Thanks for all the reviews here! And if you have further questions or concerns, please let me know.

Comment 34

[Jan de Mooij [:jandem]]

One of these patches seems to have regressed the AWFY assorted interpreter test (83% slower):

http://arewefastyet.com/#machine=12&view=single&suite=misc&subtest=bugs-608733-interpreter

needinfo in case it's something minor :)

Comment 35

[Dan Gohman [:sunfish]]

I filed bug 1068960, with a patch, for the misc-bugs-608733-interpreter.js regression.

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/69858bc21e7c
https://hg.mozilla.org/mozilla-central/rev/6e1ce34f558a
https://hg.mozilla.org/mozilla-central/rev/2079c454f908
https://hg.mozilla.org/mozilla-central/rev/f212bee81c3c
https://hg.mozilla.org/mozilla-central/rev/f7f1732c8209
https://hg.mozilla.org/mozilla-central/rev/b0f729f0e0d3
https://hg.mozilla.org/mozilla-central/rev/d123ad5677f6
https://hg.mozilla.org/mozilla-central/rev/6e9a69bead52
https://hg.mozilla.org/mozilla-central/rev/ff831540e312
https://hg.mozilla.org/mozilla-central/rev/8f27a48a25d5
https://hg.mozilla.org/mozilla-central/rev/ce0a75f9481b