922406 - Top-level use of "this" prevents ion compilation

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Jesse Ruderman]

Removing the second line of this shell program makes it 9x faster:

var start = dateNow();
this.Array;
for (var i = 0; i < 10000000; ++i) {}
var end = dateNow();
print(Math.floor(end - start) + " ms");

Pretty much anything more complicated than "this;" causes the slowdown, e.g.
  * this.Array;
  * this, {};
  * (function(){})(this);  // often needed for asm modules or strict-mode code

I noticed this slowdown while converting Mersenne Twister to asm style.

Comment 1

[Jesse Ruderman]

Attachment: regression-range.txt

Comment 2

[Jesse Ruderman]

Regression range depends on the exact second line.  The attachment above is for "this, {};".  Its large regression range seems to be due to hgrange('d2cce982a7c8', '4a6b8dd4dfe3'), # broken virtualenv

Comment 3

[Jesse Ruderman]

> (function(){})(this)

The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/e0bc47d3f814
user:        Jan de Mooij
date:        Mon Dec 17 11:04:08 2012 +0100
summary:     Bug 821682 - Compile DEFVAR, DEFCONST and PICK. r=djvj

> this.Array;

The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/c13b9e58eedb
user:        Jan de Mooij
date:        Wed Dec 19 09:43:22 2012 +0100
summary:     Bug 822744 - Compile OBJECT, REGEXP, LAMBDA. r=djvj

> this, {}; 

Large regression range in comment 1.

Comment 4

[Christian Holler (:decoder)]

Not sure if this is still valid, but putting a needinfo on jandem to be sure.

Comment 5

[Guilherme Lima]

With 'this.Array;' I get 260ms on Nightly and 45ms on Chrome 43
Without 'this.Array;' I get 23ms on Nightly and 45ms on Chrome 43.

Comment 6

[Jan de Mooij [:jandem]]

(In reply to Christian Holler (:decoder) from comment #4)
> Not sure if this is still valid, but putting a needinfo on jandem to be sure.

It is, thanks for the reminder. Patch in a bit.

Comment 7

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #6)
> (In reply to Christian Holler (:decoder) from comment #4)
> > Not sure if this is still valid, but putting a needinfo on jandem to be sure.
> 
> It is, thanks for the reminder. Patch in a bit.

Actually, this bug is a fair amount of work to fix: we either (1) should add an extra MBasicBlock/snapshot slot for `this` in global scripts, or (2) fix bug 1125423, so that we have the global `this` for the compartment readily available.

(2) is a bit annoying because it requires changes outside SpiderMonkey, but I think it's where we want to be long-term so I'd prefer it over putting a lot of effort on (1). Unfortunately neither of these is trivial.

Comment 9

[Jan de Mooij [:jandem]]

Attachment: Patch

After all the other changes, fixing this is almost trivial.

Comment 10

[Shu-yu Guo [:shu]]

Comment on attachment 8691334 [details] [diff] [review]
Patch

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

A triumph.

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/33d2af1ba94e

Comment 12

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

https://hg.mozilla.org/mozilla-central/rev/33d2af1ba94e