Closed Bug 1041126 Opened 5 years ago Closed 6 months ago

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

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: mbx, Assigned: shu)

Details

Attachments

(2 files, 2 obsolete files)

Attached file new.js
Calling `new F()` when `F.prototype = {x: 0}` is much much slower (10x) than when 'F.prototype.x = 0`.
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?
Component: JavaScript Engine → JavaScript: GC
OS: Mac OS X → All
Hardware: x86 → All
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.)
Component: JavaScript: GC → JavaScript Engine: JIT
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.
See previous comment for explanation.
Attachment #8459349 - Flags: review?(bhackett1024)
Oops, forgot that setSingletonType is fallible.
Attachment #8459349 - Attachment is obsolete: true
Attachment #8459349 - Flags: review?(bhackett1024)
Attachment #8459350 - Flags: review?(bhackett1024)
Does the real-life example the testcase was reduced from have the .prototype set at global scope?
(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 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).
Attachment #8459350 - Flags: review?(bhackett1024) → review+
(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.
Assignee: nobody → shu
Status: NEW → ASSIGNED
Keywords: leave-open
Requesting re-review since had to fix XDR crashes around singleton-typed objects.
Attachment #8459350 - Attachment is obsolete: true
Attachment #8468804 - Flags: review?(bhackett1024)
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.
Attachment #8468804 - Flags: review?(bhackett1024) → review+
The leave-open keyword is there and there is no activity for 6 months.
:sdetar, maybe it's time to close this bug?
Flags: needinfo?(sdetar)
Shu, any thoughts on what we should do with this old bug that has the leave-open keyword?
Flags: needinfo?(sdetar) → needinfo?(shu)

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

Flags: needinfo?(sdetar)

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...

Flags: needinfo?(jdemooij)

(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.

Flags: needinfo?(shu)
Flags: needinfo?(sdetar)
Flags: needinfo?(jdemooij)

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.

(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.

Status: ASSIGNED → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.