Record assignment before the interpreter goes; avoid record_SetPropHit

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: jorendorff, Assigned: jorendorff)

Tracking

Other Branch
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

(Whiteboard: hardblocker fixed-in-tracemonkey)

Attachments

(1 attachment, 9 obsolete attachments)

Comment hidden (empty)
(Assignee)

Updated

8 years ago
Blocks: 559813
(Assignee)

Comment 1

8 years ago
Created attachment 439589 [details] [diff] [review]
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.
Attachment #439589 - Flags: review?(brendan)
(Assignee)

Comment 2

8 years ago
Created attachment 439608 [details] [diff] [review]
v2

Rebased to tip and fixed a bug that v1 introduced in TR::nativeSet.
Attachment #439589 - Attachment is obsolete: true
Attachment #439608 - Flags: review?(brendan)
Attachment #439589 - Flags: review?(brendan)
Would this possibly make it easier to trace eleminc and the like on non-arrays?
(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
(Assignee)

Comment 5

8 years ago
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.
Attachment #439608 - Flags: review?(brendan)
(Assignee)

Comment 6

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

Comment 7

8 years ago
Created attachment 441609 [details] [diff] [review]
v3
Attachment #439608 - Attachment is obsolete: true
Attachment #441609 - Flags: review?(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 on attachment 441609 [details] [diff] [review]
v3

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

/be
Attachment #441609 - Flags: review?(brendan) → review+
(Assignee)

Comment 10

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

Comment 11

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

Comment 12

8 years ago
http://hg.mozilla.org/tracemonkey/rev/ae857d818793
Whiteboard: fixed-in-tracemonkey
Want a preposition, "To" or "For".

/be
(Assignee)

Comment 14

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

Comment 15

8 years ago
Created attachment 442217 [details] [diff] [review]
interdiff from v3 to v4
(Assignee)

Comment 16

8 years ago
Created attachment 442222 [details] [diff] [review]
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.
Attachment #441609 - Attachment is obsolete: true
Attachment #442222 - Flags: review?(brendan)
(Assignee)

Comment 17

8 years ago
Backout: http://hg.mozilla.org/tracemonkey/rev/8d256e784695
Whiteboard: fixed-in-tracemonkey
(Assignee)

Comment 18

8 years ago
(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 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
Attachment #442222 - Flags: review?(brendan) → review+
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
(Assignee)

Comment 21

8 years ago
Second try: http://hg.mozilla.org/tracemonkey/rev/73f23528bed6
Whiteboard: fixed-in-tracemonkey
(Assignee)

Comment 22

8 years ago
Backed out again. Different failures.

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

Comment 23

8 years ago
http://hg.mozilla.org/mozilla-central/rev/ae857d818793
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED

Comment 24

8 years ago
oops. is this really fixed in tracemonkey?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 25

8 years ago
No.
Whiteboard: fixed-in-tracemonkey
(Assignee)

Comment 26

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

Updated

8 years ago
Whiteboard: fixed-in-tracemonkey
(Assignee)

Comment 27

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

This was the likely cause of a 5% Dromaeo hit.
Whiteboard: fixed-in-tracemonkey
(Assignee)

Comment 28

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

Comment 30

8 years ago
It wasn't, and I was miscounting anyway. Rerunning...
(Assignee)

Comment 31

7 years ago
Created attachment 461270 [details] [diff] [review]
v5

Unbitrotted. Nothing new.
Attachment #442217 - Attachment is obsolete: true
Attachment #442222 - Attachment is obsolete: true
(Assignee)

Comment 32

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

Updated

7 years ago
Blocks: 581785
(Assignee)

Comment 33

7 years ago
Created attachment 494107 [details] [diff] [review]
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.
Attachment #461270 - Attachment is obsolete: true
(Assignee)

Comment 34

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

Updated

7 years ago
Attachment #494107 - Flags: review?(brendan)
Gary, see comment 34.  :-)
(In reply to comment #35)
> Gary, see comment 34.  :-)

OK, will look into it :)
(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
(Assignee)

Comment 38

7 years ago
Created attachment 497659 [details] [diff] [review]
v7 - unbitrotted
Attachment #494107 - Attachment is obsolete: true
Attachment #497659 - Flags: review?(brendan)
Attachment #494107 - Flags: review?(brendan)
(In reply to comment #38)
> Created attachment 497659 [details] [diff] [review]
> v7 - unbitrotted

No big side-effects after half an hour of fuzz-testing. :)
(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.
Need rebase for patch from bug 552432, which landed yesterday.

/be
Nominating because s-s bug 581785 (marked betaN+) depends on this.
blocking2.0: --- → ?
blocking2.0: ? → betaN+
Whiteboard: hardblocker
(Assignee)

Comment 43

7 years ago
Created attachment 503037 [details] [diff] [review]
v8

Rebased.
Attachment #497659 - Attachment is obsolete: true
Attachment #503037 - Flags: review?(brendan)
Attachment #497659 - Flags: review?(brendan)
(Assignee)

Comment 44

7 years ago
My Try server run came back clean (known orange only, and all outside JS).
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
(Assignee)

Comment 46

7 years ago
(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.
(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
(Testcase for bug in patch not landed may seem too much, but sometimes pays off down the road.)

/be
Status: REOPENED → ASSIGNED
[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
(Assignee)

Comment 50

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

Comment 51

7 years ago
Created attachment 503965 [details] [diff] [review]
v9
Attachment #503037 - Attachment is obsolete: true
Attachment #503037 - Flags: review?(brendan)
(Assignee)

Updated

7 years ago
Attachment #503965 - Flags: review?(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
Attachment #503965 - Flags: review?(brendan) → review+
Blocks: 626021
> scripted getter and setter tracing

Bug filed to track this?
(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
Whiteboard: hardblocker → hardblocker fixed-in-tracemonkey
(Assignee)

Updated

7 years ago
Depends on: 626345

Updated

7 years ago
Depends on: 626464
Depends on: 626592
(Assignee)

Comment 55

7 years ago
(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.)
Depends on: 626521
cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/33c58d16d911
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago7 years ago
Resolution: --- → FIXED
(Assignee)

Updated

7 years ago
No longer blocks: 626021

Updated

7 years ago
Depends on: 628334
You need to log in before you can comment on or make changes to this bug.