Closed Bug 451154 Opened 11 years ago Closed 11 years ago

TM: traces abort on Math.* method access

Categories

(Core :: JavaScript Engine, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla1.9.1a2

People

(Reporter: bzbarsky, Assigned: brendan)

References

Details

(Keywords: dev-doc-complete, testcase, Whiteboard: [dev doc: see comment 17])

Attachments

(4 files)

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!
Great find! Thanks, bz. Taking,

/be
Assignee: general → brendan
OS: Mac OS X → All
Priority: -- → P1
Hardware: PC → All
Summary: tracemonkey traces abort on Math.* access → TM: traces abort on Math.* method access
Target Milestone: --- → mozilla1.9.1a2
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
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.  :(
Attached file The testcase
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
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."

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.
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?
Nope.  It says:

loop edge 150 -> 58, header -216757102
Abort recording (line 31, pc 150): Loop edge does not return to header.
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
Is this a fairly recent version of the tree you are using? I would have expected an attempted (inner) tree call there.
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.
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 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
Attachment #334744 - Flags: review?(brendan) → review+
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?
Keywords: dev-doc-needed
Whiteboard: [dev doc: see comment 17]
Landed. Great fix. Thanks a lot!

http://hg.mozilla.org/tracemonkey/index.cgi/rev/a5ecf48dd03b
Keywords: dev-doc-needed
Whiteboard: [dev doc: see comment 17]
Blocks: landtm
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
(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).
Keywords: dev-doc-needed
Whiteboard: [dev doc: see comment 17]
Keywords: testcase
Depends on: 460024
What version of SpiderMonkey does this change correspond to?
Assuming this applies to SpiderMonkey 1.8, and documented as such.
does js1_8_1/trace/math-trace-tests.js provide sufficient coverage for this?
You need to log in before you can comment on or make changes to this bug.