Closed Bug 1598347 Opened 9 months ago Closed 8 months ago

Property loading and inlining regressions from 1580246

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla73
Tracking Status
firefox-esr68 --- unaffected
firefox71 --- unaffected
firefox72 + fixed
firefox73 + fixed

People

(Reporter: anba, Assigned: cfallin)

References

(Regression)

Details

(Keywords: regression)

Attachments

(2 files)

Regressions from bug 1580246.

var xs = [{a: 0}, {a: 1}];

function f(x) {
    return x.a;
}

var r = 0;
for (var i = 0; i < 10000; ++i) {
    r += f(xs[i & 1]);
}

STR:

  • Compile with IONFLAGS=logs and --ion-offthread-compile=off.
  • Execute iongraph/iongraph --final /tmp/ion.json and iongraph/genpdfs to view the generated MIR nodes.

Comparing the generated MIR shows multiple issues:

  1. f is no longer inlined.
  2. Because f isn't inlined anymore, we need to reload xs's MElements in every iteration.
  3. Loading x.a now emits an extra MGuardShape.
Regressed by: 1580246

Out of curiosity, what prompted you to spot this? Was there some degradation we hadn't spotted in our performance testing, or just your thorough knowledge of the engine made you suspect an issue?

Mostly I want to know how I could have spotted this sort of thing ahead of time (and the answer may well be "Have another 5+ years experience with this stuff" :P )

I was checking whether my changes from https://phabricator.services.mozilla.com/D53174 are still working correctly after bug 1595476. But unfortunately my optimisation doesn't work anymore, so I had to check where and why it regressed. And while doing that, I spotted that property loading emitted an extra MGuardShape, so I then started to investigate which changed added that. (I guessed either bug 1595476 or bug 1580246, so I thankfully didn't need to bisect and recompile multiple times! :-)

Thanks for finding this, :anba! The object-literal work has led us to find a lot of really subtle interactions, and this is surprising, to say the least. I'll take a look.

Assignee: nobody → cfallin

Ah, Ok :) Thanks

To clarify: Is this actually the change that causes your regression, or is it just thing-you-noticed-while-investigating. I'm wondering if this blocks your changes?

I think my current feeling is that we'd like to keep Bug 1580246 in, and address this asynchronously. Does that sound OK with you?

Assignee: cfallin → nobody
Assignee: nobody → cfallin

It looks like my regression stems from bug 1595476, so the regression reported here shouldn't block me.

Awesome. Thanks (both for letting us know it doesn't block you, and for reporting this!)

As far as I can tell so far, the reason that the inlining doesn't occur is that the OptimizationLevel for the outer script with the loop is OptimizationLevel::Normal, while the function f has level OptimizationLevel::DontCompile, so the check at https://searchfox.org/mozilla-central/source/js/src/jit/IonBuilder.cpp#514 prevents inlining. I've been digging into why f gets DontCompile. The groups should be the same as in the baseline case, but it's possible that the recent changed missed some corner case...

I've spent some more time trying to trace this back through Ion and TI to a root cause. From what I can tell so far, a ConstraintDataFreeze constraint is violated when trying to Ion-compile f -- the typeset seen for property a is empty, the ConstraintDataFreeze is created with a null typeset pointer (which I suppose means empty), generateTypeConstraint<ConstraintDataFreeze> then promptly instantiates the typeset and decides that a non-null but empty typeset is not the same as a null typeset, and causes compilation to fail. The real issue is that the typeset is empty, though, and debugging the absence of something is a bit more difficult. From first principles, the two array-element objects in xs should have the same group, because they will both be created by ObjectGroup::newPlainObject which looks up by property names. (The objects are created in BytecodeEmitter::emitObject with singleton-context true, but isInnerSingleton also true, which sets flags such that newPlainObject is called with a TenuredObject alloc-kind.) To debug this further, I would want to printf-instrument the type barrier somehow, and look at what's happening there.

So, the problem appears to be:

  • Prior to ObjLiteral, a top-level tree of {object, array} levels, nested arbitrarily deep, could be emitted as a single JSOP_OBJECT. With ObjLiteral support, we can't emit nested objects-of-arrays-of-objects-of-..., only one level at a time.
  • Prior to ObjLiteral, top-level object literals were created with singleton groups. However, object literals within arrays, or that are stored as properties of parent objects, were allocated simply as tenured objects, with groups chosen by newPlainObject according to a lookup by property list.
  • Hence, prior to ObjLiteral, the above snippet should assign the same group to both of the two objects in the xs array.
  • However, currently, they each get a singleton group.
  • This is because the isInnerSingleton logic in BytecodeEmitter works for objects within other objects (to override the singleton status), but not for objects within arrays.

I'll put together a patch to handle this case. I really don't like that we're basically emulating the incidental logic of the old BytecodeEmitter to determine which of several object allocation paths it would have taken, but it seems to have evolved toward a local maximum with the perf edges sanded smooth in every possible corner case, so that's what we shall do...

Attached patch does not completely solve the issue, but at least ensures that the two objects within the array xs do not have their own singleton groups (this matches what occurs in the pre-ObjLiteral world).

The typeset is still empty at the time of f's Ion-compilation, though, so the remaining difference seems to come from the interaction of TI's array handling and how the objects are stuffed into the array with INITELEM ops (new world) vs. set up at parse time (old world). Someone who understands TI better should probably dig into this further. The only solution involving ObjLiteral would be to handle nested literals (array-of-objects in this case) -- while that would be a very nice extension, it's also fairly involved and not an easy point-fix here.

IMHO, the attached patch is probably worthwhile to avoid any other hidden regressions even if it doesn't solve this issue yet, but I'm open to discussion.

Kannan and I poked at this a bit.

It looks like there is some pre-existing weirdness here that is being exposed by the original patch. By changing the definition of xs in the original testcase as follows:

// var xs = [{a: 0}, {a: 1,b:1}];
var a = {a: 0};
var b = {a: 1};
var xs = [a,b];

... I can replicate the same problem. Specifically: in PropertyReadNeedsTypeBarrier, we are accessing the "a" property of a singleton object with a lazy group. Because the group doesn't exist yet, maybeTypes is a null pointer. At the end of the function, we freeze the property, which sets |expected| to nullptr.

During |FinishCompilation|, we try to generate a type constraint for that freeze. This does two things: first it instantiates the property (aka delazifies the group), then it checks to make sure that the constraint holds. However, when we instantiate the property, we give it a non-null typeset. When we check to make sure the constraint holds, the new non-null typeset does not match the nullptr stored in |expected|, and we fail the compile of |f|, which later prevents us from inlining it.

This seems sub-optimal. It seems like the lazy-group singleton should be delazified earlier, although I'm not sure precisely when.

(Also, the remaining problem after Chris's latest patch is still a mystery.)

Priority: -- → P1
Attachment #9110726 - Attachment description: Bug 1598347: WIP: Slightly improve correspondence to pre-ObjLiteral world by handling the inner-singleton case for array elem objs. → Bug 1598347, part 1: Handle the inner-singleton case for array elem objs. r=djvj,iain

This change passes through the "inner singleton" status of a particular
object literal to the group-assignment logic for JSOP_NEWOBJECT and uses
this flag bit to do a different thing (use the group from the template
object). "Inner singleton" status is meant to emulate old behavior (prior to
ObjLiteral) in which the frontend built an entire tree of objects/arrays
with a top-level singleton. In the old world, these objects were
allocated by ParseNode::getConstantValue() in the frontend and built by
ObjectGroup::newPlainObject, which looked up an object group by property
names. In the new world, the default NEWOBJECT logic uses a separate
test for whether an allocation site should have a singleton group, and
even after we carefully emulate the old group behavior in the frontend,
the new-object allocation itself will decide to allocate singleton
groups. In the particular regression case motivating this change, these
singleton groups break the TI information for a singleton array (typeset
for all elems no longer has a single group) and as a result, a simple
property access can no longer be inlined. This patch matches the old
behavior and allows the inlining to occur.

Depends on D54236

Just attached a two-part patch that seems to resolve this issue, at least according to my local JIT-spew output for this test (f gets inlined).

The second half of the problem had to do with some logic for the JSOP_NEWOBJECT case -- that is, allocating an object from a template -- which is used for, among many other cases, the "inner singleton" case we have here. In particular, the NEWOBJECT logic would independently decide to allocate a singleton group according to some heuristics, irrespective of the group of the template object:

https://searchfox.org/mozilla-central/source/js/src/vm/Interpreter.cpp#5200

As a result, the ObjLiteral logic in the frontend was properly allocating the two objects in xs to have the same group, but they were getting brand-new singleton groups later.

The second patch of the two-patch stack implements a horrible hack: it steals the MSB of the 32-bit GC-thing index to indicate that the object is in "inner singleton" context. If this bit is set, then the execution-time logic will directly take the group from the template object, regardless of any other of ObjectGroup's heuristics. This is sufficient to ensure that the array-elem HeapTypeSet on the array has only one element, which lets f get Ion-compiled and subsequently inlined.

I'll run the usual try-run tests to make sure this doesn't break anything else. :anba, could you confirm that this fixes the regression you see?

Flags: needinfo?(andrebargull)

Here's the try run and baseline.

Comparisons: Talos (Kraken), Raptor (Speedometer, TP6), Octane, AWSY. All the deltas are low confidence / in the noise. All tests are green on the builds. So this seems safe and ready for review, I think, unless anyone disagrees with the approach?

(In reply to Chris Fallin [:cfallin] from comment #14)

:anba, could you confirm that this fixes the regression you see?

Yes, this fixes the regression and only a single LoadFixedSlotAndUnbox is used instead of GuardShape + LoadFixedSlotT. Thanks! :-)

Flags: needinfo?(andrebargull)
Pushed by iireland@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6da3c648995f
part 1: Handle the inner-singleton case for array elem objs. r=djvj,iain
https://hg.mozilla.org/integration/autoland/rev/7bd0784e7a6f
part 2: pass "inner singleton" to NEWOBJECT group logic. r=djvj,iain

Please request uplift when this is on m-c.

Status: NEW → RESOLVED
Closed: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla73

(In reply to Jan de Mooij [:jandem] from comment #19)

Please request uplift when this is on m-c.

Flags: needinfo?(cfallin)

Hi :jcristau, yes, could we request an uplift? (I don't know if there is any other procedure for this -- do we just ask you?)

This bug's patches should go in along with that from bug 1601074. Thanks!

Flags: needinfo?(cfallin)

You can click the "details" link on the attachment in bugzilla, flip the "approval-mozilla-beta" flag to "?", and fill in the form.

Comment on attachment 9110726 [details]
Bug 1598347, part 1: Handle the inner-singleton case for array elem objs. r=djvj,iain

Beta/Release Uplift Approval Request

  • User impact if declined: Slight regression in performance (fewer JS functions are inlined) in some cases.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: Bug 1601074
  • Risk to taking this patch: Medium
  • Why is the change risky/not risky? (and alternatives if risky): Changes to the guts of the JS engine are always slightly risky, but this has been on m-c for a few days, and the one issue that arose (1601074) was caught quickly by the fuzzing team and fixed. Given that no further issues have arisen, and given that the engine is well-covered by tests, we believe that this risk is not unacceptably high.
  • String changes made/needed:
Attachment #9110726 - Flags: approval-mozilla-beta?
Attachment #9111379 - Flags: approval-mozilla-beta?

== Change summary for alert #24215 (as of Tue, 03 Dec 2019 02:14:58 GMT) ==

Improvements:

2% kraken windows7-32-shippable opt e10s stylo 1,014.51 -> 993.33

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=24215

Comment on attachment 9110726 [details]
Bug 1598347, part 1: Handle the inner-singleton case for array elem objs. r=djvj,iain

spidermonkey perf regression fix, approved for 72.0b4

Attachment #9110726 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9111379 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Regressions: 1601996
See Also: → 1619007
You need to log in before you can comment on or make changes to this bug.