1492920 - Enhance InstanceOf IC support

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement, P2)

Description

[Matthew Gaudet (he/him) [:mgaudet]]

On Google Docs, I'm seeing lots and lots of misses trying to attach ICs for InstanceOf; spending about a minute on the test case document in Bug 1488435 I see 65,000 InstanceOf IC attachment attempts that fail. for example: 

> Type	InstanceOf 	
> Mode	Specialized
> Location	https://docs.google.com/static/document/client/js/210745210-kix_main_i18n_integrated_kix_core.js:968
> Column	110
> PC	13b230366
> LHS	Object 111289cc0 (shape: 0)
> RHS	Function 139b11840 (shape: 13a006b78)


The too many attachment attempts is Bug 1448039, however it is likely that we could do better. It may be worth investigating if a plain guardObject + VM call to HasInstance is appreciably faster than the DoInstanceOf fallback path with the attachment attempts bug fixed. 

(Interesting to note the LHS has no shape)

Comment 1

[Matthew Gaudet (he/him) [:mgaudet]]

(At evilpie's suggestion on IRC, I did another run where I patched DefaultJitOptions to set SET_DEFAULT(disableUnboxedObjects, true); it appears this makes the shapeless objects go away, but doesn't reduce InstanceOf failures )

Comment 2

[Ted Campbell [:tcampbell]]

I believe when we studied this last time it was checking instanceof of a prototype further up the chain.

Comment 3

[Nicolas B. Pierron [:nbp]]

Matthew, should this issue block Bug 1488435? Do we know how significant it is to fail 65k times? Are these failures sharing a single IC list or are these across hundreds of code locations?

Comment 4

[Matthew Gaudet (he/him) [:mgaudet]]

I have no intuition about how much time we'd save by having an IC here, but it feels like 65,000 in a minute of anything is a lot. 

The actual code that tells us we miss this many times is gone now that Bug 1448039 has landed (it'll silently transition into generic). 

I'll mark this as blocking Bug 1488435, though this will be a little more challenging to solve.

Comment 5

[Chris Fallin [:cfallin]]

It seems that this is occurring because the function on the RHS of the instanceof has a non-Function prototype. From my logging:

Trying to attach instanceof IC stub: script https://docs.google.com/static/document/client/js/4246577276-client_js_prod_kix_core.js:1128:1005                                                                                                 
 -> RHS function's prototype is not an object or is not Function                                                                                                                                                                              
Trying to attach instanceof IC stub: script https://docs.google.com/static/document/client/js/4246577276-client_js_prod_kix_core.js:850:749                                                                                                   
 -> RHS function's prototype is not an object or is not Function                                                                                                                                                                              
Trying to attach instanceof IC stub: script https://docs.google.com/static/document/client/js/4246577276-client_js_prod_kix_core.js:850:916                                                                                                   
 -> RHS function's prototype is not an object or is not Function                                                                                                                                                                              
Trying to attach instanceof IC stub: script https://docs.google.com/static/document/client/js/4246577276-client_js_prod_kix_core.js:850:749                                                                                                   
 -> RHS function's prototype is not an object or is not Function                                                                                                                                                                              
Trying to attach instanceof IC stub: script https://docs.google.com/static/document/client/js/4246577276-client_js_prod_kix_core.js:850:916                                                                                                   
 -> RHS function's prototype is not an object or is not Function                                                                                                                                                                              
Trying to attach instanceof IC stub: script https://docs.google.com/static/document/client/js/4246577276-client_js_prod_kix_core.js:850:916                                                                                                   
 -> RHS function's prototype is not an object or is not Function                                                                               
....

i.e., we're hitting the case at https://searchfox.org/mozilla-central/source/js/src/jit/CacheIR.cpp#4435 .

Pulling down that .js and staring at it a bit, there's an interesting pattern like:

function f(x) { ... x instanceof C ... }    // this is the IC with the attachment failures
function C() { ... }
C.prototype.property = ...

Which I think means that (given funProto is the prototype object for C) funProto->staticPrototype() will have changed from that of Function, if my understanding of "static prototypes" is correct?

Comment 6

[Chris Fallin [:cfallin]]

Actually, I slightly misunderstood things (and, being very rusty in JS, had forgotten the difference between __proto__ and prototype). It turns out that the important bit in the JS above is actually:

C.__proto__ = D;

i.e., the prototype chain of the constructor is modified. (It's a bit tricky to see in the obfuscated JS; for the file above, line 850 column 916, the RHS of instanceof is ckb, which is passed to A as first arg; function A is defined on line 68 col 1 and calls Oxa with ckb as first arg; Oxa is assigned from Kxa (line 67), and Kxa(a,b) does a.__proto__=b on line 67 col 403.

So tl;dr: we need to handle a instanceof b where b.__proto__ != Function.__proto__ to allow the IC stub to attach.

Comment 7

[Chris Fallin [:cfallin]]

Attachment: Bug 1492920: remove restriction on IC attachment for instanceof: allow RHS with a reassigned __proto__. r=djvj,jandem

Based on discussions with :djvj, it seems that this IC attachment logic is
overly conservative. We're seeing a case where the __proto__ of a constructor
function is reassigned, which causes all instanceof ICs to fail to attach. The
test case is like:

function C() { /* ... */ }
C.__proto__ = D;
var o = new C();
var result = o instanceof C;  // this IC fails to attach

It seems that all that is really necessary is that the RHS of the instanceof is
a callable (required by the spec), and that its .prototype property is
present directly on the object (as opposed to somewhere up the prototype
chain). The IC guards on the shape of the RHS, so the baked-in load of the slot
for .prototype will always be correct. So it seems that deleting this check
for C.__proto__ == Function.__proto__ is safe.

Comment 8

[Chris Fallin [:cfallin]]

Some stats collected from this change: using the document at link and with a simple printf addition to the attach code, the baseline (prior to this change) has 102 successful attachments of InstanceOf IC stubs to scripts from docs.google.com, while with this change, it has 397 successful attachments. (Will attach the simple printf diff and the output log for reproducibility.)

Comment 9

[Chris Fallin [:cfallin]]

Attachment: stats-printf.patch

Comment 10

[Chris Fallin [:cfallin]]

Attachment: stats-output.txt

Comment 11

[Chris Fallin [:cfallin]]

Patch accepted and try runs (link) are green; I think this should be ready to go...

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b4511cf734e
remove restriction on IC attachment for instanceof: allow RHS with a reassigned proto. r=djvj,jandem

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7e12c8c5d1a3
remove restriction on IC attachment for instanceof: allow RHS with a reassigned __proto__. r=djvj,jandem

Comment 14

[Kannan Vijayan [:djvj]]

(In reply to Chris Fallin [:cfallin] from comment #8)

Some stats collected from this change: using the document at link and with a simple printf addition to the attach code, the baseline (prior to this change) has 102 successful attachments of InstanceOf IC stubs to scripts from docs.google.com, while with this change, it has 397 successful attachments. (Will attach the simple printf diff and the output log for reproducibility.)

It would be interesting to look at the number of times total times the optimized stub is hit before and after this change. Attaches indicate the number of places where we're successfully optimization, but some additional insight into the number of times each of those sites hits would be worthwhile.

Comment 15

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2992e31e7b7e
Backed out changeset 4b4511cf734e for landing on both autoland and inbound.

Comment 16

[Chris Fallin [:cfallin]]

(In reply to Kannan Vijayan [:djvj] from comment #14)

It would be interesting to look at the number of times total times the optimized stub is hit before and after this change. Attaches indicate the number of places where we're successfully optimization, but some additional insight into the number of times each of those sites hits would be worthwhile.

Yes, agreed, dynamic counts would be interesting. As far as I understand it, the cache IR logs only record attachment events, not execution of ICs, since the execution is on the fastpath (although I could be wrong, as I found the cacheIR spew output to be lots of verbose JSON and thus measured with simple printfs instead); but I suppose there's a way to get this from the warmup counters? I'll dig a bit more...

Comment 17

[Jan de Mooij [:jandem]]

For printf-style you can use writer.callPrintString("Foo") before the return-from-IC CacheIR op to track success cases.

Comment 18

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/7e12c8c5d1a3

Comment 19

[Iain Ireland [:iain]]

(In reply to Chris Fallin [:cfallin] from comment #16)

(although I could be wrong, as I found the cacheIR spew output to be lots of verbose JSON and thus measured with simple printfs instead)

Have you seen the CacheIR analyzer?

Comment 20

[Chris Fallin [:cfallin]]

(In reply to Iain Ireland [:iain] from comment #19)

(In reply to Chris Fallin [:cfallin] from comment #16)

(although I could be wrong, as I found the cacheIR spew output to be lots of verbose JSON and thus measured with simple printfs instead)

Have you seen the CacheIR analyzer?

Wow, that's great, thanks (and thanks Jan for the tip as well)! I'll follow up with dynamic stats soon.

Comment 21

[Chris Fallin [:cfallin]]

OK, here are some stats for closure: on the same Google Docs test (but interacting by hand so non-comparable with earlier stats), I see 101235 successful IC invocations (not bailed out) with this bug's change, of which 93055 would not have attached prior to this change. Will attach patch used to measure and output log.

Comment 22

[Chris Fallin [:cfallin]]

Attachment: measure-instanceof-dynamic-instances.patch

Comment 23

[Chris Fallin [:cfallin]]

Attachment: measure-instanceof-dynamic-instances.txt.gz