Closed Bug 579471 Opened 9 years ago Closed 9 years ago

JM: Special-case |new Array(...)| and other native constructors


(Core :: JavaScript Engine, defect)

Not set





(Reporter: dmandelin, Assigned: bhackett)




(2 files, 5 obsolete files)

Make this fast:

    function f() {
      var a;
      for (var i = 0; i < 42864; ++i) {
        a = new Array();

    var t0 = new Date;
    print(new Date - t0);

We create 42864 arrays in SunSpider. See TraceRecoder::newArray for design ideas.

JM wanted SS win: 22 ms
Blocks: JaegerSpeed
Attached patch fast ctor patch (obsolete) — Splinter Review
Overhead here is split between the slow call done for each 'new' (fetch the prototype property, make a new stack frame, etc.) and the overhead of js_Array itself.

Patch from last week attached (crude, breaks other stuff) which lets constructors be fast natives and fixes the first category (this also applies to other commonly called 'new' natives, like Object/Date).  When the ctor is called with 'new' then 'this' is a magic value, and the native makes the needed object.
Summary: JM: Special-case |new Array(...)| → JM: Special-case |new Array(...)| and other native constructors
Assignee: general → bhackett1024
Attached patch revised fast ctor patch (obsolete) — Splinter Review
This patch does a few things:

Adds a JSCLASS_FAST_CONSTRUCTOR flag to JSAPI which allows the function passed to JS_InitClass to be a FastNative.

Convert Array/Object/String/Date constructors to fast natives (the only ones hit on SS in any abundance).

Modifies InvokeConstructor and SlowNew in the methodjit to call fast natives directly, without constructing a stack frame or new object first.  Instead, the 'this' value is marked as magic, and the native must either see that and fill in the new object (much faster than InvokeConstructor would, skipping the prototype lookup etc.) or call ComputeThisFromVp (as it normally would), making the new object on demand.  In all reasonable cases the former path is taken; the latter is there for weird stuff like 'x = new y.charAt(z)'.

This saves 11.8ms on SS for me, split between 3d-raytrace, crypto-aes, date-format-tofte and date-format-xparb.  There are still big gains to get in object creation, mainly in fixing GC and page fault overhead.
Attachment #457949 - Attachment is obsolete: true
Blocks: 581263
Attached patch non-breaking fast ctor patch (obsolete) — Splinter Review
This patch uses a low bit in JSFunction.flags to distinguish fast natives which can or can't be called using a magic 'this' value with no constructed object.  Doesn't affect the existing API.
Attachment #459642 - Attachment is obsolete: true
See bug 577648's patch (attachment 460169 [details] [diff] [review]), the first jsfun.h hunk, where I define a JSFUN_JOINABLE (0x01). I don't think JSFUN_FAST_CTOR should be in the public jsapi.h (also, name nit: JSFUN_FAST_NATIVE_CTOR seems better).

Attached patch Rebased to current JM (obsolete) — Splinter Review
Re: comment 4, whether the JSFUN_FAST_CTOR and JSCLASS_FAST_CONSTRUCTOR are in jsapi.h depends on whether magic values are considered to be part of the API.  With this patch, one can't write a fast native (whether a class constructor or not) that behaves special when invoked with 'new' if values can't be tested for magic.  Which headers form the API?  There's no JSVAL_IS_MAGIC in jsapi.h, but jsval.h and jsvalue.h stuff about magic values.

Once this gets sorted, who should review this?
Attachment #460383 - Attachment is obsolete: true
(In reply to comment #5)
> Once this gets sorted, who should review this?

I think jorendorff or brendan would be good choices. If their reviewer queues are already too long, I could do it--I've worked on a good deal of function-related stuff.
Attached patch patch without API changes (obsolete) — Splinter Review
This patch rebases and removes all changes to jsapi.h --- the new JSFUN and JSCLASS flags for fast constructors are in jsfun.h.  trace-tests and jstests pass.  After talking with Luke, a good plan seems to be to get this patch in, introducing but not exposing fast constructors, then do a second patch for bug 581263 which puts the fast constructors in the API and removes slow natives/constructors.
Attachment #461582 - Attachment is obsolete: true
Attachment #462413 - Flags: review?(dmandelin)
Comment on attachment 462413 [details] [diff] [review]
patch without API changes

Looks good. 2 small points:

- There is a 'TODO' remaining regarding the usage of flags/holes. This should be removed and filed as a followup bug instead. It seems OK to leave a brief note about the future plan in a comment, possibly even just "This is going to change someday, see bug XXXXXX."

- isMagic should be called with the form that takes an argument, just as an assertion check that we are getting the ctor-call-indicating hole rather than some other random kind.
Attachment #462413 - Flags: review?(dmandelin) → review+
Fix issues from comment 8.
Attachment #462413 - Attachment is obsolete: true
Attachment #462457 - Flags: review?(dmandelin)
Attachment #462457 - Flags: review?(dmandelin) → review+
Depends on: 584199
Use a MIC for native NEW.  There are only about 50k such calls in SS, so this saves ~1ms, but the symmetry with CALL is nice.
Attachment #462623 - Flags: review?(dvander)
Attachment #462623 - Flags: review?(dvander) → review+
Comment on attachment 462623 [details] [diff] [review]
MICs for native new

>+        ncc.masm.storePayload(Imm32(JS_FAST_CONSTRUCTOR), Address(temp, sizeof(Value)));
>+        ncc.masm.storeTypeTag(ImmType(JSVAL_TYPE_MAGIC), Address(temp, sizeof(Value)));

nit: better if we can use masm.storeValue() here, which Sean pointed out is faster on x64.
Closed: 9 years ago
Resolution: --- → FIXED
Depends on: 631135
You need to log in before you can comment on or make changes to this bug.