600424 - JM: PICs must obey method read barriers

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

PICs call hasGetterOrIsMethod() to check for getters, but this elides a check for method-valued shapes. We must handle this case correctly. Test-case forthcoming.

Comment 1

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

Attachment: test case

Comment 2

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

I'd be willing to do a followup to make a guard type for that check, iff'n we are so inclined to think that's a good idea.

Comment 3

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

We know this case hits in v8-raytrace. I'll see how much we lose by just disabling the PIC. If it's too much...

AFAIK (Brendan, please check me on this), we *must* call the read barrier, which does three things:
 1) Clones the function object.
 2) Deletes the method-valued property, and adds the function object as a normal property. This calls into SetPropertyHelper which does a full property lookup. That calls into js_NativeGet -> methodWriteBarrier -> putProperty, which searches the shape ancestry.
 3) Generates a new unique shape for the object, meaning some code may become (more) poly/megamorphic.

By inlining a call to the read barrier, we can avoid at least the initial property lookup that would occur in stubs::GetProp. Past that, maybe there is more we could do, but it sounds tricky. The tracer doesn't bother (though it calls the read barrier).

Comment 4

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

Attachment: fix

This patch just disables the PIC. It's about a 5ms loss on v8-raytrace, unfortunately. I dampened this in the overall suite by avoiding the slot load on method-valued shapes. Now it's about a 15ms net win (noise on SunSpider).

Branding, and potentially calling the read barrier, can be follow-up bugs.

Comment 5

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

http://hg.mozilla.org/tracemonkey/rev/93e14d3a1eff

Comment 6

[Brendan Eich [:brendan]]

Yes, the read barrier is mandatory if you defer cloning (which the interpreter and tracer do, and cloning eagerly is strictly more expensive than setting a bit).

We talked about this the other week, not sure why we thought JM could go its own way on the method barrier.

Branding on method call to make receiver (this) shape depend on identity of the method is a different case, where JM is (still) going its own way, although both is-branded and has-method-barrier tests govern the method write barrier tripping. Maybe the two flags (BRANDED, METHOD_BARRIER) were muddled together too much when we last spoke.

We need to win back what was lost here. Please cc: me on followups.

/be

Comment 7

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/93e14d3a1eff