480192 - TM: Call getters and setters on trace

Status: RESOLVED INVALID

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

We already have the fast path down, and now we have a pretty nice slow path that's used by JSOP_GETELEM and JSOP_CALLELEM.  This bug aims to make JSOP_GETPROP/CALLPROP (and others) use that slow path too.  (Currently we abort recording instead!)

At the same time there's apparently some cruft in jstracer from a darker age when we were unsoundly calling resolve hooks on trace; we can rip that out now.

Comment 1

[Jason Orendorff [:jorendorff]]

Attachment: WIP 1

This seems to work. Sorry this took so long -- it was easy once I figured out what to do. :)

Tomorrow I'll make it work for JSOP_GET{THIS,ARG,LOCAL}PROP. Brendan, I plan to do this by making four copies of the imacro and tweaking the start of each one, unless you can think of a better way. (Maybe support for goto <imacroname> in the imacro compiler? Is it worth it?)  Brendan mentioned JSOP_CALLPROP too, but I don't know if there's a common case where that happens (function-valued property with a getter).

Then on to setters.

This WIP traces JSPropertyOp getters.  Supporting script getters and setters will be a medium-to-easy follow-up bug.

Comment 2

[Jason Orendorff [:jorendorff]]

Blah.  In addition to JSOP_IMACOPATOM, fixing GETARGPROP will require JSOP_IMACOPGETARG, and GETLOCALPROP will require JSOP_IMACOPGETLOCAL.

Comment 3

[Jason Orendorff [:jorendorff]]

Attachment: WIP 2

Running tests now.  I think this is ready for review whenever bug 463153 is fixed.

This traces getters and setters triggered by a few opcodes:
  GETPROP
  GET{THIS,LOCAL,ARG}PROP
  SETPROP

but by no means all.  These opcodes still abort on a getter or setter:
  CALLNAME, CALLPROP
  NAME, GETXPROP
  BINDNAME, SETNAME
  FORNAME
  {INC,DEC}*, *{INC,DEC}

I would like to stop here and get this patch in though, which means working on prerequisites.  Follow-up bug for the rest.

Comment 4

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: WIP 2 (updated to trunk)

I updated this to trunk.

Comment 5

[Jason Orendorff [:jorendorff]]

Well?  Does it work?

I think that patch is ready for review.  Go bent go.

Comment 6

[Andreas Gal :gal]

I am really not an expert in this part of town, but it seems we can solve this more generically by stringifying/intifying sprop->id, and burning that into trace, and then calling GetProperty or GetElement. That should work for all consumers then, not just GETPROP/GET{THIS,LOCAL,ARG}PROP/SETPROP. The usual shape guards apply and make sure the burned in id is still valid.

Comment 7

[Jason Orendorff [:jorendorff]]

I thought the burned-in id would always be valid because the atom for any of these instructions is always a string.  ...That's true, right?

I did it with imacros to avoid hand-emitting a call to GetProperty_tn, which is a _FAIL builtin.  The problem with that is resuming in a good state if you bail off trace there.  I think the existing (ugly and painfully tricky) machinery for recovering after a JSOP_CALL to a builtin would also work for JSOP_GET{,THIS,LOCAL,ARG}PROP, but not for SETPROP, CALL{NAME,PROP}, or INC/DEC opcodes.  The recovery code would be be a bit different for each of those, so it seemed to me like you would end up with a lot of C++ special cases (including the painfully tricky deep-bail code in LeaveTree) instead of a lot of imacros.

Also, INC and DEC opcodes do both a get and a set.  The only way to call both GetProperty_tn and SetProperty_tn and have each one bail out in the right state is to use an imacro.

Comment 8

[Andreas Gal :gal]

Ok, going with jason's approach. I was trying to extend the builtinStatus fiddling to more bytecodes but it seems pretty painful.

Comment 9

[Andreas Gal :gal]

Attachment: refreshed on top of my fast native patch + TM tip

Comment 10

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 368489 [details] [diff] [review]
refreshed on top of my fast native patch + TM tip

This patch causes

Assertion failure: !(cx->fp->flags & JSFRAME_IMACRO_START), at /Users/bent/src/mozilla/domthreads/js/src/jstracer.cpp:4441


when running mochitests, right off the bat.

Comment 12

[Andreas Gal :gal]

Attachment: patch

New patch to trace setters. browser starts up.

Comment 13

[Jason Orendorff [:jorendorff]]

Attachment: WIP - trace some setters

This is just a refresh of Andreas's patch, with a few bug fixes.

This will trace setters that have quick stubs.  It will not trace other XPConnect setters, as those are actual Functions (JSPROP_SETTER).

Comment 14

[Jason Orendorff [:jorendorff]]

bz notes that comment 13 is wrong: e.g. attachment 383408 [details] still does not trace, because quick stub properties are SHARED.  We get:

abort:8414: can't trace inherited property set

Looking into it.

Comment 15

[Jason Orendorff [:jorendorff]]

I promised an update today.  Sorry for the late hour and paucity of news; there was a disturbance in the Force (bug 500528)...

Bug 497886 ("Document the property cache") is in decent shape.

I think I can patch SHARED setters tomorrow (P=0.75). That particular case is not complicated. Whether that actually makes quick-stubbed XPConnect setters trace, we'll see. If so, I want to land it.

It would be great to land one patch covering all cases, but I now realize there are rather a lot of cases, and it seems better to knock them out one by one.  Or, the old approach, from patch WIP2, might work--put a slow "catchall" in place.

Comment 16

[Jason Orendorff [:jorendorff]]

Today I documented JS_SetProperty (so as to have some clue what it is I'm trying to fake):

https://developer.mozilla.org/en/SpiderMonkey/JSAPI_Reference/JS_SetProperty

While doing this I discovered some bugs and "maybe-bugs" in the current WIP patch. SHARED setters still will not be terribly complicated, but I'm now shooting for Monday and estimating P=0.70.

Comment 17

[David Mandelin [:dmandelin]]

Just as encouragement, Jeremy Hiatt has written some Real JavaScript Code (tm) that aborts on a getter. It's a pretty simple case, so I'm sure landing a simple version that doesn't necessarily cover everything would be appreciated.

Comment 18

[Jason Orendorff [:jorendorff]]

I have no doubt that getters and setters are hit in real-world code! The delay has not been due to motivation, but rather a steady stream of interesting discoveries regarding the property cache...

This setters patch is moving to bug 503408, and I'll post an update there in a minute.

Comment 19

[Stuart Parmenter]

This would help a bunch of our frontend code

Comment 20

[Mike Shaver (:shaver emeritus)]

Stuart: do you need scripted or native getters and setters? Or both?

Comment 21

[Andreas Gal :gal]

Scripted is actually pretty easy and a separate problem. I suggest keeping this bug focused on native.

Comment 22

[Brendan Eich [:brendan]]

(In reply to comment #21)
> Scripted is actually pretty easy and a separate problem. I suggest keeping this
> bug focused on native.

This bug doesn't say "native" in its Summary. It does block bug 503408 though, which is "Trace native setters". Hmm.

/be

Comment 23

[Joe Drew (not getting mail)]

The Processing.js project is being hurt badly by what appears to be a lack of tracing scripted getters and setters. (The abort traces say "dynamic property of Call object.")

Comment 24

[Brendan Eich [:brendan]]

(In reply to comment #23)
> The Processing.js project is being hurt badly by what appears to be a lack of
> tracing scripted getters and setters. (The abort traces say "dynamic property
> of Call object.")

That's not this bug. Please file with TMFLAGS=aborts output for a reduced testcase if possible.

/be

Comment 25

[Andreas Gal :gal]

Luke would be the ideal person to fix this. Or jorendorff. Imacros can do this easily.

Comment 26

[David Humphrey (:humph)]

> That's not this bug. Please file with TMFLAGS=aborts output for a reduced
> testcase if possible.

Filed bug 558830, also found and filed bug 558827 in the process.

Comment 27

[Brendan Eich [:brendan]]

(In reply to comment #26)
> > That's not this bug. Please file with TMFLAGS=aborts output for a reduced
> > testcase if possible.
> 
> Filed bug 558830, also found and filed bug 558827 in the process.

Bug 558830 looks like half of this bug (scripted getters called on trace). Bug 558827 I don't completely understand, but I do not see the abort reported in comment 21 ("dynamic property of Call object") mentioned there. So what new bug is that abort reason reported in?

/be

Comment 28

[Joe Drew (not getting mail)]

(In reply to comment #27)
> Bug 558830 looks like half of this bug (scripted getters called on trace). Bug
> 558827 I don't completely understand, but I do not see the abort reported in
> comment 21 ("dynamic property of Call object") mentioned there. So what new bug
> is that abort reason reported in?

We haven't yet come up with the testcase. Dave's going to see if he can create one.

Comment 29

[David Humphrey (:humph)]

I reduced the "dynamic property of Call object" case, and spoke to Jason about it.  The issue relates to functions declared within code in an eval then being called outside the eval.  We got rid of it by rewriting the code in the eval.  I think we can ignore this.

Comment 30

[Mike Shaver (:shaver emeritus)]

That sounds like a pretty common unpack-JS pattern, no?  Can you file another bug with that case?

Comment 31

[Jason Orendorff [:jorendorff]]

This bug originally aimed to call all getters and setters except scripted ones on trace using a slow path, the one we use for GETELEM/SETELEM.

That plan never went anywhere, and this stone has gathered too much moss. Time to close it and move on.  -> RESOLVED INVALID

I like the new bugs humph is filing with small test cases and TMFLAGS=abort debug output. I'll start looking at those.

Comment 32

[David Humphrey (:humph)]

(In reply to comment #30)
> That sounds like a pretty common unpack-JS pattern, no?  Can you file another
> bug with that case?

Bug 559323