TM: Call getters and setters on trace

RESOLVED INVALID

Status

()

Core
JavaScript Engine
RESOLVED INVALID
9 years ago
8 years ago

People

(Reporter: jorendorff, Assigned: jorendorff)

Tracking

Other Branch
Points:
---
Dependency tree / graph
Bug Flags:
wanted-fennec1.0 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 obsolete attachments)

(Assignee)

Description

9 years ago
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.
(Assignee)

Updated

9 years ago
Assignee: general → jorendorff
(Assignee)

Comment 1

9 years ago
Created attachment 365598 [details] [diff] [review]
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.
(Assignee)

Comment 2

9 years ago
Blah.  In addition to JSOP_IMACOPATOM, fixing GETARGPROP will require JSOP_IMACOPGETARG, and GETLOCALPROP will require JSOP_IMACOPGETLOCAL.
(Assignee)

Updated

9 years ago
Depends on: 463153
(Assignee)

Comment 3

9 years ago
Created attachment 365923 [details] [diff] [review]
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.
Attachment #365598 - Attachment is obsolete: true
Created attachment 367688 [details] [diff] [review]
WIP 2 (updated to trunk)

I updated this to trunk.
Attachment #365923 - Attachment is obsolete: true
(Assignee)

Comment 5

9 years ago
Well?  Does it work?

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

Comment 6

9 years ago
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.
(Assignee)

Comment 7

9 years ago
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.

Updated

9 years ago
Assignee: jorendorff → gal

Comment 8

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

Comment 9

9 years ago
Created attachment 368489 [details] [diff] [review]
refreshed on top of my fast native patch + TM tip
Attachment #367688 - Attachment is obsolete: true
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.
Duplicate of this bug: 450335

Updated

9 years ago
Depends on: 490666

Updated

9 years ago
No longer depends on: 490666

Comment 12

9 years ago
Created attachment 375259 [details] [diff] [review]
patch

New patch to trace setters. browser starts up.
Attachment #368489 - Attachment is obsolete: true
(Assignee)

Updated

9 years ago
Flags: wanted1.9.2?
(Assignee)

Comment 13

9 years ago
Created attachment 384112 [details] [diff] [review]
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).
Assignee: gal → jorendorff
Attachment #375259 - Attachment is obsolete: true
(Assignee)

Comment 14

9 years ago
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.
(Assignee)

Updated

9 years ago
Depends on: 497886
(Assignee)

Comment 15

9 years ago
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.
(Assignee)

Comment 16

9 years ago
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.
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.
Blocks: 503403
(Assignee)

Updated

9 years ago
Depends on: 503408
(Assignee)

Comment 18

9 years ago
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

9 years ago
This would help a bunch of our frontend code
Flags: wanted-fennec1.0+
Stuart: do you need scripted or native getters and setters? Or both?

Comment 21

9 years ago
Scripted is actually pretty easy and a separate problem. I suggest keeping this bug focused on native.
(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
(Assignee)

Updated

9 years ago
Attachment #384112 - Attachment is obsolete: true
(Assignee)

Updated

9 years ago
Depends on: 507683
(Assignee)

Updated

9 years ago
Depends on: 512714
(Assignee)

Updated

9 years ago
Depends on: 513065
(Assignee)

Updated

9 years ago
Depends on: 513066
(Assignee)

Updated

8 years ago
Flags: wanted1.9.2?
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.")
(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

8 years ago
Luke would be the ideal person to fix this. Or jorendorff. Imacros can do this easily.
> 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.
(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
(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.
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.
That sounds like a pretty common unpack-JS pattern, no?  Can you file another bug with that case?
(Assignee)

Comment 31

8 years ago
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.
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → INVALID
(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
You need to log in before you can comment on or make changes to this bug.