582974 - Slow property ops on "water" testcase

Status: RESOLVED WORKSFORME

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

We're slower than Chrome on this testcase, and about 45% of the time is in jseng.  It's mostly on trace (42% under ExecuteTree), and breaks down like so (all percentages of total testcase time):

8% Running jitted code
18% SetPropertyByIndex calling js_SetProperty; about half of this is under
    JSScope::putProperty adding properties, and the other half is
    js_LookupPropertyWithFlags, js_NativeSet, time spent in js_SetPropertyHelper
    itself, etc.
8% GetPropertyByIndex (js_LookupPropertyWithFlags called from
   js_GetPropertyHelper here, as well as some other random stuff).

Are we missing the propcache?  Would PICs in JM help here?

The testcase is attached in bug 582973 as well as linked in the url field

Comment 1

[Brendan Eich [:brendan]]

This must mean setters -- cc'ing jorendorff.

bz: JSScope::putProperty if it doesn't tail-call JSScope::addPropertyHelper right away is not adding properties, it is changing existing (defined as bound by id in the object) properties.

/be

Comment 2

[Boris Zbarsky [:bzbarsky]]

I don't think there are any setters here.  Just array indexing... using negative indices at times, so producing slow arrays.  At least that's what I see us hitting js::TraceRecorder::initOrSetPropertyByIndex and js::TraceRecorder::getPropertyByIndex for.  Example |obj| when coming through the latter:

(gdb) p obj->isDenseArray()
$20 = false
(gdb) call js_DumpObject(obj)
object 0x1b221a80
class 0x5de6a0 Array
properties:
    enumerate 271: slot 280
    enumerate 236: slot 279
    enumerate 235: slot 278
    enumerate 234: slot 277
    enumerate 233: slot 276
    enumerate 232: slot 275
    enumerate 231: slot 274
    enumerate 230: slot 273
....
    enumerate -1: slot 43
    enumerate -2: slot 42
    enumerate -3: slot 41
    enumerate -4: slot 40

etc.  This array even has holes (slots 2, 3, and 5-10 contain undefined).

Comment 3

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Slow arrays would explain the perf issue, for sure.  I guess we need the hybrid-storage arrays or something similar.

Comment 4

[Brendan Eich [:brendan]]

Yikes, negative array indexing.

If the access sites are using o[i] and not o[-1], there's no likely PIC story in general (perhaps this benchmark uses only a small set of negative indexes).

Here is a dirty idea: optimize negative indexes as if they are dense, off the end of our dense length limit (if we still have one).

/be

Comment 5

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

That idea makes me a little nervous in my fast-path places.  Growing the array into the region of negative wraparound would require 

Having arrays use a side object for named properties would probably be not extremely difficult.  If we never wanted to make arrays slow due to sparse indexed property use, it would simplify things too.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> If the access sites are using o[i] and not o[-1]

We're talking stuff like this:

  levels[y - (dataHeightMap[index] * HEIGHT >> 10) ] = y;

in hot loops.  Also nonnegative access on sparse arrays like so:

  index = indices[y];
  data[index + 1] = pixel - 64 + (y >> 2);
  data[index + 2] = pixel - 32 + (y >> 2);

> (perhaps this benchmark uses only a small set of negative indexes)

I assume "40" is not small in this context?  The array I was looking at had about that many.

We use slow arrays on sparse use to avoid unnecessary huge buffers, right?  I'm not sure we have a good way to deal with that without slow-ifying....

Comment 7

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

We use slow arrays on non-index use basically because I thought it was rare and it avoided needing more complex property lookup code.   That array doesn't look sparse to me, so I think it's mostly a case of us wanting to be able to have named properties (including those named "-1" and such) without losing the fast path for indexed ones.

Do V8 or JSC have fast paths for negative indexes?

Comment 8

[David Anderson [:dvander] - inactive, e-mail if emergency]

JSC takes a slow path if the index, as a 32-bit unsigned integer, compares >= to the array length.

Comment 9

[Boris Zbarsky [:bzbarsky]]

So... what happens with us is that if the _first_ prop you set on an array is a negative one, then we will compile the slow path in TM with no guard as far as I can see.  And then we will take that path for all indexed prop sets on that array.  Then again, I see no guard on the index in the index >= 0 case either.  Am I just missing it?

For the gets, we just lose because the arrays aren't dense so we go through the resolve thing no matter what, even for nonnegative indices.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

This is very smooth with JM+TI.