1041126 - Allocation is much slower when using non-default prototype objects.

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Michael Bebenita [:mbx]]

Attachment: new.js

Calling `new F()` when `F.prototype = {x: 0}` is much much slower (10x) than when 'F.prototype.x = 0`.

Comment 1

[Till Schneidereit [:till]]

I.e., creating objects is an order of magnitude slower if the ctor has a custom prototype, regardless of the ctor's body.

maybe related to bug 782906?

Comment 2

[ziyunfei]

http://jsperf.com/non-default-prototype-objects

Comment 3

[Boris Zbarsky [:bzbarsky]]

This only matters for ion-compiled code, right?

The version that didn't change .prototype ends up entirely in inline jitcode as far as I can tell.

The version that changed .prototype ends up with a stubcall to js::CreateThisForFunctionWithProto, which mostly spends its time calling getNewType, NewObjectWithType, and TypeScript::SetThis, as well as a bunch of self time.

The difference seems to be that the former is doing an MCreateThisWithTemplate while the latter is doing a MCreateThisWithProto.

Looking into what actually happens in IonBuilder, here, for the case when proto was not reset we land in createThis().  We have a target function, it's not native, we call createThisScriptedSingleton, getSingletonPrototype returns non-null, we have a template object which has the right proto, so we do an MCreateThisWithTemplate.

When the proto _was_ reset, we get null back from getSingletonPrototype, so end up claling createThisScripted, which does an MGetPropertyCache to get the .prototype of the function and then creates an MCreateThisWithProto.

And the reason getSingletonPrototype returns null is that when we do the getSingleObject(0) bit in HeapTypeSetKey::singleton(), we discover that key->isSingleObject() is false.

None of which has anything to do with GC.  ;)

(On a separate note, the testcase's attempt to console.log is broken in at least two ways: browsers have a "print" function in window scope and console.log expects a Console as a this value.)

Comment 4

[Shu-yu Guo [:shu]]

Boris's analysis is right. The fast path we have is the "singleton prototype" path, which requires:

 1) The constructor function have a singleton type.
 2) Its .prototype property having a typeset with a singleton type in it.

The example satisfies 1) and 2). Seeing how the slow path is a VM call, we can do better when we just satisfy 1). The longest term solution is to implement Kannan's idea of tracking values in typesets. A longer term solution is some kind of custom IC that discriminates on values.

Thing is, the new.js example isn't even supposed to fail 2), because object literals outside of loops in global/eval code are supposed to have singleton types. This is a bug in the object interning code BytecodeEmitter. We try to intern completely static object literals (like { x: 1 } in the example), and emit JSOP_OBJECT over JSOP_NEWOBJECT, but we forget to set singleton types on those objects.

Comment 5

[Shu-yu Guo [:shu]]

Attachment: Set singleton types on interned object literals.

See previous comment for explanation.

Comment 6

[Shu-yu Guo [:shu]]

Attachment: Set singleton types on interned object literals.

Oops, forgot that setSingletonType is fallible.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Does the real-life example the testcase was reduced from have the .prototype set at global scope?

Comment 8

[Shu-yu Guo [:shu]]

(In reply to Boris Zbarsky [:bz] from comment #7)
> Does the real-life example the testcase was reduced from have the .prototype
> set at global scope?

Probably not, but it might still have a singleton type. JSOP_NEWOBJECT can get singleton types, I think?

Comment 9

[Brian Hackett [Laid off!]]

Comment on attachment 8459350 [details] [diff] [review]
Set singleton types on interned object literals.

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

This looks OK but you should restrict this to actual Object instances rather than Arrays, which don't have singleton type in global/eval scope (or anywhere else).  This isn't really a fix though, as it doesn't address the underlying issue and likely doesn't fix any websites or real benchmarks etc. which are actually affected by this issue.  So I don't think this bug should be closed until an IC is added for this (which does seem the best solution; this problem will still exist even with the approach in bug 1038917).

Comment 10

[Shu-yu Guo [:shu]]

(In reply to Brian Hackett (:bhackett) from comment #9)
> Comment on attachment 8459350 [details] [diff] [review]
> Set singleton types on interned object literals.
> 
> Review of attachment 8459350 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks OK but you should restrict this to actual Object instances rather
> than Arrays, which don't have singleton type in global/eval scope (or
> anywhere else).  This isn't really a fix though, as it doesn't address the
> underlying issue and likely doesn't fix any websites or real benchmarks etc.
> which are actually affected by this issue.  So I don't think this bug should
> be closed until an IC is added for this (which does seem the best solution;
> this problem will still exist even with the approach in bug 1038917).

I think the case that Michael cares about here is basically singleton-typed functions with .prototype = object literal, which should, most of the time, be singleton-typed already. So fixing the types for interned objects means our existing optimization should be applying as intended.

I'm not sure we can ever emit JSOP_OBJECTs for array literals, can we? Reading BytecodeEmitter, it doesn't seem like we intern array literals anyhow.

Comment 11

[Shu-yu Guo [:shu]]

Attachment: Set singleton types on interned object literals and fix cloning and XDRing singleton-typed objects.

Requesting re-review since had to fix XDR crashes around singleton-typed objects.

Comment 12

[Brian Hackett [Laid off!]]

Comment on attachment 8468804 [details] [diff] [review]
Set singleton types on interned object literals and fix cloning and XDRing singleton-typed objects.

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

::: js/src/jsobj.cpp
@@ +2203,5 @@
> +    uint32_t isSingletonTyped;
> +    if (mode == XDR_ENCODE)
> +        isSingletonTyped = obj->hasSingletonType() ? 1 : 0;
> +    if (!xdr->codeUint32(&isSingletonTyped))
> +        return false;

You'll need to bump XDR_BYTECODE_VERSION too.

Comment 13

[Shu-yu Guo [:shu]]

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

Comment 14

[Ed Morley [:emorley]]

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

Comment 15

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

The leave-open keyword is there and there is no activity for 6 months.
:sdetar, maybe it's time to close this bug?

Comment 16

[Steven DeTar [:sdetar]]

Shu, any thoughts on what we should do with this old bug that has the leave-open keyword?

Comment 17

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

The leave-open keyword is there and there is no activity for 6 months.
:sdetar, maybe it's time to close this bug?

Comment 18

[Boris Zbarsky [:bzbarsky]]

For what it's worth, the original testcase as filed here seems to be fixed: the times for the two versions are about the same.

That said, those times are about 3x slower than in Chrome...

Comment 19

[Jan de Mooij [:jandem]]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #18)

For what it's worth, the original testcase as filed here seems to be fixed: the times for the two versions are about the same.

That said, those times are about 3x slower than in Chrome...

Hm what I'm seeing here is that this is just testing empty loops now. Ion emits the following for the loop:

    0x2a4c8738f955: 49 bb dc 97 a2 03 01 00 00 00  movabsq $0x103a297dc, %r11        ; imm = 0x103A297DC 
    0x2a4c8738f95f: 41 83 3b 00                    cmpl   $0x0, (%r11)
    0x2a4c8738f963: 0f 85 71 07 00 00              jne    0x2a4c873900da
    0x2a4c8738f969: 81 fe 00 04 00 00              cmpl   $0x400, %esi              ; imm = 0x400 
    0x2a4c8738f96f: 0f 8d 05 00 00 00              jge    0x2a4c8738f97a
    0x2a4c8738f975: 83 c6 01                       addl   $0x1, %esi
    0x2a4c8738f978: eb db                          jmp    0x2a4c8738f955

And d8 has:

    0x1f8060032f0: 41 83 c3 01           addl   $0x1, %r11d
    0x1f8060032f4: 41 81 fb 00 04 00 00  cmpl   $0x400, %r11d             ; imm = 0x400 
    0x1f8060032fb: 0f 83 5f ff ff ff     jae    0x1f806003260
    0x1f806003301: 49 3b a5 c0 12 00 00  cmpq   0x12c0(%r13), %rsp
    0x1f806003308: 77 e6                 ja     0x1f8060032f0
    0x1f80600330a: e9 a2 00 00 00        jmp    0x1f8060033b1

The interrupt check and order of operations is slightly different but overall it looks very similar.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Hmm. You're right; commenting out the allocations does not change the timing.

I guess the speed difference then (for the empty loops) might be something like spectre mitigations?

In any case, this bug as filed seems to be fixed.

Comment 21

[Jan de Mooij [:jandem]]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #20)

I guess the speed difference then (for the empty loops) might be something like spectre mitigations?

Their code is a bit more compact.. If we ever use Cranelift as Ion backend it would be easier to do micro-optimizations.

In any case, this bug as filed seems to be fixed.

Agreed.