594230 - Seem to be missing a callProp PIC on this testcase

Status: NEW

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Attachment: The testcase

The testcase spends 14.5% of its time under stubs::CallProp, a lot of this doing things like js_GetMethod, property cache tests, calling array_getProperty, doing js_LookupPropertyWithFlags, etc.

Comment 1

[David Mandelin [:dmandelin]]

Attachment: Beautified test case

Here are the top lines of code where we call the NameOp slow path:

     48 NameOp file:///C:/sources/scratch/d.html:59
  10944 NameOp file:///C:/sources/scratch/d.html:61
  10944 NameOp file:///C:/sources/scratch/d.html:62
  16526 NameOp file:///C:/sources/scratch/d.html:60
  43912 NameOp file:///C:/sources/scratch/d.html:65
  43912 NameOp file:///C:/sources/scratch/d.html:66
  98656 NameOp file:///C:/sources/scratch/d.html:69
 109504 NameOp file:///C:/sources/scratch/d.html:71
 262809 NameOp file:///C:/sources/scratch/d.html:75
 262812 NameOp file:///C:/sources/scratch/d.html:73
 279468 NameOp file:///C:/sources/scratch/d.html:67
 438016 NameOp file:///C:/sources/scratch/d.html:72
 700827 NameOp file:///C:/sources/scratch/d.html:74

Comment 2

[David Mandelin [:dmandelin]]

Better data (gives the property id as well):

     16 NameOp G file:///C:/sources/scratch/d.html:59
     16 NameOp h2 file:///C:/sources/scratch/d.html:59
     16 NameOp y file:///C:/sources/scratch/d.html:59
   4999 NameOp h2 file:///C:/sources/scratch/d.html:62
   4999 NameOp w2 file:///C:/sources/scratch/d.html:61
   4999 NameOp x file:///C:/sources/scratch/d.html:61
   4999 NameOp y file:///C:/sources/scratch/d.html:62
   5002 NameOp G file:///C:/sources/scratch/d.html:60
   5031 NameOp fin file:///C:/sources/scratch/d.html:69
   5041 NameOp x file:///C:/sources/scratch/d.html:60
   5056 NameOp w2 file:///C:/sources/scratch/d.html:60
  19559 NameOp M file:///C:/sources/scratch/d.html:67
  19560 NameOp co file:///C:/sources/scratch/d.html:67
  19560 NameOp z file:///C:/sources/scratch/d.html:67
  19994 NameOp x file:///C:/sources/scratch/d.html:69
  19994 NameOp y file:///C:/sources/scratch/d.html:69
  19998 NameOp co file:///C:/sources/scratch/d.html:65
  19998 NameOp d file:///C:/sources/scratch/d.html:66
  20122 NameOp a file:///C:/sources/scratch/d.html:69
  20126 NameOp fi file:///C:/sources/scratch/d.html:66
  20126 NameOp no file:///C:/sources/scratch/d.html:65
  24991 NameOp G file:///C:/sources/scratch/d.html:69
  25010 NameOp G file:///C:/sources/scratch/d.html:71
  30008 NameOp yp file:///C:/sources/scratch/d.html:71
  39120 NameOp W file:///C:/sources/scratch/d.html:67
  39120 NameOp d file:///C:/sources/scratch/d.html:67
  39248 NameOp pi file:///C:/sources/scratch/d.html:67
  45018 NameOp yd file:///C:/sources/scratch/d.html:71
  78677 NameOp i file:///C:/sources/scratch/d.html:67
  80028 NameOp A file:///C:/sources/scratch/d.html:74
  80028 NameOp D file:///C:/sources/scratch/d.html:75
  80028 NameOp I file:///C:/sources/scratch/d.html:75
  80028 NameOp P file:///C:/sources/scratch/d.html:75
  80028 NameOp i file:///C:/sources/scratch/d.html:74
  80028 NameOp w file:///C:/sources/scratch/d.html:73
  80028 NameOp xd file:///C:/sources/scratch/d.html:73
  80028 NameOp yd file:///C:/sources/scratch/d.html:73
 100036 NameOp G file:///C:/sources/scratch/d.html:72
 120040 NameOp xp file:///C:/sources/scratch/d.html:72
 180064 NameOp xd file:///C:/sources/scratch/d.html:72
 240084 NameOp D file:///C:/sources/scratch/d.html:74
 240084 NameOp I file:///C:/sources/scratch/d.html:74

Comment 3

[David Mandelin [:dmandelin]]

I think there are two main things going wrong here:

1. Not PIC'ing typed arrays

2. Not optimizing to GNAME ops in code like this:

function cm(f) {
    for (var i = 0; i < 10; ++i) {
        f();
    }
    dis(f);
}

cm(function() {
    for (x = 0; x < 10; ++x) {
        function a(n) {
            for (y = 0; y < 10; ++y) {
            }
        }

        a(x);
    }
});

The problem is that "function a", in our extension, binds only if control reaches that point (see jsparse.cpp:3331 "ECMA ed. 3 extension"). This means that statement can create variables at run time. Currently, we deoptimize all other free variables to NAME ops in that case, on the assumption that the newly created variables might shadow any global binding.

Possible fixes:

 a. Change the behavior of function statements inside compound statements. I
    know it's a compat issue but I can't remember whether it's an insurmountable
    one.

 b. In the parser, track the set of names that might be bound later, and
    avoid optimizing to GNAME only if there is a potential collision.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Hmm.  I thought we PICed typed arrays now.  The rest of this sounds plausible.

Comment 5

[David Mandelin [:dmandelin]]

(In reply to comment #4)
> Hmm.  I thought we PICed typed arrays now.  The rest of this sounds plausible.

I may have been just misinterpreting spew.

Comment 6

[Brendan Eich [:brendan]]

(In reply to comment #3)
>  a. Change the behavior of function statements inside compound statements. I
>     know it's a compat issue but I can't remember whether it's an
> insurmountable
>     one.

We're not changing to match IE and its followers, rather to block-scoped function declaration in block bindings. See bug 585536.

Option b is good.

/be