471214 - Join function objects transparently, clone via read barrier to satisfy de-facto standard

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Jeff Walden [:Waldo]]

(followup to bug 470137 since that bug's closed now and it's not worth reopening to handle this)

Once we enter a trace for the loop in testAddAnyInconvertibleObject (and similar in the other tests adjacent to it), we shouldn't be exiting as far as I can tell, but for some reason we are.  (It's easiest to see we're exiting once each loop by changing the loop end to 100 and the count-condition to 95 and putting a |return "pass";| after the for-loop but inside the try, then looking at the printed jitstats.)  I was testing for only a single side exit in the expected jitstats, but the "sideExits" typo means that test isn't happening.

From stepping through the code, we're hitting the following trace:

    $testAddAnyInconvertibleObject.count = i2f ld1
    -40
    $testAddAnyInconvertibleObject.toString = ld sp[-40]
    -32
    $testAddAnyInconvertibleObject.threw = ld sp[-32]
    -24
    ld2 = ld sp[-24]
    $testAddAnyInconvertibleObject.i = i2f ld2
    -16
    $testAddAnyInconvertibleObject.o = ld sp[-16]
    -8
    $testAddAnyInconvertibleObject.q = ld sp[-8]
    obj
    js_FastNewObject1 = js_FastNewObject ( cx obj )

    eq1 = eq js_FastNewObject1, 0
    xt1: xt eq1 -> 0:25 sp+0 rp+0

import vp=0x2051398 name=$global0 type=boolean flags=0
    sti sp[0] = js_FastNewObject1
    2896
    $global0 = ld gp[2896]
    sti sp[8] = $global0
    ld3 = ld js_FastNewObject1[0]
    ops = ld ld3[4]
    ld4 = ld ops[20]
    OP(&js_ObjectOps)
    guard(native-map) = eq ld4, OP(&js_ObjectOps)
    xf1: xf guard(native-map) -> 0:30 sp+16 rp+0

    shape = ld ld3[16]
    569
    guard(shape) = eq shape, 569
    xf2: xf guard(shape) -> 0:30 sp+16 rp+0

    sprop
    js_AddProperty1 = js_AddProperty ( cx js_FastNewObject1 sprop )

    eq2 = eq js_AddProperty1, 0
    xt2: xt eq2 -> 0:30 sp+16 rp+0

    3
    lsh1 = lsh $global0, 3
    6
    or1 = or lsh1, 6
    sti js_FastNewObject1[16] = or1
    sti sp[8] = $testAddAnyInconvertibleObject.toString
    ld5 = ld js_FastNewObject1[0]
    ops = ld ld5[4]
    ld6 = ld ops[20]
    guard(native-map) = eq ld6, OP(&js_ObjectOps)
    xf3: xf guard(native-map) -> 0:36 sp+16 rp+0

    shape = ld ld5[16]
    636
    guard(shape) = eq shape, 636
    xf4: xf guard(shape) -> 0:36 sp+16 rp+0

    sprop
    js_AddProperty2 = js_AddProperty ( cx js_FastNewObject1 sprop )

    eq3 = eq js_AddProperty2, 0
    xt3: xt eq3 -> 0:36 sp+16 rp+0

    sti js_FastNewObject1[20] = $testAddAnyInconvertibleObject.toString
    sti sp[-16] = js_FastNewObject1
    #40140000:0 /* 5 */
    5
    sti sp[0] = 5
    sti sp[8] = js_FastNewObject1
    sti sp[16] = js_FastNewObject1
    sti sp[24] = js_FastNewObject1
    globalObj
    eq4 = eq js_FastNewObject1, globalObj
    xt4: xt eq4 -> 3:49 sp+32 rp+0

    ld7 = ld js_FastNewObject1[0]
    ops = ld ld7[4]
    ld8 = ld ops[16]
    OP(&js_ObjectOps)
    guard(native-map) = eq ld8, OP(&js_ObjectOps)
    xf5: xf guard(native-map) -> 3:49 sp+32 rp+0

    shape = ld ld7[16]
    637
    guard(kshape) = eq shape, 637
    xf6: xf guard(kshape) -> 3:49 sp+32 rp+0

    ld9 = ld js_FastNewObject1[16]
    JSVAL_TAGMASK
    and1 = and ld9, JSVAL_TAGMASK
    eq5 = eq and1, 6
    xf7: xf eq5 -> 3:49 sp+32 rp+0

    ush1 = ush ld9, 3
    sti sp[24] = ush1
    ld10 = ld js_FastNewObject1[0]
    ops = ld ld10[4]
    ld11 = ld ops[16]
    guard(native-map) = eq ld11, OP(&js_ObjectOps)
    xf8: xf guard(native-map) -> 24:49 sp+16 rp+0

    shape = ld ld10[16]
    639
    guard(kshape) = eq shape, 639
    xf9: xf guard(kshape) -> 24:49 sp+16 rp+0

If I'm reading this right, we have a kshape guard of shape==637 in xf6.  Then, after doing nothing whatsoever to change the shape, we have a kshape guard of shape==639 in xf9, but of course the shape is still 637, so we exit the trace.

Comment 1

[Jeff Walden [:Waldo]]

I fixed the original typo in the name of the side-exit property and bumped iteration counts up enough to make the side-exit problem clear.  The current numbers, oddly, are correct in a full run, but if you run the individual tests (at least some, possibly all of them) the side-exit count is off by 1.  I'm not sure why this would be happening, but this should be addressed at the same time as this bug.

Comment 2

[Bob Clary [:bc] (inactive)]

Waldo, 

in testBitOrInconvertibleObjectAny, did you miss updating the return for |if (i !== 94)| ? shouldn't it read?

if (i !== 94)
  return "expected i === 94, got " + i;

Comment 3

[Jeff Walden [:Waldo]]

Quite; fixed in TM.

Comment 4

[Jeff Walden [:Waldo]]

        if (cs->format & JOF_CALLOP) {
            if (SPROP_HAS_STUB_GETTER(sprop) &&
                SPROP_HAS_VALID_SLOT(sprop, scope)) {
                jsval v;

                v = LOCKED_OBJ_GET_SLOT(pobj, sprop->slot);
                if (VALUE_IS_FUNCTION(cx, v)) {
                    /*
                     * Great, we have a function-valued prototype property
                     * where the getter is JS_PropertyStub. The type id in
                     * pobj's scope does not evolve with changes to property
                     * values, however.
                     *
                     * So here, on first cache fill for this method, we brand
                     * the scope with a new shape and set the SCOPE_BRANDED
                     * flag.  Once this scope flag is set, any write that adds
                     * or deletes a function-valued plain old property in
                     * scope->object will result in shape being regenerated.
                     */
                    if (!SCOPE_IS_BRANDED(scope)) {
                        PCMETER(cache->brandfills++);
#ifdef DEBUG_notme
                        fprintf(stderr,
                            "branding %p (%s) for funobj %p (%s), kshape %lu\n",
                            pobj, LOCKED_OBJ_GET_CLASS(pobj)->name,
                            JSVAL_TO_OBJECT(v),
                            JS_GetFunctionName(GET_FUNCTION_PRIVATE(cx,
                                                 JSVAL_TO_OBJECT(v))),
                            kshape);
#endif
                        SCOPE_MAKE_UNIQUE_SHAPE(cx, scope);


We're hitting the above code in js_FillPropertyCache while tracing the JSOP_CALLPROP(valueOf) in the addition imacro and deciding that, because we're not branded yet (the loop creates a new object every time around), we need to bump the scope's shape and brand the scope.  Looks like maybe TraceRecorder::test_property_cache needs to do this as well somehow, maybe if a function is being set guard on that and make sure to update the shape if needed or something?  Not immediately obvious yet, but at least this is the root of the problem.

Comment 5

[Jeff Walden [:Waldo]]

One hackaround for the tests like this one would be to not create the object every time around, which I verified will make the side-exit count O(1), but this pattern certainly appears in non-stupid code as well (don't actually know what I was thinking when I wrote this as I did -- but good I did!), so it's still worth fixing.

Comment 6

[Jeff Walden [:Waldo]]

TraceRecorder::test_property_cache seems to duplicate do_getprop_with_obj from jsinterp.cpp, which is kind of bogus -- it's possible we should semi-copy that with alterations for callprop.

Comment 7

[Brendan Eich [:brendan]]

This test shows a callsite that calls the same method over and over, so the bug is in the property cache and scope-branding code, not in the tracing JIT.

Code duplication is not bad, but we must not try to guess or prefigure in the recorder what might or might not happen in the opcode, other than to fill the cache to prime a hit when the record hook returns to the interpreter. Otherwise we can get double effects, etc. (resolve and get are not idempotent).

Waldo, we talked about moving the brand-on-fill to brand-on-initprop, where the existing record_SetPropHit hook can get the post-brand shape. Did you try this yet? It looks like the fix to me right now.

/be

Comment 8

[Brendan Eich [:brendan]]

Alternative fix: keep brand-on-fill and add a record_CallPropDone to help the JIT sample the post-brand shape.

/be

Comment 9

[Brendan Eich [:brendan]]

Hrm. The object is new each time through the loop but its shape before the branding step is loop-invariant. The method being assigned to toString (I don't see anything other than valueOf: undefined) is loop-invariant. But once branded (by the first iteration, before HOTLOOP is reached) we will mindlessly make a unique shape each time, for the same given pre-fill-in-initprop object shape and function object.

We need to improve the property cache code, which I started out saying. This is an interp perf hit, without tracing. We should be able to find the appropriate derived shape, not re-generate new unique shapes for the same loop invariants.

Waldo, feel free to give this bug to me if you want to ditch it.

/be

Comment 10

[Jeff Walden [:Waldo]]

Attachment: Non-working patch

This crashes pretty quickly in trace-test.js, of course, haven't had time to look more deeply.

Comment 11

[Jeff Walden [:Waldo]]

Eager branding kills us on this test, extracted from trace-test.js

/************************************************************************/
function my_iterator_next() {
    if (this.i == 10) {
        this.i = 0;
        throw this.StopIteration;
    }
    return this.i++;
}

var o = {
  __iterator__: function () {
    return {
             i: 0,
             next: my_iterator_next,
             StopIteration: StopIteration
           };
  }
};

var a=[];
for (var k = 0; k < 100; k += 10) {
    print("k: " + (k + ""));
    for(var j in o) {
        print("k: " + (k + "") + ", j: " + (j + ""));
        a[k + (j >> 0)] = j*k;
    }
}

if (a.join() !==
    "0,0,0,0,0,0,0,0,0,0," +
    "0,10,20,30,40,50,60,70,80,90," +
    "0,20,40,60,80,100,120,140,160,180," +
    "0,30,60,90,120,150,180,210,240,270," +
    "0,40,80,120,160,200,240,280,320,360," +
    "0,50,100,150,200,250,300,350,400,450," +
    "0,60,120,180,240,300,360,420,480,540," +
    "0,70,140,210,280,350,420,490,560,630," +
    "0,80,160,240,320,400,480,560,640,720," +
    "0,90,180,270,360,450,540,630,720,810")
{
  print("FAIL");
}
else
{
  print("PASS");
}
/************************************************************************/

We loop around a bunch and hit the new iterator object returned every time, but because its shape doesn't evolve predictably each time we eventually hit an assert:

k: 0
k: 0, j: 0
k: 0, j: 1
k: 0, j: 2
k: 0, j: 3
k: 0, j: 4
k: 0, j: 5
k: 0, j: 6
k: 0, j: 7
k: 0, j: 8
k: 0, j: 9
k: 10
k: 10, j: 0
k: 10, j: 1
k: 10, j: 2
k: 10, j: 3
k: 10, j: 4
k: 10, j: 5
k: 10, j: 6
k: 10, j: 7
k: 10, j: 8
k: 10, j: 9
k: 20
Assertion failure: PCVCAP_MAKE(sprop->shape, 0, 0) == entry->vcap, at /Users/jwalden/moz/js-tm/js/src/jsinterp.cpp:6221

Not sure yet what to do here...

Comment 12

[Jeff Walden [:Waldo]]

(This is when doing the initprop for the "next" property, if it wasn't clear.)

Comment 13

[Brendan Eich [:brendan]]

Do not go through the write barrier and make a unique shape due to presetting BRANDED on the first set/init, there is no need to assign a new shape because no one could have cached an old one. Just set BRANDED. Easiest given the layering to do this after the GC_WRITE_BARRER call.

Once you fix that, any perf regressions?

/be

Comment 14

[Jeff Walden [:Waldo]]

Comment 13 doesn't make sense.  If we're setting a function-valued property we have to have a unique shape (or at least unique to the particular function being set) so that we can depend on the shape for the identity of the function at that property when tracing.  Consider:

var o = { fun: function() { return 12; } };
var p = { fun: function() { return 17; } };
var a = [o, o, o, o, p, p, p, p];
for each (var o in a)
  print(o.fun());

Further evidence: all of trace-test.js except testComparisons passes with that change.  testComparisons fails horribly because every value has the same shape, that of the first value, I believe, so the results at a glance seem to be as one would expect if every comparison were with |undefined| (so naturally a ton of failures).

Comment 15

[Brendan Eich [:brendan]]

Oops, you're right, I was thinking of the upvar2 optimization (not yet patched) that defers cloning function objects. Can you hold on this bug until I have that optimization (which is necessary to fix the testcase in upvar2) working? Should be by tomorrow night.

/be

Comment 16

[Brendan Eich [:brendan]]

Patch in my mq split from upvar2, talked to Waldo about this -- I will try to get it fixed tomorrow.

/be

Comment 17

[Robert Sayre]

(In reply to comment #16)
> Patch in my mq split from upvar2, talked to Waldo about this -- I will try to
> get it fixed tomorrow.

Got this coming?

Comment 18

[Brendan Eich [:brendan]]

Yes, this is in my q -- need to fix some regressions first. Still doable by the freeze.

/be

Comment 19

[Mike Beltzner [:beltzner, not reading bugmail]]

Do we have an ETA for a fix and a reviewer on deck?

Comment 20

[Brendan Eich [:brendan]]

Not a b4 blocker, and this kept getting starved by higher-priority stuff (or at least stuff that needed diagnosis to prove it wasn't higher priority, which often meant a fix along with diagnosis).

/be

Comment 21

[Brendan Eich [:brendan]]

Attachment: work in progress

js> function C()({m:function()42})
js> o=C()
[object Object]
js> shapeOf(o)
130
js> p=C()
[object Object]
js> shapeOf(p)
130
js> q=C()
[object Object]
js> shapeOf(q)
130
js> o.m()
42
js> shapeOf(o)
130
js> f=o.m
function () 42
js> f===o.m
true
js> f===p.m
false
js> f===q.m
false
js> shapeOf(p)
141
js> shapeOf(q)
142
js> shapeOf(o)
137

Seems to be a perf win on bench.sh but my MBP is noisy and in need of an OS patch (very laggy cmd-tab app-switching, not sure why). Anyone with time and interest please benchmark and report results.

The patch is missing trace-JIT method barrier code generation, but the idea is to emit that overhead only when the shape guards a scope with SCOPE_METHOD_BARRIER.

/be

Comment 22

[Brendan Eich [:brendan]]

This regresses performance -- must be the read barrier, which adds overhead for all property gets, not just for method gets, in the interpreter (I haven't done the tracing side, which would avoid any penalty for non-method gets). Have to time this out for 1.9.1, but the patch is usable. If only ES3's attempt to allow joined function objects were credible on the web!

/be

Comment 23

[Brendan Eich [:brendan]]

** TOTAL **:           *1.019x as slow*  2200.6ms +/- 0.1%   2243.3ms +/- 0.3%     significant

Across the board -- no test was unharmed.

/be

Comment 24

[Jeff Walden [:Waldo]]

Comment on attachment 378635 [details] [diff] [review]
work in progress

While you're here...

>                  * We assume that on average the win from this optimization is
>-                 * bigger that the cost of an extra mismatch per loop due to
>+                 * bigger that the cost of an extra mismatch per loop owing to
>                  * the bias for the following case:

"bigger than"

>                  *   obj = {}; ... for (...) { ... obj.x = ... }
>                  *
>-                 * On the first iteration JSOP_SETPROP fills the cache with
>-                 * the shape of newly created object, not the shape after
>-                 * obj.x is assigned. That mismatches obj's shape on the
>-                 * second iteration. Note that on third and the following
>-                 * iterations the cache will be hit since the shape no longer
>-                 * mutates.
>+                 * On the first iteration of such a for loop, JSOP_SETPROP
>+                 * fills the cache with the shape of newly created object, not

"of the newly"

>+                 * the shape of obj after obj.x is assigned. That mismatches
>+                 * obj's shape on the second iteration. Note that on the third
>+                 * and subsequent iterations the cache will be hit because the
>+                 * shape no longer updated.

"is no longer"

Comment 25

[Brendan Eich [:brendan]]

I think I can hide the read barrier cost in the existing non-stub getter barrier. More tomorrow.

/be

Comment 26

[Brendan Eich [:brendan]]

Prioritizing this over bug 497789 since that wants jorendorff's patch for bug 503080, while this bug is collecting dups/deps.

/be

Comment 27

[Brendan Eich [:brendan]]

(In reply to comment #26)
> Prioritizing this over bug 497789 since that wants jorendorff's patch for bug
> 503080, while this bug is collecting dups/deps.

But not bug 504829, it turns out. Still, the lack of a fix here hurts benchmarks IIRC including Mark Steele's.

/be

Comment 28

[Brendan Eich [:brendan]]

If we compute the maximum UPVAR_FRAME_SKIP when inlining a call to a function fun having upvars that are not optimized such that FUN_FLAT_CLOSURE(fun), and the skip amount is "in range" by one or another of our upvar optimizations, then we can avoid guarding on parent.

This could be an important optimization. Have to check the popular benchmarks.

/be

Comment 29

[Brendan Eich [:brendan]]

(In reply to comment #28)
Argh, this comment was for bug 504829.

/be

Comment 30

[Brendan Eich [:brendan]]

Attachment: updated patch for clone-avoiding method barrier

Comment 31

[Brendan Eich [:brendan]]

Attachment: updated patch, with fixes

Try server chewing on this. Passes js testsuite.

/be

Comment 32

[Brendan Eich [:brendan]]

Attachment: refreshed to tm tip

Comment 33

[Brendan Eich [:brendan]]

Attachment: refreshed again

Comment 34

[Brendan Eich [:brendan]]

Attachment: passes tryservers

Comment 35

[Igor Bukanov]

Comment on attachment 390690 [details] [diff] [review]
passes tryservers

>@@ -3725,6 +3729,16 @@ js_DefineNativeProperty(JSContext *cx, J
>+            JS_ASSERT(!getter && !setter);
>+            flags |= SPROP_IS_METHOD;
>+            getter = JS_EXTENSION (JSPropertyOp) JSVAL_TO_OBJECT(value);
>+        }
>+

Nit: use js_CastAsPropertyOp(JSVAL_TO_OBJECT(value)) for the cast to avoid MSVC warnings.

Comment 36

[Brendan Eich [:brendan]]

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

/be

Comment 37

[Robert Sayre]

(In reply to comment #36)
> http://hg.mozilla.org/tracemonkey/rev/e09130fcb013

had to back this out because of crashes on Talos.

http://hg.mozilla.org/tracemonkey/rev/075b80a1cf6d

Comment 38

[Brendan Eich [:brendan]]

I totally misread tryserver's happy mail as "all clear". Wish it had rained fire and brimstone on my Inbox about the talos failures.

/be

Comment 39

[Brendan Eich [:brendan]]

Attachment: refreshed to tm tip

Having trouble reproducing any of the test failures locally.

/be

Comment 40

[Robert Sayre]

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

Comment 41

[Blake Kaplan (:mrbkap) (inactive)]

This was backed out of TM, it isn't fixed.

Comment 42

[Brendan Eich [:brendan]]

Rob, any test repro luck?

/be

Comment 43

[Robert Sayre]

Attachment: refreshed again

Comment 44

[David Mandelin [:dmandelin]]

What's the status on this? I want to try to remove the call to js_GetCurrentBytecodePC for bug 505591, which touches the same function. I don't think there is a semantic conflict, but there would be merge conflicts with the patches.

Comment 45

[Brendan Eich [:brendan]]

I can't repro the failures reported by talos when this last landed (also from tryserver logs) -- very frustrating.

Dave, would you be willing to apply and test a bit? Ping me on IRC and I'll help debug.

/be

Comment 46

[Brendan Eich [:brendan]]

Attachment: fix thinko (trace, as in gc-mark, sprop->methodObject())

Many thanks to sayrer for debugging buddying over irc.

/be

Comment 47

[Robert Sayre]

looks like this has memory footprint problems on the Tp set, still waiting for more results.

Comment 48

[Robert Sayre]

(In reply to comment #47)
> looks like this has memory footprint problems on the Tp set, still waiting for
> more results.

So it passed tests, but there seems to be a leak during Tp.

Comment 49

[Brendan Eich [:brendan]]

Leak, or bloat? Shutdown should be leak free AFAICT, but I can see how over-specialization will bloat the property tree. Will work on a fix for it.

/be

Comment 50

[Robert Sayre]

yeah, could be either bloat or a leak. it was pretty big, though.

Comment 51

[Igor Bukanov]

(In reply to comment #46)

> Created an attachment (id=393022) [details]
> fix thinko (trace, as in gc-mark, sprop->methodObject())

The patch seems fine but I have no idea what causes the leak. One theory could be that for TCF_COMPILE_N_GO scripts the compiler-time created function contains the parent/proto links. With join functions this can lead to some extra rooting that the garbage/cycle collector cannot deal with.

Comment 52

[David Mandelin [:dmandelin]]

Attachment: refreshed to tip

Comment 53

[Brendan Eich [:brendan]]

Attachment: refreshed again

More soon to get this landed, I hope.

/be

Comment 54

[Brendan Eich [:brendan]]

Attachment: refreshed, sayrer thinks this might stick

Comment 55

[Mike Shaver (:shaver emeritus)]

Did it stick?

Comment 56

[Paul Wright]

Well?

Comment 57

[Brendan Eich [:brendan]]

(In reply to comment #56)
> Well?

Hi, I don't know you, but like most people, I don't respond well to one-liners such as "Well?" -- sounds like the stereotypical mother of a good-for-nothing teenager demanding an answer. :-/

I've been busy on other bugs, but I'm back to this one at sayrer's advice and I'll try to land an updated version of this patch tonight.

/be

Comment 58

[Brendan Eich [:brendan]]

Attachment: refreshed again

Testing...

/be

Comment 59

[Brendan Eich [:brendan]]

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

Have to sleep, back me out if something's bad enough. Thanks,

/be

Comment 60

[Brendan Eich [:brendan]]

Backed out:

http://hg.mozilla.org/tracemonkey/rev/0ae65841fcf8

I need to do a "semantic merge" with jorendorff's work tracing getters. That code decompomsed JSScopeProperty::get (formerly js_GetSprop) into its various cases on trace, so I didn't see any conflicts with sprop->get callsites when refreshing this bug's patch.

/be

Comment 61

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

$ cat 748-interesting.js 
try {
    (new Function("__defineSetter__(\"x\",function(){})"))()
} catch(e) {}
this.x = function () {}
$ ./patched-js-dbg-tm-darwin 748-interesting.js 
Assertion failure: !(sprop->attrs & JSPROP_SETTER), at ../jsobj.cpp:4210
Trace/BPT trap

Comment 62

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

(In reply to comment #58)
> Created an attachment (id=396675) [details]
> refreshed again
> 
> Testing...
> 
> /be

(Comment #61 was also from this patch)

for (let a in [0, String(''), String(''), {}]) {
    ({
        a: [0].map(/a/, x = a)
    })
}

Assertion failure: scope->object == obj, at ../jsops.cpp:3426


function f() {
    for (i = 0; i < 2; ++i) {
        for (var j; j < [];) {}
    }
}
f()
__defineGetter__("x", function () {})
this.x = function () {}

Assertion failure: thing, at ../jsgc.cpp:2624

Comment 63

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

(In reply to comment #58)
> Created an attachment (id=396675) [details]
> refreshed again
> 
> Testing...
> 
> /be

try {
    (function () {
        for (e in [/x/, '', /x/, '', /x/]) {
            print(({
                x: [this.__defineGetter__("", [].unwatch)]
            }))
        }
    })()
} catch(e) {}
try {
    (function () {
        for each(e in [String(), this]) {
            with({
                y: undefined
            }) {}
        }
    })()
} catch(e) {}
try {
    (f)
} catch(e) {}
try {
    (function () {
        for (a = 0; a < 1; a++) gc()
    })()
} catch(e) {}
try {
    (function () {
        for (c = 0; c < 1; c++) x
    })()
} catch(e) {}
try {
    (function () {
        with({
            b: void {}. *
        }) i
    })()
} catch(e) {}
try {
    (function () {
        for each(let a in ['', ({})]) {
            print(a)
        }
    })()
} catch(e) {}


Crash [@ js_NativeGet] at null, asserts at Assertion failure: scope->object == obj, at ../jsops.cpp:3426

(Comment is private due to long and complex testcase which might reveal the inner workings of our fuzzing capabilities)

Comment 64

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

(In reply to comment #63)
> (Comment is private due to long and complex testcase which might reveal the
> inner workings of our fuzzing capabilities)

Nope, approved to be opened. :)

Comment 65

[Brendan Eich [:brendan]]

Attachment: had to raise the branded write barrier from LOCKED_OBJ_WRITE_SLOT

This passes tryserver and other tests. Need jorendorff's advice on tracing side.

/be

Comment 66

[Brendan Eich [:brendan]]

Attachment: plain diff of last two attachments

Interdiff fail, understandable given big changes (obj->getClass(), yay!).

/be

Comment 67

[Brendan Eich [:brendan]]

The diff-of-patches reads nicely in bugzilla.

Gary, thanks a ton -- great fuzz tests as always.

/be

Comment 68

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

(In reply to comment #65)
> Created an attachment (id=397221) [details]
> had to raise the branded write barrier from LOCKED_OBJ_WRITE_SLOT

(In reply to comment #67)
> Gary, thanks a ton -- great fuzz tests as always.

Sure - here's another one with your latest patch :)

With -j:

function foo(code) {
    (eval("(function(){for(y=0;y<4;++y)({x:Error()})})"))()
    delete eval
}
foo();
gc();
gc();
(eval("with({}){print}"))()


crashes js opt shell at js_Interpret near null and asserts js dbg shell at Assertion failure: scope->object == obj, at ../jsops.cpp:3474

Comment 69

[Brendan Eich [:brendan]]

Attachment: fix bug shown by comment 68 testcase

I'll wait for bug 512617 to try to fix JSOP_INIT{PROP,ELEM} to insist that the object have its own scope. The interpreter case for JSOP_NEWINIT peeks ahead for JSOP_ENDINIT and if it doesn't see it, gives the object its own scope. But the JIT does not do likewise with the builtin calls it emits (it should).

r?jorendorff to get the ball rolling and find out where in jstracer.cpp (it has changed since the old days, for the better but no longer in my memory) we might need to worry about imposing the method barrier.

/be

Comment 70

[Brendan Eich [:brendan]]

(In reply to comment #65)
> Created an attachment (id=397221) [details]
> had to raise the branded write barrier from LOCKED_OBJ_WRITE_SLOT

To say more about this:

In Firefox 3, Gecko 1.9, SpiderMonkey 1.8, for the property cache to cache function values of "method" properties, I added scope branding. This is done on first call to avoid premature and unpredictable shape assignment as an object (often a prototype) is populated with uncalled-until-later methods.

This requires a write barrier to reshape the branded scope when a method value changes, or despecialize to not cache method values. We don't yet do the latter but we should to avoid pathological behavior on rare cases. But for correcness in the Firefox 3 timeframe I needed pervasive write monitoring, so I put the branded scope method change barrier within the LOCKED_OBJ_SET_SLOT macro (this evolved to LOCKED_OBJ_WRITE_SLOT/LOCKED_OBJ_WRITE_BARRIER).

But this is too low a level to impose the clone-escaping-joined-function-object barrier, the so-called "method barrier" that this bug's patch builds on top of the branded scope barrier (note how METHOD_BARRIER => BRANDED). So the recent and crucial fix to the patch for this bug lifts the barrier to hide behing the getter and setter barriers that already slow down property accesses where the property has a getter and/or setter.

This means extra scope->branded tests in jsobj.c and jsops.cpp, instead of buried in the common LOCKED_OBJ_WRITE_* macro. But the fan-out is not bad and the runtime cost is no worse than before -- the cost-point just moves. See js_Native[GS]et in jsobj.cpp, NATIVE_SET in jsops.cpp, and the JSOP_SETGVAR case in jsops.cpp. Testcase for the last, novel to our testsuites AFAIK:

// A gvar (memoized global slot to avoid name lookup in the interpreter)
// with a function as its value -- the global object is not branded yet.
var g = function () { return 1; }

// A function that calls g as a method of this bound to the global object.
function f() { return this.g(); }

// Call f, which calls this.g, branding the global scope (giving it a fresh
// shape) and caching the origin function value of this.g as the method to
// call from the callsite in f.
f();

// Reset g via its gvar, which must not bypass the method change barrier.
g = function () { return 2; }

// Make sure we went through the barrier.
assertEq(f(), 2);

/be

Comment 71

[Brendan Eich [:brendan]]

(In reply to comment #70)
> ... or despecialize to not cache method values. We don't yet do the latter
> but we should to avoid pathological behavior on rare cases.

Filed at bug 513326.

/be

Comment 72

[Jason Orendorff [:jorendorff]]

I told Brendan I would get to this today (well, Friday) but got less than halfway there. Everything looks good so far. I'll review on the plane tomorrow.

Comment 73

[Brendan Eich [:brendan]]

This patch plus the patch for bug 504587 give the following v8/richards speedup

no patch best of 3 | this bug's patch | bug 504587's patch = 1668 | 1287 | 1121

Joined function objects let us pre-brand with no shape explosion, winning here and helping bug 504587's patch too. Reversing the order of patch applications makes things slower then faster.

/be

Comment 74

[Brendan Eich [:brendan]]

Comment on attachment 397311 [details] [diff] [review]
fix bug shown by comment 68 testcase

Ok, I have my bearings in jstracer.cpp now -- need to emit builtin calls for method read and write barriers. New patch soon.

/be

Comment 75

[Brendan Eich [:brendan]]

Attachment: patch with clearer barriers, trace-JIT builtin method read barrier

Comment 76

[Brendan Eich [:brendan]]

Bugzilla interdiff works. The interdiff shows a bug fix to the method read barrier: it used to set the cloned function object as the value of the id'ed property on the direct obj, not on pobj (the prototype in which the method was found).

/be

Comment 77

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

(In reply to comment #75)
> Created an attachment (id=397581) [details]
> patch with clearer barriers, trace-JIT builtin method read barrier

This patch seems better - no problems found after one night of fuzzing on one machine. :)

Comment 78

[Jason Orendorff [:jorendorff]]

In jsscope.h:
>+    bool hasSetter() const {
>+        return attrs & JSPROP_SETTER;
>+    }

That's an unfortunate name, since native setters don't count. How about
hasSetterObject? Same goes for hasGetter.

>-    if (sprop->attrs & JSPROP_GETTER) {
>+    if (attrs & JSPROP_GETTER) {
>         js_ReportGetterOnlyAssignment(cx);
>         return JS_FALSE;
>     }

return false; while you're passing through?

In jsops.cpp:
>@@ -609,9 +609,10 @@
>     JS_BEGIN_MACRO                                                    \
>         TRACE_2(SetPropHit, entry, sprop);                            \
>         if (SPROP_HAS_STUB_SETTER(sprop) &&                           \
>-            (sprop)->slot != SPROP_INVALID_SLOT) {                    \
>+            (sprop)->slot != SPROP_INVALID_SLOT &&                    \
>+            !OBJ_SCOPE(obj)->branded()) {                             \
>             /* Fast path for, e.g., Object instance properties. */    \
>-            LOCKED_OBJ_WRITE_SLOT(cx, obj, (sprop)->slot, *vp);       \
>+            LOCKED_OBJ_SET_SLOT(obj, (sprop)->slot, *vp);             \
>         } else {                                                      \
>             if (!js_NativeSet(cx, obj, sprop, vp))                    \
>                 goto error;                                           \

IIUC this speeds up SETPROP at the expense of SETNAME for globals,
assuming the global object usually gets branded.

I think you could add an escape route for global names without punishing
the fast case:

          if (SPROP_HAS_STUB_SETTER(sprop) && 
              (sprop)->slot != SPROP_INVALID_SLOT &&
              (!OBJ_SCOPE(obj)->branded() || (!OBJ_SCOPE(obj)->isMethod() &&
                                              !VALUE_IS_FUNCTION(cx, *vp)))) {

Maybe that's too much gunk.


In jsops.cpp:
>             /*
>              * Protect obj from any GC hiding below JSObject::setProperty or
>              * JSObject::defineProperty.  All paths from here must flow through
>-             * the "Restore fp->scopeChain" code below the
>-             * parent->defineProperty call.
>+             * the fp->scopeChain" code below the parent->defineProperty call.
>              */
>             MUST_FLOW_THROUGH("restore_scope");
>             fp->scopeChain = obj;

Stray quotes here.

Comment 79

[Jason Orendorff [:jorendorff]]

>-        if (!js_NativeGet(cx, obj, obj2, sprop, vp))
>+        if (!js_NativeGet(cx, obj, obj2, sprop, JSGET_METHOD_BARRIER, vp))

FWIW it seems like it would be better to have a special flag for JSGET_SKIP_METHOD_BARRIER, and make the safest behavior the default.

Comment 80

[Brendan Eich [:brendan]]

(In reply to comment #78)
> In jsscope.h:
> >+    bool hasSetter() const {
> >+        return attrs & JSPROP_SETTER;
> >+    }
> 
> That's an unfortunate name, since native setters don't count. How about
> hasSetterObject? Same goes for hasGetter.

Done, thanks.

> >-    if (sprop->attrs & JSPROP_GETTER) {
> >+    if (attrs & JSPROP_GETTER) {
> >         js_ReportGetterOnlyAssignment(cx);
> >         return JS_FALSE;
> >     }
> 
> return false; while you're passing through?

Done, and I got rid of the other JS_FALSE in jsscope.h (the actual arg to js_GenerateShape).

> In jsops.cpp:
> >@@ -609,9 +609,10 @@
> >     JS_BEGIN_MACRO                                                    \
> >         TRACE_2(SetPropHit, entry, sprop);                            \
> >         if (SPROP_HAS_STUB_SETTER(sprop) &&                           \
> >-            (sprop)->slot != SPROP_INVALID_SLOT) {                    \
> >+            (sprop)->slot != SPROP_INVALID_SLOT &&                    \
> >+            !OBJ_SCOPE(obj)->branded()) {                             \
> >             /* Fast path for, e.g., Object instance properties. */    \
> >-            LOCKED_OBJ_WRITE_SLOT(cx, obj, (sprop)->slot, *vp);       \
> >+            LOCKED_OBJ_SET_SLOT(obj, (sprop)->slot, *vp);             \
> >         } else {                                                      \
> >             if (!js_NativeSet(cx, obj, sprop, vp))                    \
> >                 goto error;                                           \
> 
> IIUC this speeds up SETPROP at the expense of SETNAME for globals,
> assuming the global object usually gets branded.

This is a zero-net change, since LOCKED_OBJ_WRITE_SLOT used to start with

        JSScope *scope_ = OBJ_SCOPE(obj);                                     \
        JS_ASSERT(scope_->object == obj);                                     \
        if (scope_->branded()) {                                              \

But yeah, "too much gunk". Globals already have the JSOP_*GVAR optimization, which predates the property cache and still exists because it wins in pure interpreted performance by quite a bit (I haven't measured lately, though).

If we get the property cache fast enough, the GVAR ops could go away. But my patch did not regress NATIVE_SET for JSOP_SETNAME from the status quo.

Thanks for the review, new patch in a few minutes.

/be

Comment 81

[Brendan Eich [:brendan]]

Attachment: patch updated to address jorendorff's comments

Comment 82

[Brendan Eich [:brendan]]

Bugzilla interdiff whines but does the right thing.

/be

Comment 83

[Brendan Eich [:brendan]]

Attachment: oops, forgot to finish the JSGET_{,NO_}METHOD_BARRIER change

For clarity of all callsites (no opaque 0 args) I kept JSGET_METHOD_BARRIER but defined it as 0. This left the test !(getHow & JSGET_METHOD_BARRIER) horked!

/be

Comment 84

[Jason Orendorff [:jorendorff]]

>+             * So here, on first cache fill for this method, we brand
>+             * the scope with a new shape and set the SCOPE_BRANDED
>+             * flag. Once this scope flag is set, any write that adds
>+             * or deletes a function-valued plain old property in
>+             * scope->object will result in shape being regenerated.

Is this wording better than before? Adding or deleting *any* property will change the shape. What exactly is the important property here?

Comment 85

[Brendan Eich [:brendan]]

That's an old comment. It should talk about reassigning existing method values changing the shape. Obviously shape is not value-sensitive otherwise (yet...).
'
/be

Comment 86

[Brendan Eich [:brendan]]

diff -u b/js/src/jsinterp.cpp b/js/src/jsinterp.cpp
--- b/js/src/jsinterp.cpp
+++ b/js/src/jsinterp.cpp
@@ -213,10 +213,10 @@
                      * values, however.
                      *
                      * So here, on first cache fill for this method, we brand
-                     * the scope with a new shape and set the SCOPE_BRANDED
-                     * flag. Once this scope flag is set, any write that adds
-                     * or deletes a function-valued plain old property in
-                     * scope->object will result in shape being regenerated.
+                     * the scope with a new shape and set the JSScope::BRANDED
+                     * flag. Once this flag is set, any property assignment
+                     * that changes the value from or to a different function
+                     * object will result in shape being regenerated.
                      */
                     if (!scope->branded()) {
                         PCMETER(cache->brandfills++);

/be

Comment 87

[Julian Seward [:jseward]]

Patch on comment #83 appears to cause a major performance regression
on v8-richards.  32099:c1a97865c476 requires 3,735,552,706 insns
(gcc -m32 -O2).  32099:c1a97865c476 + #83 requires 4,942,951,214
insns.

User time for 10 runs on this box increases from 8.7s to 12.6s.

Comment 88

[Julian Seward [:jseward]]

(In reply to comment #87)
The number of frag entries for v8-richards increases from 11836222
to 25024452.  Seems like the hottest trace got converted into a set
of five, all of which bounce around.  Before, the hottest trace 
has this:

FragID=000002, total count 3734848:
   GuardID=019     194600 ( 5.21%)
   GuardID=025    2300550 (61.60%)
   Looped (050)   1239698 (33.19%)

and the next hottest is not so hot.  Afterwards, there's a group of
5 all of which have 3734848 entries (more or less); most of them
exit very early.

FragID=000002, total count 3734848:
   GuardID=013    3734847 (100.00%)
   GuardID=025          1 ( 0.00%)
   Looped (050)         0 ( 0.00%)

FragID=000004, total count 3734844:
   GuardID=005    3734839 (100.00%)
   GuardID=017          4 ( 0.00%)
   GuardID=041          1 ( 0.00%)
   Looped (042)         0 ( 0.00%)

FragID=000010, total count 3734837:
   GuardID=005    3734815 (100.00%)
   GuardID=011          2 ( 0.00%)
   GuardID=017          8 ( 0.00%)
   GuardID=051          9 ( 0.00%)
   GuardID=084          2 ( 0.00%)
   GuardID=152          1 ( 0.00%)
   Looped (153)         0 ( 0.00%)

FragID=000019, total count 3734813:
   GuardID=005    3734812 (100.00%)
   GuardID=051          1 ( 0.00%)
   Looped (120)         0 ( 0.00%)

FragID=000021, total count 3734810:
   GuardID=011     194597 ( 5.21%)
   GuardID=017    1239688 (33.19%)
   GuardID=051    1486439 (39.80%)
   GuardID=063     296094 ( 7.93%)
   GuardID=084      30098 ( 0.81%)
   GuardID=087      81549 ( 2.18%)
   GuardID=092      81898 ( 2.19%)
   GuardID=103      30098 ( 0.81%)
   GuardID=115      21000 ( 0.56%)
   GuardID=291     273349 ( 7.32%)
   Looped (292)         0 ( 0.00%)

Comment 89

[Brendan Eich [:brendan]]

Julian, can you say which patch regressed richards? IIRC you had tried an earlier patch for this bug under a patch for bug 504587 and not seen this regression.

/be

Comment 90

[Brendan Eich [:brendan]]

Comment on attachment 397759 [details] [diff] [review]
oops, forgot to finish the JSGET_{,NO_}METHOD_BARRIER change

>+                    if (op == JSOP_SETMETHOD) {
>+#ifdef DEBUG
>+                        op2 = JSOp(regs.pc[JSOP_LAMBDA_LENGTH + JSOP_SETMETHOD_LENGTH]);
>+                        JS_ASSERT(op2 == JSOP_POP || op2 == JSOP_POPV);
>+#endif
>+
>+                        if (JSVAL_IS_OBJECT(lval) &&
>+                            (obj2 = JSVAL_TO_OBJECT(lval)) &&
>+                            OBJ_GET_CLASS(cx, obj2) == &js_ObjectClass) {
>+                            /*
>+                             * Ensure we have our own scope to avoid falling
>+                             * off the do_setprop fast path.
>+                             */
>+                            JS_LOCK_OBJ(cx, obj2);
>+                            scope = OBJ_SCOPE(obj2);
>+                            if (scope->object != obj2) {
>+                                scope = js_GetMutableScope(cx, obj2);
>+                                if (!scope) {
>+                                    JS_UNLOCK_OBJ(cx, obj2);
>+                                    goto error;
>+                                }
>+                            }
>+                            JS_UNLOCK_SCOPE(cx, scope);

This is not needed, the no-owned-scope path below do_setprop is fast enough. I'm removing it.

>+                            PUSH_OPND(OBJECT_TO_JSVAL(obj));
>+                            regs.pc += JSOP_LAMBDA_LENGTH;
>+                            goto do_setprop;
>+                        }
>+                    } else if (op == JSOP_INITMETHOD) {
>+                        JS_ASSERT(!JSVAL_IS_PRIMITIVE(lval));
>+                        obj2 = JSVAL_TO_OBJECT(lval);
>+                        scope = OBJ_SCOPE(obj2);
>+
>+                        /*
>+                         * JSOP_NEWINIT gave the new object it created (obj2
>+                         * here) its own scope.
>+                         */
>+                        JS_ASSERT(scope->object == obj2);

And I'm removing the scope temp, but adding an assertion that obj2 is of "Object" class.

Sorry for this noise.

/be

Comment 91

[Julian Seward [:jseward]]

(In reply to comment #89)
> Julian, can you say which patch regressed richards? IIRC you had tried an
> earlier patch for this bug under a patch for bug 504587 and not seen this
> regression.

The numbers above are unrelated to 504587.  I first tried out the
504587 + this bug's patch, found a large regression, and tried this
patch by itself.  IOW it's this patch.

Comment 92

[Brendan Eich [:brendan]]

Understood, but I think a recent version of this bug's patch introduced a regression -- if attachment 397581 [details] [diff] [review] shows the same regression but attachment 397311 [details] [diff] [review] does not, and the latter actually speeds up richards, then I will dance a jig.

Thanks for the fragment profiling help,

/be

Comment 93

[Julian Seward [:jseward]]

(In reply to comment #92)
No difference between them, afaict.  What I have is:

baseline        I refs:      3,735,553,034
R471214_397311  I refs:      3,735,403,060
R471214_397581  I refs:      3,735,381,751

assuming I didn't fluff the patchery.  So it must be 397752 or 397759.

Comment 94

[Jason Orendorff [:jorendorff]]

This affects the order of properties.

obj = {a: function () 0, b: 1};
assertEq(""+[p for (p in obj)], "a,b");  // passes
var f = obj.a;
assertEq(""+[p for (p in obj)], "a,b");  // fails: got "b,a", expected "a,b"

Comment 95

[Jason Orendorff [:jorendorff]]

In js_FillPropertyCache:
>-        if ((cs->format & JOF_CALLOP) &&
>+        if (cs->format & JOF_CALLOP) {
>+            jsval v;
>+            if (sprop->isMethod()) {
>+                [...]
>+                break;
>+            }
>+
>-            SPROP_HAS_STUB_GETTER(sprop) &&
>-            SPROP_HAS_VALID_SLOT(sprop, scope)) {
>+            if (SPROP_HAS_STUB_GETTER_OR_IS_METHOD(sprop) &&
>+                SPROP_HAS_VALID_SLOT(sprop, scope)) {

The change adding _OR_IS_METHOD here (on the last-but-one quoted line) is misleading. If it's a method, we don't get there.

Also in js_FillPropertyCache:
>         /* If getting a value via a stub getter, we can cache the slot. */
>         if (!(cs->format & (JOF_SET | JOF_INCDEC | JOF_FOR)) &&
>-            SPROP_HAS_STUB_GETTER(sprop) &&
>+            SPROP_HAS_STUB_GETTER_OR_IS_METHOD(sprop) &&
>             SPROP_HAS_VALID_SLOT(sprop, scope)) {
>             /* Great, let's cache sprop's slot and use it on cache hit. */
>             vword = SLOT_TO_PCVAL(sprop->slot);

This is a bug. We can't cache the that slot if sprop->isMethod(). It
would skip the read barrier.

function C() {
    this.f = function () {};
}
var a = [new C, new C];
var f;
for each (var obj in a)
    f = obj.f;
assertEq(f, a[1].f);

Comment 96

[Jason Orendorff [:jorendorff]]

In jsinterp.cpp:
@@ -991,7 +1007,7 @@ js_OnUnknownMethod(JSContext *cx, jsval 
     id = ATOM_TO_JSID(cx->runtime->atomState.noSuchMethodAtom);
-    ok = js_GetMethod(cx, obj, id, false, &tvr.u.value);
+    ok = js_GetMethod(cx, obj, id, 0, &tvr.u.value);

Do you want JSGET_NO_METHOD_BARRIER here?

In jsiter.cpp:
>@@ -374,7 +374,7 @@ js_ValueToIterator(JSContext *cx, uintN 
>-        if (!js_GetMethod(cx, obj, ATOM_TO_JSID(atom), false, vp))
>+        if (!js_GetMethod(cx, obj, ATOM_TO_JSID(atom), 0, vp))

And here?

In jsobj.cpp:
@@ -5493,7 +5554,7 @@ js_TryMethod(JSContext *cx, JSObject *ob
-    ok = js_GetMethod(cx, obj, id, false, &fval);
+    ok = js_GetMethod(cx, obj, id, 0, &fval);

And here?

In jsops.cpp:
>             id = ATOM_TO_JSID(atom);
>             PUSH(JSVAL_NULL);
>             if (!JSVAL_IS_PRIMITIVE(lval)) {
>-                if (!js_GetMethod(cx, obj, id, !!entry, &rval))
>+                if (!js_GetMethod(cx, obj, id, entry ? JSGET_CACHE_RESULT : 0, &rval))
>                     goto error;
>                 STORE_OPND(-1, OBJECT_TO_JSVAL(obj));
>                 STORE_OPND(-2, rval);
>             } else {
>                 JS_ASSERT(obj->map->ops->getProperty == js_GetProperty);
>-                if (!js_GetPropertyHelper(cx, obj, id, true, &rval))
>-                    goto error;
>+                if (!js_GetPropertyHelper(cx, obj, id,
>+                                          JSGET_CACHE_RESULT | JSGET_NO_METHOD_BARRIER,
>+                                          &rval)) {
>+                    goto error;
>+                }

Why NO_METHOD_BARRIER for the second case and not the first?

Comment 97

[Brendan Eich [:brendan]]

(In reply to comment #93)
> (In reply to comment #92)
> No difference between them, afaict.  What I have is:
> 
> baseline        I refs:      3,735,553,034
> R471214_397311  I refs:      3,735,403,060
> R471214_397581  I refs:      3,735,381,751
> 
> assuming I didn't fluff the patchery.  So it must be 397752 or 397759.

Huge help, jorendorff just spotted the problem: I failed to update js_GetMethod to pass the right NO_METHOD_BARRIER jazz. Thanks.

(In reply to comment #94)
> This affects the order of properties.
> 
> obj = {a: function () 0, b: 1};
> assertEq(""+[p for (p in obj)], "a,b");  // passes
> var f = obj.a;
> assertEq(""+[p for (p in obj)], "a,b");  // fails: got "b,a", expected "a,b"

Old property tree bug, never filed but commented upon near the bottom of JSScope::add, formerly biting only in case of overwrite error but now easy to provoke. Marginal, but worth fixing for sure; I'd like to fix later.

I'll fix the great catches from comment 95 and comment 96 now and attach a new patch shortly. Thanks again.

/be

Comment 98

[Brendan Eich [:brendan]]

Attachment: this should do it

Julian, please do feel free to confirm the fix! :-)

/be

Comment 99

[Brendan Eich [:brendan]]

Bugzilla interdiff works well until the bottom, where it gets confused by something in the jstracer.cpp hunks.

/be

Comment 100

[Julian Seward [:jseward]]

(In reply to comment #98)
> Created an attachment (id=397899) [details]
> this should do it

Looks good to me:
* for richards, no perf regression wrt baseline (no improvement either)
* for richards, no change in the frag-level profiles (generated code
  is identical, I'd guess)

Comment 101

[Brendan Eich [:brendan]]

Attachment: hack-fix to pass two regressed trace-tests

Any imacro or better self-hosted implementation that gets a method may force an unnecessary clone. Let's avoid it for now by testing fp->imacpc.

/be

Comment 102

[Brendan Eich [:brendan]]

Long-term, we will probably want more static analysis in the compiler to decide which gets might actually leak.

/be

Comment 103

[Jason Orendorff [:jorendorff]]

Apologies if I bring up points that have already been fixed-- I'm reading and testing attachment 397759 [details] [diff] [review].

I think JSScope::methodShapeChange() turns function-valued properties into methods too aggressively. The value of a method must be a funobj that was never exposed to script, because we will clone it in the read barrier. If it was exposed before, you can detect the cloning:

function f() {}
var obj = {a: function(){}};
obj.f = 3;
obj.f = f;  // creates a method
assertEq(obj.f, f);

Comment 104

[Jason Orendorff [:jorendorff]]

These comments refer to attachment 397759 [details] [diff] [review].

Why are methods slotful? Since they need both a read barrier and a write
barrier they are not much like plain old properties.  (We discussed this
on IRC, but I have already forgotten the answer, and it might as well be
in this bug.)

Correct me: This patch makes no change to the meaning of shape. Certain
properties that were totally plain before are now (with the patch)
fairly magical, but shape still covers everything-except-the-slot-values
as before.

>@@ -2553,7 +2569,7 @@ AssertValidPropertyCacheHit(JSContext *c
>         JS_ASSERT(OBJ_SCOPE(pobj)->branded());
>-        JS_ASSERT(SPROP_HAS_STUB_GETTER(sprop));
>+        JS_ASSERT(SPROP_HAS_STUB_GETTER_OR_IS_METHOD(sprop));
>         JS_ASSERT(SPROP_HAS_VALID_SLOT(sprop, OBJ_SCOPE(pobj)));

If it's a method we can additionally assert that we're executing a
JOF_CALL opcode and that the Function object is the right one.

More importantly, in the other cases here we should assert that sprop is
*not* a method.

In jsobj.cpp, js_DefineNativeProperty:
>         /* Add a new property, or replace an existing one of the same id. */
>         if (clasp->flags & JSCLASS_SHARE_ALL_PROPERTIES)
>             attrs |= JSPROP_SHARED;
>+
>+        if (defineHow & JSDNP_SET_METHOD) {
>+            JS_ASSERT(obj->getClass() == &js_ObjectClass);
>+            JS_ASSERT(VALUE_IS_FUNCTION(cx, value));
>+            JS_ASSERT(!(attrs & (JSPROP_GETTER | JSPROP_SETTER)));
>+            JS_ASSERT(!getter && !setter);
>+
>+            JSObject *funobj = JSVAL_TO_OBJECT(value);
>+            if (FUN_OBJECT(GET_FUNCTION_PRIVATE(cx, funobj)) == funobj) {

Can the condition on the last line be false? Strengthen it to an assertion?

Can we also assert the relevant details about the function itself here?
It must not escape (i.e. it must not use JSOP_ARGUMENTS, its own name,
or eval). Anything else?

>+                flags |= SPROP_IS_METHOD;
>+                getter = js_CastAsPropertyOp(JSVAL_TO_OBJECT(value));
>+            }

Micro-nit: JSVAL_TO_OBJECT(value) -> funobj

(This immediately follows the lines above where funobj = JSVAL_TO_OBJECT(value).)

In jsscope.h, methodShapeChange:
>+    JS_ASSERT(!sprop->setter || sprop->setter == js_watch_set);
>+#endif
>+
>+    if (hasMethodBarrier()) {
>+        JSPropertyOp getter = NULL;
>+        uintN flags = sprop->flags;
>+
>+        if (VALUE_IS_FUNCTION(cx, toval)) {
>+            getter = js_CastAsPropertyOp(JSVAL_TO_OBJECT(toval));
>+            flags |= SPROP_IS_METHOD;
>+        }
>+
>+        sprop = add(cx, sprop->id, getter, NULL, sprop->slot, sprop->attrs,
>+                    flags, sprop->shortid);

sprop->setter instead of NULL, to avoid removing a watchpoint?

More generally, I didn't think about how this interacts with
watchpoints. It might be better to avoid the issue by making
JS_SetWatchPoint devolve a method-property into a plain old property.

Comment 105

[Brendan Eich [:brendan]]

(In reply to comment #103)
> Apologies if I bring up points that have already been fixed-- I'm reading and
> testing attachment 397759 [details] [diff] [review].
> 
> I think JSScope::methodShapeChange() turns function-valued properties into
> methods too aggressively. The value of a method must be a funobj that was never
> exposed to script, because we will clone it in the read barrier. If it was
> exposed before, you can detect the cloning:
> 
> function f() {}
> var obj = {a: function(){}};
> obj.f = 3;
> obj.f = f;  // creates a method

No, only assigning or initializing from a null closure lambda with compile to runtime invariant parent (global object) sets SPROP_IS_METHOD.

> assertEq(obj.f, f);

What was o.a for? Confused. If you called o.a to brand o's scope, then the obj.f = f; assignment would wrongly reshape. That's old paranoia from Firefox 3 daze, as noted on IRC. I fix...

/be

Comment 106

[Jason Orendorff [:jorendorff]]

jsopcode.tbl:
>+OPDEF(JSOP_INITMETHOD,    236,"initprop",      [...]

The name is wrong!

Comment 107

[Brendan Eich [:brendan]]

Re: comment 105, unconfused now, it's a bug in JSScope::methodWriteBarrier that bites in conjunction with the unnecessary reshaping done when changing from non-function to function value in a branded scope (the old Firefox 3 paranoia I mentioned). New patch shortly.

/be

Comment 108

[Brendan Eich [:brendan]]

(In reply to comment #104)
> These comments refer to attachment 397759 [details] [diff] [review].
> 
> Why are methods slotful? Since they need both a read barrier and a write
> barrier they are not much like plain old properties.  (We discussed this
> on IRC, but I have already forgotten the answer, and it might as well be
> in this bug.)

js_NativeGet has a fast path for slotful properties. Once you get past that and notice the lack of the method barrier (JSGET_NO_METHOD_BARRIER), you already have the method value. So there's no win apart from the assertion in loading sprop->methodValue().

You might ask whether we need the js_NativeGet fast path when jsops.cpp uses NATIVE_GET to get a truly fast path to slotful properties. array_getProperty and js_GetPropertyHelper still benefit, but it's a good question. Still, however we slice it, the generic decision tree goes something like this

  if (slotful) read slot value;
  if (stub getter) done;
  if (method and no read barrier) done;

Without static analysis I don't see how to split things to an

  if (sprop->isMethod()) return sprop->methodValue();

doesn't add cost for the non-method case.

> Correct me: This patch makes no change to the meaning of shape.

True! but...

> Certain
> properties that were totally plain before are now (with the patch)
> fairly magical, but shape still covers everything-except-the-slot-values
> as before.

Not quite: shape still covers function values in branded scopes, as before. The method barrier implies branded.

> >@@ -2553,7 +2569,7 @@ AssertValidPropertyCacheHit(JSContext *c
> >         JS_ASSERT(OBJ_SCOPE(pobj)->branded());
> >-        JS_ASSERT(SPROP_HAS_STUB_GETTER(sprop));
> >+        JS_ASSERT(SPROP_HAS_STUB_GETTER_OR_IS_METHOD(sprop));
> >         JS_ASSERT(SPROP_HAS_VALID_SLOT(sprop, OBJ_SCOPE(pobj)));
> 
> If it's a method we can additionally assert that we're executing a
> JOF_CALL opcode and that the Function object is the right one.
> 
> More importantly, in the other cases here we should assert that sprop is
> *not* a method.

Indeed -- will do.

> In jsobj.cpp, js_DefineNativeProperty:
> >         /* Add a new property, or replace an existing one of the same id. */
> >         if (clasp->flags & JSCLASS_SHARE_ALL_PROPERTIES)
> >             attrs |= JSPROP_SHARED;
> >+
> >+        if (defineHow & JSDNP_SET_METHOD) {
> >+            JS_ASSERT(obj->getClass() == &js_ObjectClass);
> >+            JS_ASSERT(VALUE_IS_FUNCTION(cx, value));
> >+            JS_ASSERT(!(attrs & (JSPROP_GETTER | JSPROP_SETTER)));
> >+            JS_ASSERT(!getter && !setter);
> >+
> >+            JSObject *funobj = JSVAL_TO_OBJECT(value);
> >+            if (FUN_OBJECT(GET_FUNCTION_PRIVATE(cx, funobj)) == funobj) {
> 
> Can the condition on the last line be false?

Yes. I didn't burden jsops.cpp with inter-opcode-case flags to convey that the JSOP_SETMETHOD following a JSOP_LAMBDA had been dispatched by goto do_setprop. If the optimization does not apply (the lambda wasn't a null closure and the base object wasn't of Object class) then JSDNP_SET_METHOD would still be set in the defineHow flags.

Rather than torture Sully (or you and dmandelin, now) with jsops.cpp inter-case hacks I decided it was better to retest here.

> Can we also assert the relevant details about the function itself here?
> It must not escape (i.e. it must not use JSOP_ARGUMENTS, its own name,
> or eval).

Sure.

> Anything else?

That's enough for now.

> >+                flags |= SPROP_IS_METHOD;
> >+                getter = js_CastAsPropertyOp(JSVAL_TO_OBJECT(value));
> >+            }
> 
> Micro-nit: JSVAL_TO_OBJECT(value) -> funobj

Thanks.

> In jsscope.h, methodShapeChange:
> >+    JS_ASSERT(!sprop->setter || sprop->setter == js_watch_set);
> >+#endif
> >+
> >+    if (hasMethodBarrier()) {
> >+        JSPropertyOp getter = NULL;
> >+        uintN flags = sprop->flags;
> >+
> >+        if (VALUE_IS_FUNCTION(cx, toval)) {
> >+            getter = js_CastAsPropertyOp(JSVAL_TO_OBJECT(toval));
> >+            flags |= SPROP_IS_METHOD;
> >+        }
> >+
> >+        sprop = add(cx, sprop->id, getter, NULL, sprop->slot, sprop->attrs,
> >+                    flags, sprop->shortid);
> 
> sprop->setter instead of NULL, to avoid removing a watchpoint?

Gah.

> More generally, I didn't think about how this interacts with
> watchpoints. It might be better to avoid the issue by making
> JS_SetWatchPoint devolve a method-property into a plain old property.

Will investigate.

Molto grazie.

/be

Comment 109

[Brendan Eich [:brendan]]

Molte grazie, even -- grazie mille let's say (I haven't been in Italy in a long while).

/be

Comment 110

[Brendan Eich [:brendan]]

It was unclear to Jason, therefore to the rest of humanity, that JSOP_LAMBDA;JSOP_{SET,INIT}METHOD sequences are emitted by the compiler even when it could (with more effort) prove the combo can't be optimized to set a method in the SPROP_IS_METHOD, property-tree-memoizes-method-value sense.

The problems include ordering closure optimization and instruction selection, and determining the type of the receiver object in the case of JSOP_SETMETHOD. For JSOP_INITMETHOD we know more, but I didn't reorder instruction selection since we'll have to do runtime checking for JSOP_SETMETHOD anyway. Bug me if you think this is worth addressing some day.

/be

Comment 111

[Brendan Eich [:brendan]]

(In reply to comment #104)
> More importantly, in the other cases here we should assert that sprop is
> *not* a method.

Can't do that in AssertValidPropertyCacheHit if PCVAL_IS_SPROP, could be a method extraction. But we can assert that if sprop->isMethod(), the method value == the slot value (no barrier), else the slot value is a clone.

> Can we also assert the relevant details about the function itself here?
> It must not escape (i.e. it must not use JSOP_ARGUMENTS, its own name,
> or eval).

I said "Sure." but the compiler does not record TCF_FUN_USES_ARGUMENTS in any function flag (there's room in the low three bits). I'd like to defer that to a later bug. The compiler's judgments about when not to optimize are easy to check and the runtime need not assert them. If instruction selection were any hairier, it might be worth doing this.

As things stand the likelier worry is the runtime checking not being correct in some way that the compiler can't check, not the runtime code getting a lambda that the compiler shouldn't have optimized. But both kinds of bugs could happen for sure. I do want JSFUN_USES_ARGUMENTS at some point...

/be

Comment 112

[Jason Orendorff [:jorendorff]]

The same optimization could help local functions that happen to be null
closures. Seems like an easy follow-up bug.

  function f() {
      function g() {
          // ...well-behaved null closure...
      }

      g();  // is called, but doesn't escape
  }

Important for the same reason: we could trace calls to g on trace
better. And easier to implement, since g can't escape.  It's just a
shortcut in JSOP_DEFLOCALFUN and the corresponding place in the JIT.

In jsops.cpp, NATIVE_SET:
>         if (SPROP_HAS_STUB_SETTER(sprop) &&                           \
>-            (sprop)->slot != SPROP_INVALID_SLOT) {                    \
>+            (sprop)->slot != SPROP_INVALID_SLOT &&                    \
>+            !OBJ_SCOPE(obj)->branded()) {                             \
>             /* Fast path for, e.g., Object instance properties. */    \
>-            LOCKED_OBJ_WRITE_SLOT(cx, obj, (sprop)->slot, *vp);       \
>+            LOCKED_OBJ_SET_SLOT(obj, (sprop)->slot, *vp);             \
>         } else {                                                      \
>-            if (!js_NativeSet(cx, obj, sprop, vp))                    \
>+            if (!js_NativeSet(cx, obj, sprop, false, vp))             \
>                 goto error;                                           \
>         }                                                             \

It looks like this change punishes ordinary assignment to undeclared
global variables when the global object is branded. Is it worth adding a
"fast slow path"?

In case JSOP_LAMBDA:
>             obj = FUN_OBJECT(fun);
> 
>             if (FUN_NULL_CLOSURE(fun)) {
>-                obj = js_CloneFunctionObject(cx, fun, fp->scopeChain);
>-                if (!obj)
>-                    goto error;
>+                parent = fp->scopeChain;
>+
>+                if (OBJ_GET_PARENT(cx, obj) == parent) {

This last condition is true in all the cases we want to optimize, right?

It would be false for code compiled without COMPILE_N_GO, I think. Do we
care?

>+                    lval = FETCH_OPND(-1);

Nit: In cases where the next op is *not* SETMETHOD or INITMETHOD, the
stack can be empty here, so this isn't reading a valid stack location.

Comment 113

[Brendan Eich [:brendan]]

(In reply to comment #112)
> The same optimization could help local functions that happen to be null
> closures. Seems like an easy follow-up bug.
> 
>   function f() {
>       function g() {
>           // ...well-behaved null closure...
>       }
> 
>       g();  // is called, but doesn't escape
>   }
> 
> Important for the same reason: we could trace calls to g on trace
> better. And easier to implement, since g can't escape.  It's just a
> shortcut in JSOP_DEFLOCALFUN and the corresponding place in the JIT.

Good ideas -- please do file bugs, or ping me and I'll file.

> In jsops.cpp, NATIVE_SET:
> >         if (SPROP_HAS_STUB_SETTER(sprop) &&                           \
> >-            (sprop)->slot != SPROP_INVALID_SLOT) {                    \
> >+            (sprop)->slot != SPROP_INVALID_SLOT &&                    \
> >+            !OBJ_SCOPE(obj)->branded()) {                             \
> >             /* Fast path for, e.g., Object instance properties. */    \
> >-            LOCKED_OBJ_WRITE_SLOT(cx, obj, (sprop)->slot, *vp);       \
> >+            LOCKED_OBJ_SET_SLOT(obj, (sprop)->slot, *vp);             \
> >         } else {                                                      \
> >-            if (!js_NativeSet(cx, obj, sprop, vp))                    \
> >+            if (!js_NativeSet(cx, obj, sprop, false, vp))             \
> >                 goto error;                                           \
> >         }                                                             \
> 
> It looks like this change punishes ordinary assignment to undeclared
> global variables when the global object is branded. Is it worth adding a
> "fast slow path"?

This is the same as the LOCKED_OBJ_WRITE_SLOT code (status quo pre-patch). See my "zero-net change" in comment 80.

> In case JSOP_LAMBDA:
> >             obj = FUN_OBJECT(fun);
> > 
> >             if (FUN_NULL_CLOSURE(fun)) {
> >-                obj = js_CloneFunctionObject(cx, fun, fp->scopeChain);
> >-                if (!obj)
> >-                    goto error;
> >+                parent = fp->scopeChain;
> >+
> >+                if (OBJ_GET_PARENT(cx, obj) == parent) {
> 
> This last condition is true in all the cases we want to optimize, right?

Right.

> It would be false for code compiled without COMPILE_N_GO, I think. Do we
> care?

We have to for safety. COMPILE_N_GO is used for <script> tag content on the Web and XUL brutal sharing, also IIRC for XBL. Oh, and for that minor function known as eval ;-).

> >+                    lval = FETCH_OPND(-1);
> 
> Nit: In cases where the next op is *not* SETMETHOD or INITMETHOD, the
> stack can be empty here, so this isn't reading a valid stack location.

Ugh, hoisted that load a bit too high. Thanks.

/be

Comment 114

[Jason Orendorff [:jorendorff]]

In jsscope.h, JSScope::methodReadBarrier:
>+    JS_ASSERT(sprop->isMethod());
>+
>+    JSObject *funobj = JSVAL_TO_OBJECT(*vp);
>+    JSFunction *fun = GET_FUNCTION_PRIVATE(cx, funobj);
>+
>+    if (FUN_OBJECT(fun) == funobj && FUN_NULL_CLOSURE(fun)) {

As discussed on IRC, *this* check surely can be an assertion...

In jsobj.cpp, js_DefaultValue:
>             if (sprop &&
>-                SPROP_HAS_STUB_GETTER(sprop) &&
>+                SPROP_HAS_STUB_GETTER_OR_IS_METHOD(sprop) &&
>                 SPROP_HAS_VALID_SLOT(sprop, scope)) {

sprop can't be a method here, since we already checked that pobj's class
is JSString.  (In context, we're checking that the property in question
is the original String.prototype.toString.)

In js_DefineNativeProperty:
>+        if ((defineHow & JSDNP_SET_METHOD) &&
>+            obj->getClass() == &js_ObjectClass) {

That fits on one line.

Also in js_DefineNativeProperty:
>+                getter = JS_EXTENSION (JSPropertyOp) JSVAL_TO_OBJECT(*vp);

js_CastAsPropertyOp?

Comment 115

[Brendan Eich [:brendan]]

Attachment: address latest comments, plus IRC golden comment about methodReadBarrier redundant test

Pure gold I say!

/be

Comment 116

[Brendan Eich [:brendan]]

Attachment: incorporate missed feedback from comment 114

Some of comment 114 after the golden comment was based on old code, already fixed. I took the opportunity to use js_CastAsPropertyOp throughout jsops.cpp.

/be

Comment 117

[Brendan Eich [:brendan]]

Attachment: important fix to JSScope::methodShapeChange

This is obvious in hindsight. XBL has JS-implemented methods, they are null closures defined in object initialisers. One such is QueryInterface on the _observer field in findbar.xml. Alas XPConnect wants to QI, so it digs out the value using JS_GetPropertyById, which runs the method barrier.

We clone. We write back. JSScope::methodWriteBarrier calls methodShapeChange and the latter for some silly reason tries to define a method (SPROP_IS_METHOD sense) but the value written back is the clone. We then botch the "golden" assertion on the next get. D'oh!

This patch despecializes on every method shape change of a specialized (let's call these proptree-memoized methods) method.

For later is getting XPConnect to use a better API that bypasses the barrier.

/be

Comment 118

[Brendan Eich [:brendan]]

Attachment: combine if (sprop->isMethod()) DEBUG and non-DEBUG statements

Calling it a night for now. Try server is munching on last patch.

/be

Comment 119

[Brendan Eich [:brendan]]

Try server success. Will consult with mrbkap on better XPConnect callee getting (it really shouldn't run the method read barrier).

/be

Comment 120

[Jason Orendorff [:jorendorff]]

TraceRecorder::isValidSlot:
>     /* This check applies even when setflags == 0. */
>-    if (setflags != JOF_SET && !SPROP_HAS_STUB_GETTER(sprop))
>+    if (setflags != JOF_SET && !SPROP_HAS_STUB_GETTER_OR_IS_METHOD(sprop))
>         ABORT_TRACE_RV("non-stub getter", false);

Both callers of isValidSlot assume that a "valid slot" means one they
can just read from.  Not a bug, because both callers are looking at the
scope chain, which never contains any objects of class js_ObjectClass;
but I still think _OR_IS_METHOD is misleading here.

TraceRecorder::callProp:
>-            js_GetPropertyHelper(cx, obj, sprop->id, JS_FALSE, &nr.v);
>+            js_GetPropertyHelper(cx, obj, sprop->id,
>+                                 (op == JSOP_CALLNAME)
>+                                 ? JSGET_NO_METHOD_BARRIER
>+                                 : JSGET_METHOD_BARRIER,
>+                                 &nr.v);

obj has js_CallClass, so it cannot have methods.

Actually obj always == obj2 here. I'm puzzled as to why callProp has
both parameters.

TraceRecorder::prop:
>    LIns* v_ins = unbox_jsval(STOBJ_GET_SLOT(obj, slot),
>                              stobj_get_slot(obj_ins, slot, dslots_ins),
>                              snapshot(BRANCH_EXIT));
>
>    /*
>     * Joined function object stored as a method must be cloned when extracted
>     * as a property value other than a callee. Note that shapes cover method
>     * value as well as other property attributes and order, so this condition
>     * is trace-invariant.
>     */
>    if (isMethod) {
>        LIns* args[] = { v_ins, INS_CONSTSPROP(sprop), obj_ins, cx_ins };
>        v_ins = lir->insCall(&MethodReadBarrier_ci, args);
>    }

In this case we can skip stobj_get_slot and unbox_jsval, since the
result must be sprop->methodObject(), which is trace-invariant.

This call will reshape obj, but I don't think the patch in bug 504587
needs to be updated (to purge any cached shape guards on obj-- the
change is unavoidable, so the bogus cached shape guard will never be
used again anyway). Mentioning this in case I'm wrong.

Comment 121

[Jason Orendorff [:jorendorff]]

In jsapi.cpp, JS_CallFunctionName calls JS_GetMethodById. It should instead call js_GetMethod passing JSGET_NO_METHOD_BARRIER.

Brendan, is this enough for XPConnect, or do we still need a follow-up bug?

Comment 122

[Jason Orendorff [:jorendorff]]

It's not enough. Filed follow-up bug 514186.

Comment 123

[Jason Orendorff [:jorendorff]]

These comments refer to attachment 397759 [details] [diff] [review].

SETMETHOD and INITMETHOD do not trace:
  abort: 9978: can't trace function-valued property set in branded scope

=== testSetMethod.js

function C() {
    this.a = function() {};
    this.b = function() {};
}
for (var i = 0; i < 9; i++)
    new C;

checkStats({
    recorderStarted: 1,
    recorderAborted: 0,
    traceCompleted: 1,
    sideExitIntoInterpreter: 1
    });

=== testInitMethod.js

for (var i = 0; i < 9; i++)
    x = {a: function() {}, b: function() {}};

checkStats({
    recorderStarted: 1,
    recorderAborted: 0,
    traceCompleted: 1,
    sideExitIntoInterpreter: 1
    });

Comment 124

[Jason Orendorff [:jorendorff]]

These comments refer to attachment 397759 [details] [diff] [review].

var x = {f: function() {}};
x.f++;
// Assertion failure: getter, at ../jsscope.cpp:1061

This assertion prevents me from testing the JIT path, which might also have a bug:

In TraceRecorder::prop:
>        if (!SPROP_HAS_STUB_GETTER_OR_IS_METHOD(sprop)) {
>            if (slotp)
>                ABORT_TRACE("can't trace non-stub getter for this opcode");
>            ...
>        }
>        if (!SPROP_HAS_VALID_SLOT(sprop, OBJ_SCOPE(obj2)))
>            ABORT_TRACE("no valid slot");
>        slot = sprop->slot;

If slotp is non-null here, the caller wants a slot it can just read and write. methodReadBarrier ensures that such a slot exists--but does it necessarily reuse the method's slot? If not, this is a bug.

for (var i = 0; i < 9; i++) {
    var x = {f: function() {}};
    x.f++;
    assertEq(isNaN(x.f), true);
}

Comment 125

[Jason Orendorff [:jorendorff]]

Sigh, isNaN(function(){}) is true, so that last test is not testing what I want.

for (var i = 0; i < 9; i++) {
    var x = {f: function() {}};
    x.f++;
    assertEq(""+x.f, "NaN");
}

We should change assertEq to ES5's SameValue semantics.

Comment 126

[Jason Orendorff [:jorendorff]]

Filed bug 514222 to arrange it so that this bug's change to JSOP_NEWINIT doesn't cause a property cache miss on the next INITPROP.

Comment 127

[Jason Orendorff [:jorendorff]]

Comment on attachment 398069 [details] [diff] [review]
combine if (sprop->isMethod()) DEBUG and non-DEBUG statements

OK. Be sure to add all the test cases from this bug. r=me with that!

Comment 128

[Brendan Eich [:brendan]]

(In reply to comment #120)
> TraceRecorder::isValidSlot:
> >     /* This check applies even when setflags == 0. */
> >-    if (setflags != JOF_SET && !SPROP_HAS_STUB_GETTER(sprop))
> >+    if (setflags != JOF_SET && !SPROP_HAS_STUB_GETTER_OR_IS_METHOD(sprop))
> >         ABORT_TRACE_RV("non-stub getter", false);
> 
> Both callers of isValidSlot assume that a "valid slot" means one they
> can just read from.  Not a bug, because both callers are looking at the
> scope chain, which never contains any objects of class js_ObjectClass;
> but I still think _OR_IS_METHOD is misleading here.

Removed, I'm asserting !sprop->isMethod before the ABORT instead. Thanks.

> TraceRecorder::callProp:
> >-            js_GetPropertyHelper(cx, obj, sprop->id, JS_FALSE, &nr.v);
> >+            js_GetPropertyHelper(cx, obj, sprop->id,
> >+                                 (op == JSOP_CALLNAME)
> >+                                 ? JSGET_NO_METHOD_BARRIER
> >+                                 : JSGET_METHOD_BARRIER,
> >+                                 &nr.v);
> 
> obj has js_CallClass, so it cannot have methods.

Future-proofing for when bug 514046 is fixed. Commented.

> Actually obj always == obj2 here. I'm puzzled as to why callProp has
> both parameters.

Fixed.

> TraceRecorder::prop:
> >    LIns* v_ins = unbox_jsval(STOBJ_GET_SLOT(obj, slot),
> >                              stobj_get_slot(obj_ins, slot, dslots_ins),
> >                              snapshot(BRANCH_EXIT));
> >
> >    /*
> >     * Joined function object stored as a method must be cloned when extracted
> >     * as a property value other than a callee. Note that shapes cover method
> >     * value as well as other property attributes and order, so this condition
> >     * is trace-invariant.
> >     */
> >    if (isMethod) {
> >        LIns* args[] = { v_ins, INS_CONSTSPROP(sprop), obj_ins, cx_ins };
> >        v_ins = lir->insCall(&MethodReadBarrier_ci, args);
> >    }
> 
> In this case we can skip stobj_get_slot and unbox_jsval, since the
> result must be sprop->methodObject(), which is trace-invariant.

No, we're going to clone. MethodReadBarrier has a dependency on the value in the slot, passed via:

    if (!OBJ_SCOPE(obj)->methodReadBarrier(cx, sprop, tvr.addr())

so that JSScope::methodReadBarrier can load funobj and clone it. So we must unbox the slot and pass it in.

If there's no need for the method read barrier, then we still need the slot value, of course.

> This call will reshape obj, but I don't think the patch in bug 504587
> needs to be updated (to purge any cached shape guards on obj-- the
> change is unavoidable, so the bogus cached shape guard will never be
> used again anyway). Mentioning this in case I'm wrong.

That patch forgets guarded shapes for the scope's object in generateOwnShape. All good there.

Updating to incorporate this feedback. More comments to come. Hope you can r+ again if the changes are bigger than a breadbox.

/be

Comment 129

[Brendan Eich [:brendan]]

(In reply to comment #128)
> > >    if (isMethod) {
> > >        LIns* args[] = { v_ins, INS_CONSTSPROP(sprop), obj_ins, cx_ins };
> > >        v_ins = lir->insCall(&MethodReadBarrier_ci, args);
> > >    }
> > 
> > In this case we can skip stobj_get_slot and unbox_jsval, since the
> > result must be sprop->methodObject(), which is trace-invariant.
> 
> No, we're going to clone.

I'm still in the bad old yesterday state of worrying about the method read barrier being called on a slot with a cloned value in it due to the write barrier bug mentioned in comment 117. D'oh!

Thanks, this helps simplify the code, removing an argument to MethodReadBarrier and relegating the *vp load in JSScope::methodReadBarrier to DEBUG-only code.

/be

Comment 130

[Brendan Eich [:brendan]]

(In reply to comment #129)
> Thanks, this helps simplify the code, removing an argument to MethodReadBarrier
> and relegating the *vp load in JSScope::methodReadBarrier to DEBUG-only code.

But that assertion would botch without the fourth argument to MethodReadBarrier. Think I'll play it safe and keep JIT and interpreter code paths both fulfilling the contract. Jason, let me know if this seems wrong to you.

/be

Comment 131

[Brendan Eich [:brendan]]

Attachment: patch with fixes for latest comments, plus three trace-test additions

Tests courtesy jorendorff. More to add to js/tests in the morning. Try-server'ing again, but this seems unregressed.

Julian, if you could stand another round of testing to confirm no ill effects, I'd be grateful.

Interdiff should work, I'll narrate in a minute.

/be

Comment 132

[Brendan Eich [:brendan]]

jsapi.cpp:JS_CallFunctionName changed per comment 121.

jsinterp.cpp:AssertValidPropertyCacheHit changed per IRC discussion to assert prop non-null (this assertion existed already; the change here removes an early if (!prop) return true).

jsopcode.tbl fixes ancient stack budget latent bugs for JSOP_INITPROP and JSOP_INITELEM, the former cloned into JSOP_INITMETHOD: these ops did not cover the stack depth they plumb when reaching under inputs to find the "in/out" obj being initialized, which is left at top of stack post-op. They had the correct stack balance but not the right absolute nuses and ndefs, which were both too small by one.

Fixing this required ancient code in the JSOP_GETTER/SETTER case in jsops.cpp that tested js_CodeSpec[op2].ndefs (not zero) to guard a push, to be fixed to test ndefs > nuses (and assert if so that they differ by one).

I got rid of the inter-opcode-case gotos in jsops.cpp. There's no need for them and they will trip up the work Jason plans to do eliminating record_SetPropHit. This required a do-while(0) for break instead of intra-opcode-case downward goto, as we do elsewhere, indenting the new JSOP_LAMBDA code that peeks ahead to check for JSOP_{SET,INIT}METHOD.

Consolidated setIndexedProperties() and setMethodBarrier() calls, which were not commoned properly into JSScope::extend, rather copied inconsistently to follow extend calls (sometimes). The only duplication now is one commented intentionally specialized inline expansion of JSScope::extend for the JSOP_INIT{PROP,METHOD} opcode case.

Beefed up JSScope::methodReadBarrier to assert that sprop->methodValue() == *vp, as suggested in comment 130.

Changes mentioned in comment 128 in response to review comments.

LeaveTree's big assertion that op is one of 15 known opcodes is now 16 -- please let me know if I should add JSOP_INIT{PROP,METHOD} as well.

Added MethodWriteBarrier builtin called when a branded scope is being extended (entry->adding()) with a function-valued property. We abort if the scope is the global object's. This addresses the tests in comment 123, which are included as trace-tests as noted previously (thanks!).

I used a switch in TR::record_SetPropHit instead of an overlong || chain of ops, and a whitelist instead of a blacklist, for the logic to distinguish JSOP_INIT* from JSOP_SET* ops (the latter need to set their result on the stack if they are not followed by JSOP_POP).

Squashed a rogue JS_FALSE that I spotted (more elsewhere, one down at least).

/be

Comment 133

[Brendan Eich [:brendan]]

(In reply to comment #132)
> LeaveTree's big assertion that op is one of 15 known opcodes is now 16 --
> please let me know if I should add JSOP_INIT{PROP,METHOD} as well.

This big assertion is in the

        if (!(bs & JSBUILTIN_ERROR)) {

consequent -- is it possible to avoid enumerating umpteen ops chained with || in the assertion, and use a JOF_* flag instead? Maybe we could cross-check it.

/be

Comment 134

[Brendan Eich [:brendan]]

Test fodder, for js/tests not trace-test/... I think:

comment 70
comment 94
comment 95
comment 103

/be

Comment 135

[Julian Seward [:jseward]]

(In reply to comment #131)
> Created an attachment (id=398330) [details]
> patch with fixes for latest comments, plus three trace-test additions


Looks fine.  Insns and mem refs increase by less than 0.1%.
No discernable change in cache misses.  More notable is that the
mispredicts are up by about 5% -- expected?  Overall though 
a 2.1% mispredict rate is not bad.


 Command: ./R471214base/js/src/BuildR/js -j ./SunSpider/tests/v8-richards.js

 I   refs:      3,705,244,483
 I1  misses:       32,450,884
 L2i misses:            5,944
 I1  miss rate:          0.87%
 L2i miss rate:          0.00%

 D   refs:      1,769,637,973  (1,175,989,667 rd   + 593,648,306 wr)
 D1  misses:          155,616  (      129,404 rd   +      26,212 wr)
 L2d misses:           41,272  (       20,649 rd   +      20,623 wr)
 D1  miss rate:           0.0% (          0.0%     +         0.0%  )
 L2d miss rate:           0.0% (          0.0%     +         0.0%  )

 L2 refs:          32,606,500  (   32,580,288 rd   +      26,212 wr)
 L2 misses:            47,216  (       26,593 rd   +      20,623 wr)
 L2 miss rate:            0.0% (          0.0%     +         0.0%  )

 Branches:        541,437,248  (  541,343,204 cond +      94,044 ind)
 Mispredicts:      10,948,618  (   10,921,718 cond +      26,900 ind)
 Mispred rate:            2.0% (          2.0%     +        28.6%   )



 Command: ./R471214_398330/js/src/BuildR/js -j ./SunSpider/tests/v8-richards.js

 I   refs:      3,706,511,605
 I1  misses:       32,441,120
 L2i misses:            5,962
 I1  miss rate:          0.87%
 L2i miss rate:          0.00%

 D   refs:      1,770,181,827  (1,176,340,814 rd   + 593,841,013 wr)
 D1  misses:          161,902  (      135,615 rd   +      26,287 wr)
 L2d misses:           41,214  (       20,534 rd   +      20,680 wr)
 D1  miss rate:           0.0% (          0.0%     +         0.0%  )
 L2d miss rate:           0.0% (          0.0%     +         0.0%  )

 L2 refs:          32,603,022  (   32,576,735 rd   +      26,287 wr)
 L2 misses:            47,176  (       26,496 rd   +      20,680 wr)
 L2 miss rate:            0.0% (          0.0%     +         0.0%  )

 Branches:        541,648,836  (  541,554,789 cond +      94,047 ind)
 Mispredicts:      11,533,000  (   11,506,241 cond +      26,759 ind)
 Mispred rate:            2.1% (          2.1%     +        28.4%   )

Comment 136

[Brendan Eich [:brendan]]

Any way to focus on which branches are most mispredicted? There's a new barrier on the "property that appears to have a getter" read-path. Perhaps more builtin-expect usage could help.

/be

Comment 137

[Julian Seward [:jseward]]

(In reply to comment #136)
> Any way to focus on which branches are most mispredicted?

In general yes (we can get details down the the loc level) but
unfortunately in this case, no, because the increase appears to
be all in jit-generated code.  There's a patch drifting round
somewhere which improves V's support for jit-generate code.  I
should revive it.

Here's all the info I have.  Seems like there's an 900k increase
in mispredicts in generated code, but a 240k fall in js_BoxDouble.
We're simulating a correlating branch predictor here, so perhaps
js_BoxDouble benefits from improved correlation of the previous
7 or so conditional branches?

Bc = # conditional branches, Bcm = # conditional branches mispredicted
???:??? denotes jit-generated code

R471214base:

         Bc       Bcm  file:function
-----------------------------------------------------------------------------
484,757,736 7,999,272  ???:???
 12,846,148 2,140,920  ???:js_BoxDouble(JSContext*, double)
  2,978,439   330,434  ???:js_Array_dense_setelem_double(JSContext*, JSObject*,
    678,474    39,370  ???:nanojit::Assembler::asm_leave_trace(nanojit::LIns*)
    345,265    33,349  ???:js_AddProperty(JSContext*, JSObject*, JSScopeProper

R471214_398330:

         Bc       Bcm  file:function
------------------------------------------------------------------------------
484,753,013 8,896,400  ???:???
 12,846,148 1,883,058  ???:js_BoxDouble(JSContext*, double)
  2,978,439   296,840  ???:js_Array_dense_setelem_double(JSContext*, JSObject*,
    678,474    39,592  ???:nanojit::Assembler::asm_leave_trace(nanojit::LIns*)
    366,395    32,223  ???:bool VisitFrameSlots<DetermineTypesVisitor>(Determi

Comment 138

[Jason Orendorff [:jorendorff]]

Comment on attachment 398330 [details] [diff] [review]
patch with fixes for latest comments, plus three trace-test additions

>+                 * We do not impose the method read barrier if in an imacro,
>+                 * assuming any property gets it does (e.g., for 'toString'
>+                 * from JSOP_NEW) will not be leaked to the calling script.

Eeuurgh, what a hack! :)

We could instead look ahead for IFPRIMTOP. That seems slightly less
nauseating to me, but it's no real change in behavior.

(In reply to comment #130)
> (In reply to comment #129)
> > Thanks, this helps simplify the code, removing an argument to MethodReadBarrier
> > and relegating the *vp load in JSScope::methodReadBarrier to DEBUG-only code.
> 
> But that assertion would botch without the fourth argument to
> MethodReadBarrier. Think I'll play it safe and keep JIT and interpreter code
> paths both fulfilling the contract. Jason, let me know if this seems wrong to
> you.

Not wrong, but I think you can fulfill the contract without the slot load and unbox:

  static JSObject* FASTCALL
 -MethodReadBarrier(JSContext* cx, JSObject* obj, JSScopeProperty* sprop, JSObject* funobj)
 +MethodReadBarrier(JSContext* cx, JSObject* obj, JSScopeProperty* sprop)
  {
 -    JSAutoTempValueRooter tvr(cx, funobj);
 +    JSAutoTempValueRooter tvr(cx, sprop->methodValue());

According to Rob, we're allowed to check tests into js/tests and it doesn't mess up bclary's system when we do. It would be a nice habit to get into. The less human bugzilla-scraping is needed to keep our test suite complete and up-to-date, the better.

Comment 139

[Brendan Eich [:brendan]]

(In reply to comment #138)
> (From update of attachment 398330 [details] [diff] [review])
> >+                 * We do not impose the method read barrier if in an imacro,
> >+                 * assuming any property gets it does (e.g., for 'toString'
> >+                 * from JSOP_NEW) will not be leaked to the calling script.
> 
> Eeuurgh, what a hack! :)

I don't think this is that bad, because the imacro expands to thinner bytecode to interpret, and it's up to the interpreter to select the read barrier or not. Any internal gets for implicit conversion calls must not leak, since they either lead to a call (callee doesn't leak, no read barrier in JOF_CALLOP ops) or to an alternative conversion hook attempt, or to failure.

But this is all about imacros today. Perhaps there could be a leaking use-case some day. Hard to imagine what it would be, though.

> We could instead look ahead for IFPRIMTOP. That seems slightly less
> nauseating to me, but it's no real change in behavior.

I thought about this but it seemed too tightly wound. I've gone that way in the past and been burned when an over-tight assumption unwound slightly.

> Not wrong, but I think you can fulfill the contract without the slot load and
> unbox: ...

But I want a non-vacuous assertion on trace too. Small perf hit for hard case, can optimize harder if necessary based on method-extracting benchmarketing (if any exists).

> According to Rob, we're allowed to check tests into js/tests and it doesn't
> mess up bclary's system when we do. It would be a nice habit to get into. The
> less human bugzilla-scraping is needed to keep our test suite complete and
> up-to-date, the better.

Sure, I was too tired to write the tests, s'all.

Revised patch coming, worth another look (small interdiff, but could use your review again -- thanks).

/be

Comment 140

[Brendan Eich [:brendan]]

Attachment: MethodWriteBarrier condition fix

Retesting, need a testcase for the previous patch showing the bug.

/be

Comment 141

[Jason Orendorff [:jorendorff]]

Comment on attachment 398419 [details] [diff] [review]
MethodWriteBarrier condition fix

I think this is more correct. Even though I said on IRC that it should be OK to kill the directHit() check, I think I as wrong.  The more restrictive we are here, the better.

Comment 142

[Brendan Eich [:brendan]]

http://hg.mozilla.org/tracemonkey/rev/842e6c09e35a

Hoping it sticks this time. Had an anomalous Windows tryserver orange in a11y test (sayrer said it wasn't me).

/be

Comment 143

[Julian Seward [:jseward]]

Final perf comments,
  32129:6f6aeece5fe4 (pre) vs 32130:842e6c09e35a (post)
running the old monolithic trace-tests with -e 'gSkipSlowTests=true'

  I refs, D refs, D misses: increase by less than 0.1%
  I misses: increase by 3%
  Mispredicts: decrease by 0.7%

Mixed bag, basically harmless IMO.

Leaks: At end of program:

  Before: in use at exit: 163,915 bytes in 2,282 blocks
  After:  in use at exit: 163,963 bytes in 2,283 blocks

so there's one extra block unfreed.  But it was leaking pretty bad
before this landed.

Comment 144

[Julian Seward [:jseward]]

Oh, and apart from the leaks, no visible Memcheck or Ptrcheck
bad-address or undefined-value errors either before or after.

Comment 145

[Brendan Eich [:brendan]]

(In reply to comment #143)
> Final perf comments,
>   32129:6f6aeece5fe4 (pre) vs 32130:842e6c09e35a (post)
> running the old monolithic trace-tests with -e 'gSkipSlowTests=true'
> 
>   I refs, D refs, D misses: increase by less than 0.1%
>   I misses: increase by 3%
>   Mispredicts: decrease by 0.7%
> 
> Mixed bag, basically harmless IMO.

Thanks, good information to have. Could we ever automate and track trends?

> Leaks: At end of program:
> 
>   Before: in use at exit: 163,915 bytes in 2,282 blocks
>   After:  in use at exit: 163,963 bytes in 2,283 blocks
> 
> so there's one extra block unfreed.  But it was leaking pretty bad
> before this landed.

Can you find the allocating stack for that block?

/be

Comment 146

[Robert Sayre]

(In reply to comment #145)
> 
> Thanks, good information to have. Could we ever automate and track trends?

Yes, given a test infrastructure engineer assigned to the task.

Comment 147

[Blake Kaplan (:mrbkap) (inactive)]

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

Comment 148

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/mozilla-central/rev/842e6c09e35a (for real this time).

Comment 149

[Robert Sayre]

going to try and keep this off 1.9.2 for now, unless we can bug 518103 fixed up.

Comment 150

[Robert Sayre]

the amazon bug 520319, and bug 520778 seem to keep this out of the beta, and therefore 1.9.2. anyone disagree?

Comment 151

[Brendan Eich [:brendan]]

I'm concerned about divergence between the branch and trunk but maybe it'll be ok. I'm also working on 520319, hope to have a fix shortly. Bug 520778 is not in the same league -- we could change __noSuchMethod__ not to fire for new o.nosuch and see how that flies. Or I could fix that too, and I did a bit of work to see how that might look last night.

It may be we will end up wanting this bug and bugs it blocks, particularly bug 514186 -- but no one has done that to measure the win.

/be

Comment 152

[Bob Clary [:bc] (inactive)]

js/src/trace-test/tests/basic/testInitMethod.js
js/src/trace-test/tests/basic/testMethodInc.js
js/src/trace-test/tests/basic/testSetMethod.js

Comment 153

[Rob Campbell [:rc] (:robcee)]

since this patch seems to be causing a crash with Firebug and/or Chromebug in trunk, we're going to want to keep this patch out of branch 1.9.2 for the foreseeable future.

This could cause problems for compatibility and js fixes down the road, but the alternative is rewriting some APIs to support Firebug that we don't really have time to do before release.

see bug 525009 and friends.

Comment 154

[Rob Campbell [:rc] (:robcee)]

we should probably change the target milestone to 1.9.3 to be extra clear, but I'll leave that for the bug owner.

Comment 155

[Brendan Eich [:brendan]]

The big regression is bug 524826, about to be fixed.

Is this bug causing any crashes in Firebug beyond those the upvar2 bug caused? Rob, I read bug 525009 and I don't see this as more dangerous. Joining function objects does not introduce new upvar-fetching crashes.

/be

Comment 156

[Rob Campbell [:rc] (:robcee)]

Brendan, if you think this is safe to land then go for it. I'll keep an eye on the landing and make sure we don't see any crashes from Firebug.

I was under the impression something in this patch was causing crashes in trunk, but maybe that's not the case. Feel free to remove my whiteboard status.

Comment 157

[Brendan Eich [:brendan]]

No, this bug is not ready to land on a branch, see its open dependencies. The "do not land in 1.9.2" status whiteboard is fine but redundant given those blockers and the TM setting.

But, if you had a real crash bug (the open blockers are not crash bugs) then what bug was it, and why wasn't it made blocking?

/be

Comment 158

[Robert Sayre]

(In reply to comment #157)
> 
> But, if you had a real crash bug (the open blockers are not crash bugs) then
> what bug was it, and why wasn't it made blocking?

It is bug 522590, which is listed above.

https://bugzilla.mozilla.org/show_bug.cgi?id=522590#c47

Comment 159

[Brendan Eich [:brendan]]

Thanks, sayrer -- I was worried there was another bug.

/be

Comment 160

[Rob Campbell [:rc] (:robcee)]

yeah, thanks for the clarification, sayrer.