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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Dannii]

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

Comment 1

[Boris Zbarsky [:bzbarsky]]

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

Comment 2

[Boris Zbarsky [:bzbarsky]]

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.

Comment 3

[Boris Zbarsky [:bzbarsky]]

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

Comment 4

[Andreas Gal :gal]

Nick recently redid the uint promotion code. Did we maybe break some of this?

Comment 5

[Boris Zbarsky [:bzbarsky]]

Possible.  Did we not use to have separate IsPromotedInt32 and IsPromotedUint32?

Comment 6

[Nicholas Nethercote [inactive]]

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.

Comment 7

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Do the simple thing for now

Comment 9

[Boris Zbarsky [:bzbarsky]]

I filed bug 628896 on making alu() work for u2d().

Comment 10

[Nicholas Nethercote [inactive]]

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.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Made those changes.

Nicholas, want to just review the patch?  ;)

Comment 12

[Brendan Eich [:brendan]]

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

Comment 13

[Nicholas Nethercote [inactive]]

Comment on attachment 507027 [details] [diff] [review]
Do the simple thing for now

r=me with the comment change.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/tracemonkey/rev/13ccf78a3eed

Comment 15

[Chris Leary [:cdleary] (not checking bugmail)]

cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/13ccf78a3eed