Closed Bug 824257 Opened 7 years ago Closed 7 years ago

IonMonkey: better codegen for v8-richards schedule() call

Categories

(Core :: JavaScript Engine, defect)

Other Branch
x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: bhackett, Unassigned)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

The critical loop for the v8-richards benchmark is in this function, the task scheduler:

Scheduler.prototype.schedule = function () {
  this.currentTcb = this.list;
  while (this.currentTcb != null) {
    if (this.currentTcb.isHeldOrSuspended()) {
      this.currentTcb = this.currentTcb.link;
    } else {
      this.currentId = this.currentTcb.id;
      this.currentTcb = this.currentTcb.run();
    }
  }
};

Note all those .currentTcb accesses in there.  None of those accesses are folded together, and there are type barriers on all but the one in the loop test (to watch for nulls produced afterwards).  It is, however, ok to fold these together, as the only intervening function call which could modify this.currentTcb is the isHeldOrSuspended call, and that is inlined and does not write anywhere.

If I rewrite this function to manually fold redundant currentTcb accesses:

Scheduler.prototype.schedule = function () {
  this.currentTcb = this.list;
  var why = this.currentTcb;
  while (why != null) {
    if (why.isHeldOrSuspended()) {
      this.currentTcb = why.link;
    } else {
      this.currentId = why.id;
      this.currentTcb = why.run();
    }
    why = this.currentTcb;
  }
};

Combined with the patch in bug 824249, this improves the benchmark score from 10006 (trunk) to 13476.  v8 is 12919, and is unaffected by this modification (i.e. it is already doing this folding).

I'm not sure yet what changes are necessary to ensure we can do this folding automatically, but may cut across several areas (GVN, alias analysis, type barrier elimination) so filing now.
Depends on: 824275
Bug 824275 fixes the redundant loads of this.currentTcb, but there are still type barriers that execute for each of the folded loads.  The attached patch gets rid of these by determining that the barrier is redundant given the earlier non-barriered access and null/undefined test on the result it produces.

This analysis is pretty specialized and is really just the starting point for optimizing away null/undefined checks.  Still, this is a good start with low overhead and does pretty much all that's needed for v8-richards.  Combined with bug 824257 (which this needs) and bug 824249 I get scores on the benchmark of around 13800.
Attachment #695246 - Flags: review?(jdemooij)
Fix dumb mistake (looked for a not-null/undefined test, but didn't check what operand it was actually testing), add a test for the problem.
Attachment #695246 - Attachment is obsolete: true
Attachment #695246 - Flags: review?(jdemooij)
Attachment #695262 - Flags: review?(jdemooij)
Comment on attachment 695262 [details] [diff] [review]
eliminate redundant type barriers

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

This optimization is indeed pretty specialized, but considering the possible wins it seems worth it. The issue below may slightly complicate things though.

::: js/src/ion/IonAnalysis.cpp
@@ +1239,5 @@
> +                        break;
> +                      case JSOP_NE:
> +                      case JSOP_STRICTNE:
> +                        if (direction == TRUE_BRANCH)
> +                            ensuresNotNullOrUndefined = true;

If we have 

if (x.y != null)
    x.y;

Inside the "if", x.y must be non-null and non-undefined. However, if it's a strict comparison, x.y can still be undefined.
Attachment #695262 - Flags: review?(jdemooij)
Attached patch updatedSplinter Review
Oops, good point.  Attached patch corrects for this and adds a test which fails on the earlier patch.
Attachment #695262 - Attachment is obsolete: true
Attachment #695509 - Flags: review?(jdemooij)
Attachment #695509 - Flags: review?(jdemooij) → review+
https://hg.mozilla.org/mozilla-central/rev/78a77949db8e
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Nice. It looks like this was about a 13% win on richards on AWFY. Oddly enough, that happens to cancel out a regression that happened a few weeks ago. Do we know what that regression was yet?
(In reply to David Mandelin [:dmandelin] from comment #7)
> Nice. It looks like this was about a 13% win on richards on AWFY. Oddly
> enough, that happens to cancel out a regression that happened a few weeks
> ago. Do we know what that regression was yet?

I think it's bug 821040.
You need to log in before you can comment on or make changes to this bug.