Closed
Bug 802869
Opened 12 years ago
Closed 12 years ago
IonMonkey: Fast path for LIn for Int32 in dense array
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla19
People
(Reporter: sstangl, Assigned: evilpie)
References
(Blocks 1 open bug)
Details
(Whiteboard: [ion:t])
Attachments
(2 files)
440 bytes,
application/x-javascript
|
Details | |
16.50 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
Bug 801915 implemented support for JSOP_IN using callVM, mirroring existing functionality in JM, with the exception of a single fast-path for int32 lookups in dense arrays. The original JM bug for this is Bug 664824.
Reporter | ||
Comment 1•12 years ago
|
||
Note that such a fast-path for IM would involve MInitializedLength and some new MBoundsCheck-like opcode, not more stuff in LIn codegen as in JM.
Comment 2•12 years ago
|
||
This test-case shows that we need the fast path to efficiently compile the self-hosted Array extras in IM. Once bug 784291 is fixed, we will compile them, and in many cases get slower than we are right now. Time in the shell: ~295ms Time with --no-ion: ~62ms
Assignee | ||
Comment 3•12 years ago
|
||
Hey, I have some free time again. I am going to do this!
Assignee: general → evilpies
Status: NEW → ASSIGNED
Assignee | ||
Comment 4•12 years ago
|
||
This was pretty easy to do with our current infrastructure. Jandem: please check if I got the typeset & resume stuff right, my last ion work has been some time now. I will have a look into what needs to be test for this. Times for testcase from comment #2. JM: 52 JM/Ion before: 274 JM/Ion after: 27 So an easy 2x improvement over JM.
Attachment #682781 -
Flags: review?(jdemooij)
Comment 5•12 years ago
|
||
Comment on attachment 682781 [details] [diff] [review] v1 Review of attachment 682781 [details] [diff] [review]: ----------------------------------------------------------------- Looks great, thanks! r=me with nits fixed. Please make sure the tests below pass and add them to the patch: function test1() { var dense = [1, 2, 3]; var denseHoles = [1, , 3]; var result = 0; for (var i=0; i<70; i++) { if (i in dense) result += 1; if (1 in dense) result += 2; if (3 in dense) result += 3; if (-1000 in dense) result += 4; if (i in denseHoles) result += 5; if (1 in denseHoles) result += 6; } assertEq(result, 153); } test1(); function test2() { var a = [1, 2, 3]; for (var i=0; i<70; i++) { assertEq(-0 in a, true); assertEq(1.9 in a, false); assertEq(NaN in a, false); } } test2(); ::: js/src/ion/CodeGenerator.cpp @@ +3907,5 @@ > + Register initLength = ToRegister(lir->initLength()); > + Register output = ToRegister(lir->output()); > + > + // If the index is out of bounds, load |undefined|. Otherwise, load the > + // value. Nit: please update this comment. ::: js/src/ion/MIR.h @@ +3516,5 @@ > > +// Test whether the index is in the array bounds or a hole. > +class MInArray > + : public MTernaryInstruction, > + public SingleObjectPolicy Nit: InArray doesn't need a type policy. @@ +3535,5 @@ > + public: > + INSTRUCTION_HEADER(InArray); > + > + static MInArray *New(MDefinition *elements, MDefinition *index, > + MDefinition *initLength, bool needsHoleCheck) { Nit: fix indentation so that "MDefinition *initLength" lines up with "MDefinition *elements".
Attachment #682781 -
Flags: review?(jdemooij) → review+
Assignee | ||
Comment 6•12 years ago
|
||
Thank you, this was fast and on a weekend! https://hg.mozilla.org/integration/mozilla-inbound/rev/ece6848a46a7
Comment 7•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/ece6848a46a7
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
You need to log in
before you can comment on or make changes to this bug.
Description
•