1212183 - Global lexicals regressed dromaeo DOM real bad

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Shu-yu Guo [:shu]]

I regressed Dromaeo DOM real bad in bug 589199 by 20% [1]. I'm suspecting it's some buggy code to do with optimizing global name accesses that happen to be DOM getters. Will investigate.

[1] http://graphs.mozilla.org/graph.html#tests=[[73,131,35]]&sel=1444082271000,1444255071000&displayrange=7&datatype=geo

Comment 1

[Shu-yu Guo [:shu]]

Attachment: Fix DOM getter optimizations in the JITs

Comment 2

[Shu-yu Guo [:shu]]

Comment on attachment 8670597 [details] [diff] [review]
Fix DOM getter optimizations in the JITs

Review of attachment 8670597 [details] [diff] [review]:
-----------------------------------------------------------------

So I don't know these DOM optimizations real well. Are there other places that need to be updated?

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8670597 [details] [diff] [review]
Fix DOM getter optimizations in the JITs

Argh.  This stuff is all so fragile...  :(

For UpdateExistingGetPropCallStubs is globalLexical really the right receiver?  Seems weird and at least worth documenting.

Past that, I think you got everything, assuming "CallNativeGlobal" is the only new IC stub that got added that's relevant for gets/sets on the global.

Comment 4

[Shu-yu Guo [:shu]]

(In reply to Boris Zbarsky [:bz] from comment #3)
> Comment on attachment 8670597 [details] [diff] [review]
> Fix DOM getter optimizations in the JITs
> 
> Argh.  This stuff is all so fragile...  :(
> 
> For UpdateExistingGetPropCallStubs is globalLexical really the right
> receiver?  Seems weird and at least worth documenting.
> 

Yeah, it's subtle and worth documenting. It's the receiver only for the sake of the stub generating a shape guard for it. The weirdness is because we need to guard 3 things:

1. The global lexical scope shape, since it can shadow. Done via receiver because at the point of GETGNAME, the global lexical scope is the scope chain and in the receiver register.
2. The holder shape. Done via holder per normal.
3. The global object shape. Done via a manually-generated shape check.

Comment 5

[Shu-yu Guo [:shu]]

(In reply to Shu-yu Guo [:shu] from comment #4)
> (In reply to Boris Zbarsky [:bz] from comment #3)
> > Comment on attachment 8670597 [details] [diff] [review]
> > Fix DOM getter optimizations in the JITs
> > 
> > Argh.  This stuff is all so fragile...  :(
> > 
> > For UpdateExistingGetPropCallStubs is globalLexical really the right
> > receiver?  Seems weird and at least worth documenting.
> > 
> 
> Yeah, it's subtle and worth documenting. It's the receiver only for the sake
> of the stub generating a shape guard for it. The weirdness is because we
> need to guard 3 things:
> 
> 1. The global lexical scope shape, since it can shadow. Done via receiver
> because at the point of GETGNAME, the global lexical scope is the scope
> chain and in the receiver register.
> 2. The holder shape. Done via holder per normal.
> 3. The global object shape. Done via a manually-generated shape check.

Worth to note also that in the case of the holder == global object, we generate a superfluous check. I haven't bothered to optimize this away yet.

Comment 6

[Jan de Mooij [:jandem]]

Comment on attachment 8670597 [details] [diff] [review]
Fix DOM getter optimizations in the JITs

Review of attachment 8670597 [details] [diff] [review]:
-----------------------------------------------------------------

I'll be refactoring IC code the coming weeks, let me know if you (or bz) have ideas to make this better.

Comment 7

[Guilherme Lima]

On bug 589199 I forgot to say that the patch also regressed sunspider-partial-sums by 46%. Same problem as on Dromaeo?

Comment 8

[Shu-yu Guo [:shu]]

(In reply to Guilherme Lima from comment #7)
> On bug 589199 I forgot to say that the patch also regressed
> sunspider-partial-sums by 46%. Same problem as on Dromaeo?

No, not the same as this one. That one might be unavoidable for now.

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c109ff711c09

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/5bc4d467648c for mass bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=15347554&repo=mozilla-inbound

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/41c83c26dc79

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/41c83c26dc79

Comment 13

[Joel Maher ( :jmaher ) (UTC -8)]

should this be reopened?  it was backed out and then closed.  The perf regression from talos was fixed temporarily and now it is back to the regressed state.

Comment 14

[Boris Zbarsky [:bzbarsky]]

The backout was comment 10, of the landing in comment 9.  Then comment 11 does a relanding with the failing assert adjusted, which was merged around (comment 12).

That said, the relanding seems to be missing the nice "The CallNativeGlobal stub needs to generate 3 shape checks" comment for some reason?

Comment 15

[Shu-yu Guo [:shu]]

(In reply to Boris Zbarsky [:bz] from comment #14)
> The backout was comment 10, of the landing in comment 9.  Then comment 11
> does a relanding with the failing assert adjusted, which was merged around
> (comment 12).
> 
> That said, the relanding seems to be missing the nice "The CallNativeGlobal
> stub needs to generate 3 shape checks" comment for some reason?

Ugh, good catch. I'll fix this in a bit.

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b8b729c0dfb2

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b8b729c0dfb2