1166709 - for-of loops are ~6x slower with unboxed objects.

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Nicolas B. Pierron [:nbp]]

As reported in bug 1165348 comment 7, using unboxed objects on misc-basic-array-forof [1] cause a ~6x slow down.  (~3.3s vs. ~560ms)


[1] https://github.com/nbp/arewefastyet/blob/iterator-loops/benchmarks/misc/tests/assorted/misc-basic-array-forof.js

Comment 1

[Brian Hackett [Laid off!]]

Attachment: part 1

When a 'new' object is converted from unboxed to native, we invalidate things so that calling 'new' on the same script will create native objects in the future.  There isn't a similar mechanism for initializer objects, and in this case we keep creating unboxed objects in self hosted code for 'for of' and immediately converting them to natives, which is expensive.  The attached patch adds a mechanism for this invalidation, along with some perf improvements for NEWOBJECT with dynamic slots.  This improves my time with unboxed objects from a 10x slowdown to a 20% slowdown.  There is still more to do; after creating the object we have a 'SetPropertyCache; GuardShape; Slots; StoreSlot' series of instructions to initialize the object, rather than 'Slots; StoreSlot; StoreSlot' like we should.

Comment 2

[Brian Hackett [Laid off!]]

Attachment: part 2

Improve type information so that we know the new NEWOBJECT group has definite properties.  This fixes the codegen issue around initializing these objects, but doesn't speed things up any more.  The bottleneck seems to be the larger size of the objects we are creating now --- Slots0 + 8 dynamic slots compared to Slots2 with no dynamic slots.  A couple things:

- We might want to think about lowering SLOT_CAPACITY_MIN sometime.  It made sense when slot arrays were all dynamically allocated, but maybe not so much now that we have a generational GC.  If I reduce this constant to 2, I see a difference of 2% with/without unboxed objects instead of 20%.

- Does scalar replacement work on this testcase?  If I use --ion-scalar-replacement=off it doesn't affect perf for me, whether I am using unboxed objects or not.

Comment 3

[Nicolas B. Pierron [:nbp]]

(In reply to Brian Hackett (:bhackett) from comment #2)
> - Does scalar replacement work on this testcase?  If I use
> --ion-scalar-replacement=off it doesn't affect perf for me, whether I am
> using unboxed objects or not.

Scalar replacement no longer works on iterators because of a spec change, this is what Bug 1165348 address.

Comment 4

[Nicolas B. Pierron [:nbp]]

(In reply to Nicolas B. Pierron [:nbp] from comment #3)
> (In reply to Brian Hackett (:bhackett) from comment #2)
> > - Does scalar replacement work on this testcase?  If I use
> > --ion-scalar-replacement=off it doesn't affect perf for me, whether I am
> > using unboxed objects or not.
> 
> Scalar replacement no longer works on iterators because of a spec change,
> this is what Bug 1165348 address.

Yes, this fix the issue, when Bug 1165348 patch is applied I have the following result:

--ion-scalar-replacement=off   618.037 ms
--ion-scalar-replacement=on    345.105 ms

On the other hand I got the following assertion when testing with a debug build:

Assertion failure: shape->numFixedSlots() == NativeObject::MAX_FIXED_SLOTS || maybeOriginalUnboxedGroup(), at js/src/vm/TypeInference.cpp:2649

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c21058eb90ff

Comment 6

[Brian Hackett [Laid off!]]

The above push is a partial fix for this bug (the full patch had some weird try failures I haven't figured out yet) which maintains the perf improvement on this testcase but isn't generating optimal code quite yet.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c21058eb90ff

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b6c9612de738

Comment 9

[Brian Hackett [Laid off!]]

The above push marks the properties in the replacement group as definite, though the replacement group can still have a different / more appropriate allocation kind than the original unboxed group.  This fixes the codegen issue around initializing these objects.  The full patch still has weird problems on try, and since trying to keep the allocation kinds consistent in these replacement groups is kind of a dubious design strategy anyways, I think this fix should be sufficient for this bug.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b6c9612de738