451154 - TM: traces abort on Math.* method access

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

I ran into this in code that had the following function:

      function abs(a) {
        return Math.sqrt(a.r * a.r + a.i * a.i);
      }

tracemonkey bailed out of tracing through this with the following message:

  abort: 2628: non-stub getter

Looking at jstracer.cpp:2628, it looks like it's testing !SPROP_HAS_STUB_GETTER(sprop), which just tests for sprop->getter being null.

Unfortunately, in this case, the getter is the "sqrt" getter on Math, and the property is defined by calling js_DefineFunction with the JSFUN_STUB_GSOPS flag set (got there by defining the whole thing via JS_FN macro, then calling JS_DefineFunctions on the JSFunctionSpec array).

So the point is, the getter and setter are not null but rather JS_PropertyStub.  It seems like we should be able to trace this particular case, since the stubs do nothing, no?

For what it's worth, changing the code above to read:

      var mySqrt = Math.sqrt;
      function abs(a) {
        return mySqrt(a.r * a.r + a.i * a.i);
      }

(so that the Math.sqrt get only happens once instead of a bunch of times) speeds up my testcase by a factor of 2.5 or so.  Presumably we trace through a lot more of it!

Comment 1

[Brendan Eich [:brendan]]

Great find! Thanks, bz. Taking,

/be

Comment 2

[Brendan Eich [:brendan]]

Hrm, wait. js_DefineNativeProperty will turn null getter or setter into clasp->getProperty or clasp->setProperty, which is why this JSFUN_STUB_GSOPS flag is needed -- but js_AddScopeProperty will turn JS_PropertyStub into null at its layer, to optimize tests.

So we shouldn't see any Math.sqrt or other methods with JS_PropertyStub in the sprop->getter or sprop->setter fields. I just stepped through this and don't see any such. Bz, what is the testcase you used?

/be

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: I thought this would help, but no...

Even with this patch, we still bail out on this line a lot.  When we do, the object we're doing the get on is the Window object, and the getter in sprop is XPC_WN_Helper_GetProperty.  Not sure why we end up hitting that for window.Math, which is what we presumably hit it for in this case.

That does make it a lot harder to jit through this, since Math could be some iframe or something.  :(

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: The testcase

Comment 5

[Brendan Eich [:brendan]]

If !SPROP_HAS_STUB_[GS]ETTER(sprop) you can assert sprop->[gs]etter != JS_PropertyStub, hence my confusion in comment 2.

Ok, so the evil XPConnect wrapped native code is costing us. Paging mrbkap!

/be

Comment 6

[Boris Zbarsky [:bzbarsky]]

So I did a little more digging through the log here.  It looks like the most common trace aborts here are, in order:

  Math.abs: non-stub getter for the Math
  Math.floor: non-stub getter for the Math
  for (var i = 1; i < threshold; ++i) { : "Loop edge does not return to header."

Comment 7

[Boris Zbarsky [:bzbarsky]]

What particularly confuses me is that my change mentioned in comment 0 to use mySqrt is sticking it on the global object too, so I would have thought it would behave no differently from Math.  Yet it does.

Comment 8

[Andreas Gal :gal]

for (var i = 1; i < threshold; ++i) { : "Loop edge does not return to
header."

Does it say something about inner tree not suitable for calling right above the "Loop edge does not return to header" message?

Comment 9

[Boris Zbarsky [:bzbarsky]]

Nope.  It says:

loop edge 150 -> 58, header -216757102
Abort recording (line 31, pc 150): Loop edge does not return to header.

Comment 10

[Brendan Eich [:brendan]]

bz: can you hop on IRC?

The reason var mySqrt = Math.sqrt works is that var, if it can create the named property instead of just restate it, will make it JSPROP_PERMANENT. This means we can optimize its accesses using JSOP_GETGVAR to be slot indexes instead of name lookups. Such accesses bypass the class getter or setter.

So too should Math's accesses. I may fix this in the JS engine if mrbkap does not have a better XPConnect idea.

/be

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: Here's what looks like a complete log of the header thing

Comment 12

[Andreas Gal :gal]

Is this a fairly recent version of the tree you are using? I would have expected an attempted (inner) tree call there.

Comment 13

[Boris Zbarsky [:bzbarsky]]

All of the above was with changeset 793e5bc71728, which is dated 2008-Aug-18.  Summary is "Add a place to store the current shape of the global object as we add slots it the global slot list".

So reasonably recent.  I'll pull right now and see whether that changes anything.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Ok, with today's pull I no longer get that loop edge thing.  The leading trace abort causes are:

  347 Abort recording (line 22, pc 0): JSOP_NAME.
   20 Abort recording (line 81, pc 354): Loop edge does not return to header.

The former is 336 times the non-stub getter thing, and 11 times "abort: 2602: no cacheable property found".

The latter is "loop edge 354 -> 142, header 125".  This is the inner loop over rows (incrementing j from 0 to numRows).

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: Stub the getter and setter on class prototypes and constructors

Comment 16

[Brendan Eich [:brendan]]

Comment on attachment 334744 [details] [diff] [review]
Stub the getter and setter on class prototypes and constructors

Great job -- dev-doc-needed, please, and Andreas is landing on TM.

/be

Comment 17

[Boris Zbarsky [:bzbarsky]]

I assume the dev-doc-needed is for the fact that JS_InitClass now sets up the prototype or constructor with stub getter and setter ops instead of class ops, right?

Comment 18

[Andreas Gal :gal]

Landed. Great fix. Thanks a lot!

http://hg.mozilla.org/tracemonkey/index.cgi/rev/a5ecf48dd03b

Comment 19

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #17)
> I assume the dev-doc-needed is for the fact that JS_InitClass now sets up the
> prototype or constructor with stub getter and setter ops instead of class ops,
> right?

Yeah. That's actually a pretty big change. Mozilla has been implicitly relying on the old behavior for several years for security checks (which are now taken care of by XOWs).

Comment 20

[Eric Shepherd [:sheppy]]

What version of SpiderMonkey does this change correspond to?

Comment 21

[Eric Shepherd [:sheppy]]

Assuming this applies to SpiderMonkey 1.8, and documented as such.

Comment 22

[Bob Clary [:bc] (inactive)]

does js1_8_1/trace/math-trace-tests.js provide sufficient coverage for this?