Closed Bug 628209 Opened 9 years ago Closed 9 years ago

Unsigned typed arrays half the speed of regular arrays for simple integer arithmetic

Categories

(Core :: JavaScript Engine, defect, P1)

x86
Windows 7
defect

Tracking

()

RESOLVED FIXED
mozilla2.0b11

People

(Reporter: curiousdannii, Assigned: bzbarsky)

References

(Blocks 1 open bug, )

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(2 files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2.13) Gecko/20101203 Firefox/3.6.13
Build Identifier: Firefox Beta 4.0b9pre

The test page compares typed arrays with regular arrays by incrementing each entry. Typed arrays should not perform worse compared to regular arrays.

Reproducible: Always
Attached file Shell testcase
When I run this with -j, I get:

  18
  35
  39

This is specific to incrementing and reassigning; if I just assign the typed arrays are faster.
Status: UNCONFIRMED → NEW
Ever confirmed: true
For the regular array, we compile JSOP_ADD to:

    addxovi1 = addxovi q2i2, immi3/*1*/ -> exit=0x101127f60 pc=0x1008152c0 imacpc=0x0 sp+32 rp+0 OVERFLOW
    i2d2 = i2d addxovi1
    sti.sp sp[16] = addxovi1

For the typed 32-bit array we compile JSOP_ADD to:

    addd1 = addd ui2d1, immd1/*1*/
    std.sp sp[16] = addd1

And then we end up calling js_DoubleToUint32_1 on it during the JSOP_SETELEM.

This seems to be specific to typed arrays of unsigned ints.  If I look at Int32Array, we end up doing integer arithmetic.  The difference there is that typedArrayElement() is calling ui2d() on the return value for the unsigned cases and i2d() for the signed ones.

For 32-bit unsigned, this is a pain because those may not in fact fit into an int.  But for unsigned 8 and 16-bit, can we get away with just calling i2d() in typedArrayElement?

Alternately, can/should we make alu() work with integers when one of the arguments IsPromotedUint32?  Right now it only works with IsPromotedInt32()...

It's possible that type inference will completely change the landscape here too.
Summary: Typed arrays half the speed of regular arrays → Unsigned typed arrays half the speed of regular arrays for simple integer arithmetic
Depends on: 557407
So I looked at getting alu() to work with promoted uints.  Would it make sense to check for promoted uint, then if it is guard on fitting within an int and convert to int?  Otherwise we need to audit the various code there (e.g., what does LIR_divi do given a uint?).
Nick recently redid the uint promotion code. Did we maybe break some of this?
Possible.  Did we not use to have separate IsPromotedInt32 and IsPromotedUint32?
I'm pretty sure I didn't change the functionality, just the code structure.  

Unsigned ints are really rare -- typed arrays are the only way they come up often -- so I bet we've always done badly with this kind of code.  Compared d2i() with d2u() -- we're obviously just not trying as hard in the latter case, because it hasn't mattered until now.
Yeah, it's unlikely that unsigned int arrays got as much performance testing as did floats.

> For 32-bit unsigned, this is a pain because those may not in fact fit into an
> int.  But for unsigned 8 and 16-bit, can we get away with just calling i2d() in
> typedArrayElement?

Absolutely.  I missed that when I wrote this code, though I guess I wasn't thinking about i2d having more optimization framework than ui2d (given that ui's aren't as common in js).

> Alternately, can/should we make alu() work with integers when one of the
> arguments IsPromotedUint32?  Right now it only works with IsPromotedInt32()...
> 
> It's possible that type inference will completely change the landscape here
> too.

I'd suggest taking the easy ui2d->i2d fix for 8 bit and 16-bit for fx 4, and doing the other bits in a followup bug..
I filed bug 628896 on making alu() work for u2d().
Assignee: general → bzbarsky
Priority: -- → P1
Whiteboard: [need review]
The comment is good, though could be clearer;  I suggest this:

// i2d on purpose here:  it's safe, because an 8-bit uint is guaranteed to fit
// in a 32-bit int, and i2d gets more optimization than ui2d.

And s/8/16/ in the second copy of the comment, obviously.
Made those changes.

Nicholas, want to just review the patch?  ;)
Comment on attachment 507027 [details] [diff] [review]
Do the simple thing for now

Don't pussyfoot around at this late date in the release :-P.

/be
Attachment #507027 - Flags: review?(gal) → review?(nnethercote)
Comment on attachment 507027 [details] [diff] [review]
Do the simple thing for now

r=me with the comment change.
Attachment #507027 - Flags: review?(nnethercote) → review+
Whiteboard: [need review] → [need approval]
Attachment #507027 - Flags: approval2.0?
Attachment #507027 - Flags: approval2.0? → approval2.0+
Whiteboard: [need approval] → [need landing]
Pushed http://hg.mozilla.org/tracemonkey/rev/13ccf78a3eed
Blocks: 467263
Flags: in-testsuite?
Whiteboard: [need landing] → fixed-in-tracemonkey
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b11
You need to log in before you can comment on or make changes to this bug.