Closed
Bug 579471
Opened 14 years ago
Closed 14 years ago
JM: Special-case |new Array(...)| and other native constructors
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
People
(Reporter: dmandelin, Assigned: bhackett1024)
References
Details
Attachments
(2 files, 5 obsolete files)
(deleted),
patch
|
dmandelin
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
dvander
:
review+
|
Details | Diff | Splinter Review |
Make this fast:
function f() {
var a;
for (var i = 0; i < 42864; ++i) {
a = new Array();
}
}
var t0 = new Date;
f();
print(new Date - t0);
We create 42864 arrays in SunSpider. See TraceRecoder::newArray for design ideas.
JM wanted SS win: 22 ms
Reporter | ||
Updated•14 years ago
|
Blocks: JaegerSpeed
Assignee | ||
Comment 1•14 years ago
|
||
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.
Reporter | ||
Updated•14 years ago
|
Summary: JM: Special-case |new Array(...)| → JM: Special-case |new Array(...)| and other native constructors
Assignee | ||
Updated•14 years ago
|
Assignee: general → bhackett1024
Assignee | ||
Comment 2•14 years ago
|
||
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
Assignee | ||
Comment 3•14 years ago
|
||
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
Comment 4•14 years ago
|
||
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).
/be
Assignee | ||
Comment 5•14 years ago
|
||
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
Reporter | ||
Comment 6•14 years ago
|
||
(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.
Assignee | ||
Comment 7•14 years ago
|
||
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)
Reporter | ||
Comment 8•14 years ago
|
||
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+
Assignee | ||
Comment 9•14 years ago
|
||
Fix issues from comment 8.
Attachment #462413 -
Attachment is obsolete: true
Attachment #462457 -
Flags: review?(dmandelin)
Reporter | ||
Updated•14 years ago
|
Attachment #462457 -
Flags: review?(dmandelin) → review+
Assignee | ||
Comment 10•14 years ago
|
||
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)
Updated•14 years ago
|
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.
Comment 12•14 years ago
|
||
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•