557357 - JM: PIC for method-valued properties and getprop

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, defect)

Description

[David Mandelin [:dmandelin]]

Method-valued properties are different, because if the object is branded, having the same shape means even the property object identity is the same. Also, a read barrier needs to be called when reading this out.

Currently, we aren't optimizing them at all, because they look like they have getters, and because of the read barrier, which is not implemented in the PICs currently. This does hurt us on v8-raytrace. Near the top of the file, they have:

  this.initialize.apply(...)

|this.initialize| is method-valued, so we are not getting it with the PIC.

Note also that the read barrier is not truly needed here. We probably also want to optimize that away.

Comment 1

[Jan de Mooij [:jandem]]

Blocking JaegerSpeed. This is not fixed yet, right?

JM+TM starts to perform well on most of the V8 benchmarks, except v8-raytrace. The patch for bug 561506 improves it a lot, but when it was applied it was still more than 3 times slower than JSC on awfy (JM: 436 ms, JM+TM: 490 ms). Could this bug be the next showstopper?

Comment 2

[Brendan Eich [:brendan]]

(In reply to comment #0)
> Method-valued properties are different, because if the object is branded,
> having the same shape means even the property object identity is the same.

Right.

> Also, a read barrier needs to be called when reading this out.

Only if METHOD_BARRIER is set -- BRANDED by itself does not require a barrier. These are JSObject::flags now of course. Small point but wanted to clarify it.

>   this.initialize.apply(...)
> 
> |this.initialize| is method-valued, so we are not getting it with the PIC.
> 
> Note also that the read barrier is not truly needed here. We probably also want
> to optimize that away.

Yes, getting the method just to apply it (use it as |this| to call Function.prototype.apply) does not need to clone the function object (go through the barrier). Based on apply being the right built-in, we should be able to avoid the method barrier.

/be

Comment 3

[Brendan Eich [:brendan]]

Yes, this could be important.

/be

Comment 4

[David Mandelin [:dmandelin]]

It looks like we successfully IC that line now--dvander might have fixed that up while porting over to the register allocator. I did notice that we stop growing that PIC because we reach the stub limit (there are at least 15 different x.initialize in there), but increasing the stub limit didn't help perf.

I did notice that increasing the stub limit seemed to hurt us on various SunSpider benchmarks, so it seems that is a tuning knob we should play with at some point.

Comment 5

[Brendan Eich [:brendan]]

JM doesn't brand objects, so how does it (or TM and the old interpreter, in full) handle method change?

/be

Comment 6

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

For SETPROP, non-adding case, we guard that the method won't change. For GETPROP/CALLPROP we load out of the slot.

Comment 7

[Brendan Eich [:brendan]]

Is this really coherent? If JM guards for its own uses, that's good as far as it goes. But if it calls TM without having set a BRANDED flag, won't that lead to TM (or the old interpreter) failing to notice method change and reshape?

Do you implement the method read barrier in JM?

/be

Comment 8

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

Turns out it wasn't coherent, that's fixed, and we bake in method values now - see bug 600424. We still don't bake in a call to the read barrier, or brand though, so keeping this bug open.

Comment 9

[Brendan Eich [:brendan]]

Don't forget the write barrier.

/be

Comment 10

[Luke Wagner [:luke]]

JM removed.