559653 - Record assignment before the interpreter goes; avoid record_SetPropHit

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Comment 1

[Jason Orendorff [:jorendorff]]

Attachment: v1

This is needed to trace script setters.

The new code is based very closely on the cache-miss paths in the interpreter, for correctness. It's longer than the previous code, which I think mostly reflects how much work the interpreter and property cache were doing for us.

For example, TR::addDataProperty has to guess which slot the new property will get and maybe even create a JSScopeProperty. We don't have that problem if the interpreter goes first and hands us the results.

TR::nativeSet also reflects this--I had to turn several assertions into if statements, because the code can no longer rely on PropertyCache::fill to reject weird cases.

This code also fixes bug 458271 inasmuch as it affects SET instructions in the tracer. v8 and SunSpider show no slowdown.

Comment 2

[Jason Orendorff [:jorendorff]]

Attachment: v2

Rebased to tip and fixed a bug that v1 introduced in TR::nativeSet.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Would this possibly make it easier to trace eleminc and the like on non-arrays?

Comment 4

[Brendan Eich [:brendan]]

(In reply to comment #1)
> Created an attachment (id=439589) [details]
> v1
> 
> This is needed to trace script setters.
> 
> The new code is based very closely on the cache-miss paths in the interpreter,
> for correctness. It's longer than the previous code, which I think mostly
> reflects how much work the interpreter and property cache were doing for us.

Indeed -- that's why I ended up with record_SetPropHit back in the day: too much to duplicate and keep parallel. Is the situation now more stable? It sure is in terms of bugs fixed ;-). Is there a way to reduce the coupling, somehow?

js::PropertyTree::getScopePropertyForObject seems mislocated -- it should be JSScope::getScopeProperty I think.

Can you rebase to tm tip? I'll review quickly. Thanks,

/be

Comment 5

[Jason Orendorff [:jorendorff]]

Comment on attachment 439608 [details] [diff] [review]
v2

(In reply to comment #4)
> Is there a way to reduce the coupling, somehow?

I'll go back and look. Maybe I can duplicate less code by reusing code in jsobj.h. Maybe I can use templates to common up code without incurring runtime "fooHow" or "flags" arguments and conditional branches.

> js::PropertyTree::getScopePropertyForObject seems mislocated -- it should be
> JSScope::getScopeProperty I think.

I'll look at that too. I think I sort of wedged it into the only place that I could put it without making more class members public.

This Monday I'll check on those two ideas and rebase to tip.

Comment 6

[Jason Orendorff [:jorendorff]]

(In reply to comment #5)
> (In reply to comment #4)
> > Is there a way to reduce the coupling, somehow?
> 
> I'll go back and look. Maybe I can duplicate less code by reusing code in
> jsobj.h. Maybe I can use templates to common up code without incurring runtime
> "fooHow" or "flags" arguments and conditional branches.

I looked, but this wasn't going to be a big savings, nor was it going to be pretty.

> > js::PropertyTree::getScopePropertyForObject seems mislocated -- it should be
> > JSScope::getScopeProperty I think.

Yep. Relocated with no problems at all.

Comment 7

[Jason Orendorff [:jorendorff]]

Attachment: v3

Comment 8

[Brendan Eich [:brendan]]

Comment on attachment 441609 [details] [diff] [review]
v3

JSScope::getScopeProperty might fit better in the JSScope class, and in the .cpp file, right near getChildProperty.

As MethodWriteBarrier is BOOL_FAIL, does it need the enter/leaveDeepBail calls bracketing it?

/be

Comment 9

[Brendan Eich [:brendan]]

Comment on attachment 441609 [details] [diff] [review]
v3

Looks ok otherwise -- optimistically marking, given nit fix and answer to previous comment's question.

/be

Comment 10

[Jason Orendorff [:jorendorff]]

(In reply to comment #8)
> (From update of attachment 441609 [details] [diff] [review])
> JSScope::getScopeProperty might fit better in the JSScope class, and in the
> .cpp file, right near getChildProperty.
> 
> As MethodWriteBarrier is BOOL_FAIL, does it need the enter/leaveDeepBail calls
> bracketing it?

I'm convinced it does not, but this patch moves that code and it would be tricksy to change it at the same time. So I'll fix that in bug 561200.

Comment 11

[Jason Orendorff [:jorendorff]]

(In reply to comment #8)
> (From update of attachment 441609 [details] [diff] [review])
> JSScope::getScopeProperty might fit better in the JSScope class, and in the
> .cpp file, right near getChildProperty.

I looked closer at it and the new method is actually a variant of addProperty() that creates or finds the appropriate JSScopeProperty but doesn't actually add it.

So I called it JSScope::prepareAddProperty() and gave it exactly the same signature as JSScope::addProperty(), but const.

Comment 12

[Jason Orendorff [:jorendorff]]

http://hg.mozilla.org/tracemonkey/rev/ae857d818793

Comment 13

[Brendan Eich [:brendan]]

Want a preposition, "To" or "For".

/be

Comment 14

[Jason Orendorff [:jorendorff]]

Ah - sure, for some reason I was thinking along the lines of JDBC 'prepareStatement', so 'AddProperty' was, in my head, a noun -- hopeless! 'prepareForAddProperty' is much better.

This bounced off Tinderbox. I'll re-post for review today.

Comment 15

[Jason Orendorff [:jorendorff]]

Attachment: interdiff from v3 to v4

Comment 16

[Jason Orendorff [:jorendorff]]

Attachment: v4

v1-v3 tried to fix a bug in the way we handle the method write barrier on trace, but the fix makes the bug rather worse; it would trip assertions in a mochitest.

v4 fixes the bug properly and adds 3 trace-tests (2 for the existing bug and 1 that tests for the mistake in v3).

Still not fixing bug 561200 here -- it turns out we *can* deep-bail in there, so it needs a bit more fixing than I want to do in this bug.

Comment 17

[Jason Orendorff [:jorendorff]]

Backout: http://hg.mozilla.org/tracemonkey/rev/8d256e784695

Comment 18

[Jason Orendorff [:jorendorff]]

(In reply to comment #14)
> Ah - sure, for some reason I was thinking along the lines of JDBC
> 'prepareStatement', so 'AddProperty' was, in my head, a noun -- hopeless!
> 'prepareForAddProperty' is much better.

...I guess it's still a noun. Still, the preposition is what makes sense of it: after "for" you strongly expect a noun phrase, so you automatically read "AddProperty" as a name. Or so I hope.

Comment 19

[Brendan Eich [:brendan]]

Comment on attachment 442222 [details] [diff] [review]
v4

>+    bool inDictionaryMode() const   { return flags & DICTIONARY_MODE; }
>+    void setDictionaryMode()        { flags |= DICTIONARY_MODE; }
>+    void clearDictionaryMode()      { flags &= ~DICTIONARY_MODE; }
>+    bool sealed() const             { return flags & SEALED; }
>+    bool branded() const            { JS_ASSERT(!generic()); return flags & BRANDED; }
>+    bool generic() const            { return flags & GENERIC; }
>+    void setGeneric()               { flags |= GENERIC; }
>+    bool hadIndexedProperties() const { return flags & INDEXED_PROPERTIES; }
>+    void setIndexedProperties()     { flags |= INDEXED_PROPERTIES; }
>+    bool hasOwnShape() const        { return flags & OWN_SHAPE; }
>+    bool hasRegenFlag(uint8 regenFlag) const { return (flags & SHAPE_REGEN) == regenFlag; }
..1234567890123456789012345678901234567890

Nit: why not indent to column 40 for the {s and let hasRegenFlag (which stands alone) be the only exception?

>+    JSScopeProperty* sprop = obj->scope()->prepareForAddProperty(cx, id, getter, setter, slot,
>+                                                                 JSPROP_ENUMERATE, flags, shortid);
>+    if (!sprop)
>+        RETURN_ERROR("JSScope::getScopeProperty failed");

Abort reason string needs updating.

Looks good otherwise, but I am rushed. Will review fully later tonight.

/be

Comment 20

[Brendan Eich [:brendan]]

Comment on attachment 442222 [details] [diff] [review]
v4

>+SafeLookup(JSContext *cx, JSObject* obj, jsid id, JSObject** pobjp, JSScopeProperty** spropp)
>+{
>+    for (; obj; obj = obj->getProto()) {

Could use do {...} while ((obj = obj->getProto()) != NULL); here.

>+    // Handle setting an inherited non-SHARED property.
>+    if (sprop->hasSlot()) {
>+        if (sprop->hasShortID()) {
>+            return addDataProperty(obj, obj_ins, id, sprop->getter(), sprop->setter(),
>+                                   JSScopeProperty::HAS_SHORTID, sprop->shortid,
>+                                   v, v_ins);
>+        } else {

else after return non-sequitur.

>+            JSClass *cls = obj->getClass();
>+            return addDataProperty(obj, obj_ins, id, cls->getProperty, cls->setProperty,
>+                                   0, 0, v, v_ins);

Common tail could be fused with some more locals: getter, setter, flags, shortid. Compiler can do it for sure but it could help readability too.

Nice work. Wish we could common the logic, somehow. Templates? Future bug.

/be

Comment 21

[Jason Orendorff [:jorendorff]]

Second try: http://hg.mozilla.org/tracemonkey/rev/73f23528bed6

Comment 22

[Jason Orendorff [:jorendorff]]

Backed out again. Different failures.

http://hg.mozilla.org/tracemonkey/rev/5aa83042d4d3

Comment 23

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/ae857d818793

Comment 24

[Robert Sayre]

oops. is this really fixed in tracemonkey?

Comment 25

[Jason Orendorff [:jorendorff]]

No.

Comment 26

[Jason Orendorff [:jorendorff]]

The bug was a GC hazard. Fixed with:

    5.24      if (inDictionaryMode()) {
    5.25          JSScopeProperty *sprop = JS_PROPERTY_TREE(cx).newScopeProperty(cx, true);
    5.26          if (sprop) {
    5.27              new (sprop) JSScopeProperty(id, getter, setter, slot, attrs, flags, shortid);
    5.28 +            sprop->parent = NULL;
    5.29 +            sprop->kids = NULL;
    5.30 +            sprop->shape = JSObjectMap::SHAPELESS;
    5.31          }
    5.32          return sprop;
    5.33      }

(valgrind would have caught this, but running the browser under valgrind on my super-slow mac laptop wouldn't have been fun either.)

Re-landed: http://hg.mozilla.org/tracemonkey/rev/a72a9d72c028
And a trivial merge: http://hg.mozilla.org/tracemonkey/rev/53a1da406359

Comment 27

[Jason Orendorff [:jorendorff]]

Backed out. http://hg.mozilla.org/tracemonkey/rev/b18beea31ad3

This was the likely cause of a 5% Dromaeo hit.

Comment 28

[Jason Orendorff [:jorendorff]]

According to http://dromaeo.com/?id=106014,106016 we lose the most on "DOM Traversal - childNodes" (before 83 runs/s -> after 23 runs/s).

But when I take it out of the Dromaeo harness, it does 2400 runs/s.

Comment 29

[Boris Zbarsky [:bzbarsky]]

The harness is key for Dromaeo; it involves closures, etc, and is _very_ sensitive to closure property access.

Was your out-of-harness version writing to a closure var ("ret") the way dromaeo does?

As a separate note, I'm surprised by the 2400 runs/s, since I wouldn't expect us to trace that loop, in or out of harness...

Comment 30

[Jason Orendorff [:jorendorff]]

It wasn't, and I was miscounting anyway. Rerunning...

Comment 31

[Jason Orendorff [:jorendorff]]

Attachment: v5

Unbitrotted. Nothing new.

Comment 32

[Jason Orendorff [:jorendorff]]

This fixes bug 581785, but see comment 27. I never found time to chase down the performance regression.

It is likely something simple. Does anyone have time to push this through?

Comment 33

[Jason Orendorff [:jorendorff]]

Attachment: v6

Unbitrotted and scaled back slightly.

Now, instead of the TR::record_SetPropHit hook, we have TR::record_AddProperty, which triggers only when the interpreter actually adds a new property to an object.

This is a compromise. Keeping TR::record_AddProperty means the tracer doesn't have to predict in every detail what shape the interpreter is going to add, which was sort of a silly amount of code for something the interpreter must determine anyway. At the same time, the tracer predicts what assignment will do in the case we need for tracing script setters. And as before, we replace an overly generic abort (where the interpreter simply told TR::record_SetPropHit "sorry, you lose") with specific aborts that we can patch later if perf warrants.

Passes tests. I still need to fuzz-test and benchmark this.

Comment 34

[Jason Orendorff [:jorendorff]]

OK. This shows no change in SunSpider or V8. I can't fuzz-test this now.

Bug 560072 is no longer a blocker, but I think this bug is important. Let's get it in.

Comment 35

[Jeff Walden [:Waldo]]

Gary, see comment 34.  :-)

Comment 36

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to comment #35)
> Gary, see comment 34.  :-)

OK, will look into it :)

Comment 37

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to comment #33)
> Created attachment 494107 [details] [diff] [review]
> v6
> 
> Unbitrotted and scaled back slightly.

This patch seems to have bitrotted (again).

patching file shell/js.cpp
Hunk #1 succeeded at 1285 (offset 154 lines).
(Stripping trailing CRs from patch.)
patching file jit-test/tests/basic/testInitPropOverMethod.js
(Stripping trailing CRs from patch.)
patching file jsapi-tests/Makefile.in
Hunk #1 FAILED at 63.
1 out of 1 hunk FAILED -- saving rejects to file jsapi-tests/Makefile.in.rej
(Stripping trailing CRs from patch.)
patching file jsapi-tests/testSetPropertyWithNativeGetterStubSetter.cpp
(Stripping trailing CRs from patch.)
patching file jsinterp.cpp
Hunk #1 succeeded at 3214 (offset -3 lines).
Hunk #2 succeeded at 4408 (offset -1 lines).
Hunk #3 succeeded at 5942 (offset -4 lines).
Hunk #4 succeeded at 5983 (offset -1 lines).
(Stripping trailing CRs from patch.)
patching file jsobj.cpp
Hunk #1 succeeded at 4628 (offset 152 lines).
Hunk #3 succeeded at 5410 (offset 152 lines).
Hunk #4 succeeded at 5343 (offset -1 lines).
Hunk #5 succeeded at 5542 (offset 152 lines).
(Stripping trailing CRs from patch.)
patching file jsobj.h
Hunk #1 FAILED at 1011.
1 out of 1 hunk FAILED -- saving rejects to file jsobj.h.rej
(Stripping trailing CRs from patch.)
patching file jsscope.cpp
(Stripping trailing CRs from patch.)
patching file jsscope.h
(Stripping trailing CRs from patch.)
patching file jstracer.cpp
Hunk #2 succeeded at 7144 (offset -1 lines).
Hunk #3 succeeded at 11745 (offset 5 lines).
Hunk #4 succeeded at 12069 (offset -1 lines).
Hunk #5 succeeded at 14355 (offset 3 lines).
Hunk #6 succeeded at 14950 (offset -1 lines).
Hunk #7 succeeded at 16163 (offset 3 lines).
(Stripping trailing CRs from patch.)
patching file jstracer.h
(Stripping trailing CRs from patch.)
patching file trace-test/tests/basic/testMethodWriteBarrier.js
(Stripping trailing CRs from patch.)
patching file trace-test/tests/basic/testMethodWriteBarrier2.js
(Stripping trailing CRs from patch.)
patching file trace-test/tests/basic/testMethodWriteBarrier3.js

Comment 38

[Jason Orendorff [:jorendorff]]

Attachment: v7 - unbitrotted

Comment 39

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to comment #38)
> Created attachment 497659 [details] [diff] [review]
> v7 - unbitrotted

No big side-effects after half an hour of fuzz-testing. :)

Comment 40

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to comment #39)
> (In reply to comment #38)
> > Created attachment 497659 [details] [diff] [review] [details]
> > v7 - unbitrotted
> 
> No big side-effects after half an hour of fuzz-testing. :)

Tested on a 32-bit debug js shell on Windows 7.

Comment 41

[Brendan Eich [:brendan]]

Need rebase for patch from bug 552432, which landed yesterday.

/be

Comment 42

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Nominating because s-s bug 581785 (marked betaN+) depends on this.

Comment 43

[Jason Orendorff [:jorendorff]]

Attachment: v8

Rebased.

Comment 44

[Jason Orendorff [:jorendorff]]

My Try server run came back clean (known orange only, and all outside JS).

Comment 45

[Brendan Eich [:brendan]]

Comment on attachment 503037 [details] [diff] [review]
v8

This patch seems better over time ;-). Close to r+, one question at bottom after some nits.

INITMETHOD) case has isNative assert, no longer needed due to added isObject
(fix to use that used directly instead of using its hand-expansion)

Several "TODO - fall back on slow path" comments should be FIXME: bug nnnnnn.

    // On trace, call js_AddProperty to do the dirty work.

s/Add/Add{,Atom}/

    // ... The Call objects we create on trace are bogus

How so? FIXME needed?

    Class *clasp = obj->getClass();
    if (clasp == &js_CallClass)

Use isCall() here, there's no further use of clasp AFAICS.

Four TRACE_1(AddProperty, obj) calls is better, but still a bit a drag. I
didn't understand the meaning of deferred, controlling

        set(&l, r_ins);

in TraceRecorder::recordSetPropertyOp. That bool out param comes back true only
in the addDataProperty(obj) tail call cases in TraceRecorder::setProperty, but
don't we need to update the tracker for &l in all cases?

/be

Comment 46

[Jason Orendorff [:jorendorff]]

(In reply to comment #45)
>     // ... The Call objects we create on trace are bogus
> 
> How so? FIXME needed?

No, no FIXME. The objects are not really "bogus"; rather, like so much of the tracer state, they're not fully populated until we leave trace. I'll change the comment.

> Four TRACE_1(AddProperty, obj) calls is better, but still a bit a drag. I
> didn't understand the meaning of deferred, controlling
> 
>         set(&l, r_ins);
> 
> in TraceRecorder::recordSetPropertyOp. That bool out param comes back true only
> in the addDataProperty(obj) tail call cases in TraceRecorder::setProperty, but
> don't we need to update the tracker for &l in all cases?

deferred=true means a property will be added, so (a) we didn't know the resulting shape; (b) we didn't emit code to add it; (c) we're expecting a TRACE_1(AddProperty, obj) callback from the interpreter.

The logic of the current patch is: call set(&l, r_ins) after emitting all the code for the property assignment.

But as you point out, there is no actual dependency between the two! So it'll be much simpler to set the result unconditionally in recordSetPropertyOp. I'll make the change.

Comment 47

[Brendan Eich [:brendan]]

(In reply to comment #46)
> But as you point out, there is no actual dependency between the two! So it'll
> be much simpler to set the result unconditionally in recordSetPropertyOp. I'll
> make the change.

Right, there's no actual dependency -- but the lack of tracker update was a bug, right? If so, worth a testcase?

/be

Comment 48

[Brendan Eich [:brendan]]

(Testcase for bug in patch not landed may seem too much, but sometimes pays off down the road.)

/be

Comment 49

[Brendan Eich [:brendan]]

[12:31pm] brendan: jorendorff: https://bugzilla.mozilla.org/show_bug.cgi?id=559653#c47
[12:32pm] jorendorff: brendan: No, it wasn't missing -- it was postponed until record_AddProperty though
[12:32pm] brendan: how'd that get &l?
[12:33pm] jorendorff: brendan: toward the bottom, "// Finish off a SET instruction by moving sp[-1] to sp[-2]."
[12:33pm] brendan: oic
[12:33pm] brendan: thx
[12:33pm] jorendorff: that tracker update is just moving the assigned value to the right stack slot
[12:33pm] brendan: yeah, so consolidate? easier to follow
[12:33pm] jorendorff: yes, for sure

Comment 50

[Jason Orendorff [:jorendorff]]

D'oh. I was wrong. There is a dependency.

set(&l, r_ins) modifies the tracker's entry for sp[-2]. record_AddProp calls stackval(-2).

So I'll leave `deferred` where it is, with a better comment.

Comment 51

[Jason Orendorff [:jorendorff]]

Attachment: v9

Comment 52

[Brendan Eich [:brendan]]

Comment on attachment 503965 [details] [diff] [review]
v9

Thanks, great to get this in.

Post-Mozill-2/Firefox-4, scripted getter and setter tracing should be a great boost for code that uses short scripted accessors.

/be

Comment 53

[Boris Zbarsky [:bzbarsky]]

> scripted getter and setter tracing

Bug filed to track this?

Comment 54

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to comment #52)
> Comment on attachment 503965 [details] [diff] [review]
> v9
> 
> Thanks, great to get this in.
> 
> Post-Mozill-2/Firefox-4, scripted getter and setter tracing should be a great
> boost for code that uses short scripted accessors.
> 
> /be

http://hg.mozilla.org/tracemonkey/rev/33c58d16d911

Comment 55

[Jason Orendorff [:jorendorff]]

(In reply to comment #53)
> > scripted getter and setter tracing
> 
> Bug filed to track this?

Script getters are traced. For script setters, see bug 559813. (That bug has always depended on this one. The already-reviewed patch there is stale, but the same basic approach will work.)

Comment 56

[Chris Leary [:cdleary] (not checking bugmail)]

cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/33c58d16d911