1800384 - Improve sequential AddAndStoreSlot codegen

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement, P1)

Description

[Doug Thayer [:dthayer] (he/him)]

There's a number of things that are a problem here:

1. We don't need the prebarrier at all
2. We can eliminate redundant slots loads
3. The proto chain shape guards can't be eliminated by the EliminateRedundantShapeGuards pass because the objects don't match the object whose shape is being set by the AddAndStoreSlot

Comment 1

[Doug Thayer [:dthayer] (he/him)]

Attachment: Bug 1800384 - Remove redundant PreBarrier in AddAndStoreSlot r?jandem

This is based on conclusions from sfink and iain. But the gist of this is
that we know there is a reference to the old shape alive here, because
either it is being guarded on by a guard which holds a reference to the
shape, or it was just set by something like AddAndStoreSlot which holds
a reference to the shape.

Comment 2

[Doug Thayer [:dthayer] (he/him)]

Attachment: Bug 1800384 - Eliminate redundant proto shape guards r?jandem

The simplest solution I could find here was to create a new kind of shape
guard with the extra information needed to ensure we could eliminate the
redundant guards in EliminateRedundantProtoShapeGuards. An alternative to
this ugly duplication could be to just include this extra information in
an optional way in GuardShape. That might be better? I'm not sure if the
extra space needed for adding that to every GuardShape would be a
problem though. Suggestions welcome.

Depends on D161962

Comment 3

[Doug Thayer [:dthayer] (he/him)]

Attachment: Bug 1800384 - Eliminate redundant proto shape guards r?iain

Instead of creating a new shape guard op with extra information,
this just creates a new LoadObject and corresponding Constant op
with extra information, relying on the existing shape guard
elimination pass to clean up the shape guards which are now all
relying on commoned up ConstantObjectProtos.

This breaks without a change to congruentIfOperandsEqual, though,
as the ConstantObjectProtos will each have a different GuardShape
dependency, and thus won't look the same. It looks to me to be
safe to just eliminate object guards in congruentIfOperandsEqual,
though? I don't expect this to be particularly expensive either,
so it seems like a good idea to just go ahead and do it so we're
not killing optimization opportunities with slot stores whose
shape guards aren't cleaned up until the eliminate redundant
shapeguards pass well after GVN.

One last thing to note here is I didn't add a separate NurseryObject
op for cases where the object has a nursery index. I actually am
not entirely sure how common that is or what reproduces that - I
kind of expected that in my simple test case the object would only
live in the nursery, as I just loop creating objects with a
constructori, so I'm confused why I don't hit this case. Clearly
I'm misunderstanding something here, so I just left the bit that I
didn't know enough about out of my patch for now.

Comment 4

[BugBot [:suhaib / :marco/ :calixte]]

The following patch is waiting for review from a reviewer who resigned from the review:

ID	Title	Author	Reviewer Status
D161963	Bug 1800384 - Eliminate redundant proto shape guards r?jandem	dthayer	iain: Resigned from review

:dthayer, could you please find another reviewer?

For more information, please visit auto_nag documentation.

Comment 5

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/aed9d4bd5fbd
https://hg.mozilla.org/mozilla-central/rev/77591550134c

Comment 6

[Jan de Mooij [:jandem]]

Doug, is it possible this regressed Kraken's audio-oscillator test a bit?

https://treeherder.mozilla.org/perfherder/graphs?highlightAlerts=1&highlightChangelogData=1&highlightCommonAlerts=0&selected=3870237,1610024395&series=autoland,3870237,1,1&timerange=5184000

(I'm mostly curious if there's an extra bailout or something now that we could avoid.)

Comment 7

[Doug Thayer [:dthayer] (he/him)]

It's definitely responsible for that regression. I'm not sure yet why though.

EDIT: the following is wrong - was profiling the wrong test 🤦‍♂️
Something interesting about that benchmark is it defines a few functions on Array.prototype and makes pretty heavy use of them:

Array.prototype.findGraphNode = function(obj) {
    for(var i=0;i<this.length;i++) {
        if(this[i].pos == obj.pos) { return this[i]; }
    }
    return false;
};
Array.prototype.removeGraphNode = function(obj) {
    for(var i=0;i<this.length;i++) {
        if(this[i].pos == obj.pos) { this.splice(i,1); }
    }
    return false;
};

It looks like the regression holds if I turn MConstantProto into a trivial wrapper for MConstant and treat it as such. I'm wondering if one of the many checks of isConstant() gates a meaningful optimization for that test, which we're now blocking because the instruction isn't an MConstant anymore, even though it could be treated as such.

Comment 8

[Doug Thayer [:dthayer] (he/him)]

Okay scratch that. Most of the difference lives in this function:

Oscillator.prototype.generate = function() {
  var frameOffset = this.frameCount * this.bufferSize;
  var step = this.waveTableLength * this.frequency / this.sampleRate;
  var offset;

  for ( var i = 0; i < this.bufferSize; i++ ) {
    //var step = (frameOffset + i) * this.cyclesPerSample % 1;
    //this.signal[i] = this.func(step) * this.amplitude;
    //this.signal[i] = this.valueAt(Math.round((frameOffset + i) * step)) * this.amplitude; 
    offset = Math.round((frameOffset + i) * step);
    this.signal[i] = this.waveTable[offset % this.waveTableLength] * this.amplitude;
  }

  this.frameCount++;

  return this.signal;
};

Comment 9

[Doug Thayer [:dthayer] (he/him)]

So, prior to this patch, we were able to hoist the proto shape guards out of the loop in LICM. Because the MConstantProtos have operands inside the loop, though, they can't be hoisted, and so we have to guard on them for every iteration. I'm not sure how best to address this - we don't actually use the operands for anything - one is an MConstant which could be replaced by its value, and the other is the receiver object which we don't actually do anything with, and just use to determine if an MGuardShape can be aliased by certain stores. We crash if I try to fudge this in HasOperandInLoop in LICM.cpp - I havent diagnosed this yet but it seems reasonable - if we get moved to a point before one of our operands it seems reasonable that that would be bad.

Comment 10

[Mayank Bansal]

Nice 10% improvement on AWFY-Jetstream2-raytrace* tests:

https://treeherder.mozilla.org/perfherder/graphs?highlightAlerts=1&highlightChangelogData=1&highlightCommonAlerts=0&selected=3911455,1610026840&series=mozilla-central,3738068,1,13&series=mozilla-central,3738068,1,13&series=autoland,3911455,1,13&timerange=5184000&zoom=1669948207728,1670226070618,205.89099702534492,599.9861157119092