503408 - Trace native setters

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[David Mandelin [:dmandelin]]

jhiatt's app hits this abort:

    if (PCVCAP_TAG(entry->vcap) >= 1)
        ABORT_TRACE("can't trace inherited property set");

My first tries didn't produce a test case that triggers this, so maybe I don't understand what it means. Maybe we can reduce a case from his code.

Comment 1

[Brendan Eich [:brendan]]

What testcase? jorendorff probably is already on the case here, but we need a targeted testcase.

/be

Comment 2

[David Mandelin [:dmandelin]]

jhiatt, could you please provide a minimized test case from your code?

Comment 3

[jhiatt]

Attachment: Testcase to trigger abort.

I've uploaded a testcase that sets regex lastIndex properties (and triggers the abort) in a similar manner to our "Real Code."

Comment 4

[Jason Orendorff [:jorendorff]]

Thanks. This is exactly what I'm working on, other than bug 503080. It's actually very nice to have a specific bug to fit that work into, since that patch does not trace all getters and setters.

Note that "inherited property set" in practice means a SHARED non-default setter.

Comment 5

[Jason Orendorff [:jorendorff]]

Attachment: WIP 1

The test case will not trace even with this patch, because it calls getters as well as setters.

The patch also has a bug: it does not handle tinyids. Easy to fix, but unfortunately I don't have the time right now.

Comment 6

[Jason Orendorff [:jorendorff]]

Attachment: WIP 2

Tiny fix (interdiff works, for the curious).

This might be up for review tomorrow morning, if the Try Server comes up green.

Comment 7

[Jason Orendorff [:jorendorff]]

Try Server is green, except for a test failure on Mac that I can't make heads or tails of:

http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1248396606.1248404204.12526.gz

But I discovered two bugs by inspection.

First, on trace I don't do anything analogous to this check (in js_NativeSet):

>    if (SLOT_IN_SCOPE(slot, scope) &&
>        (JS_LIKELY(cx->runtime->propertyRemovals == sample) ||
>         scope->has(sprop))) {
>  set_slot:
>        LOCKED_OBJ_WRITE_SLOT(cx, obj, slot, *vp);
>    }

I think on trace, we must shape-check again after calling the native setter.

Second, if the native setter deep-bails, I bail out without storing the result. Oops.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Can we add tests that would have caught those bugs?

Comment 9

[Jason Orendorff [:jorendorff]]

>I think on trace, we must shape-check again after calling the native setter.

Checking the shape is no good. While the native setter was running, there may have been a GC that regenerated shapes, so a coincidental match is possible.

So I think we actually need to sample rt->propertyRemovals before and after, just like js_NativeSet does. (...Another possibility would be to sample scope->lastProp before and after. Actually I'm not sure why js_NativeSet doesn't do that.)

If there is a mismatch, it's extremely inconvenient to bail off trace here, as we're sort of in the middle of something. So instead I am going to implement that if-statement in LIR, and call a builtin on the slow path.

Calling a native setter generates a long ton of code. This will make it that much worse. :-P

Comment 10

[Jason Orendorff [:jorendorff]]

(In reply to comment #8)
> Can we add tests that would have caught those bugs?

Hard to do. We don't have a lot of convenient native setters, and it is hard to put those in trace-tests, for obvious reasons. (But see bug 505798.  Sigh, stretched a bit thin these days...)

(Maybe the shell should have a function that runs a string of C++ through the compiler and loads it...)

Comment 11

[Jason Orendorff [:jorendorff]]

(In reply to comment #7 and comment #9)

Discovered an elegant solution: don't trace assignment to non-SHARED properties with setters.

Comment 12

[Jason Orendorff [:jorendorff]]

Attachment: interdiff v2 to v3

Comment 13

[Jason Orendorff [:jorendorff]]

Attachment: v3

Comment 14

[Brendan Eich [:brendan]]

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

Pre-existing comment that's relocated by the patch:

    // Interpreter calls to PROPERTY_CACHE_TEST guard on native object ops

really wants to use /*\n * Major\nstyle\n */ -- why not convert it now?

I just realized test_property_cache (now a method factored from it) could use a common snapshot() for the two BRANCH_EXIT guards. Should it?

alreadyEmittedGuardOnNativeOps is false in one call, true in another. It's used only at the top of TraceRecorder::guardPropertyCacheHit. So just take that code out into the caller who passes false.

Nit: in this comment:

     * if rval is JS_FALSE (error), and then shift that by 1 which is
     * JSBUILTIN_ERROR.

"... which is the log2 of JSBUILTIN_ERROR", right?

Is lir->ins2i(LIR_and, ok_ins, 1) necessary? Woulda thunk ok_ins resulted in 0 or 1.

Nit: sentence after

     * If obj is the global object, there are two additional problems. We would

uses subjunctive, but the next sentence begins "And we must cope if... is different than ..." -- use "would have to cope if... were different from ..." ("other than" vs. "different from" Jacques Barzun style point bonus! ;-)

    // These two cases are actually errors and would normally have bumped us
    // off trace already.

Why didn't we get bumped off trace already, then? Or is this comment saying that we test late where we could test earlier, to consolidate the tests?

    JS_ASSERT_IF(PCVCAP_TAG(entry->vcap) == 0 && PCVCAP_SHAPE(entry->vcap) == entry->kshape,

This is shorter and prettier if you test PCVCAP_MAKE(entry->kshape, 0, 0) == entry->vcap.

     * Find obj2 if relevant.  If entry->adding(), then entry's TAG does not
     * contain accurate PROTOBITS and SCOPEBITS, and a cache hit indicates obj2

is the TAG inaccurate or just all zeroes (not useful but not incorrect)?

Time to s/native_set/nativeSet/, s/test_property_cache/propertyCacheTest/, etc.? Your call.

Why does test_property_cache take a JSPropCacheEntry out param and fill it in, when callers (AFAICT) only use entry.vword after?

Looks good, I'll r+ but I wanted to throw those comments and questions out quickly. Edited them into a side-file instead of using "Edit Attachment As Comment", I hope they are clear enough.

/be

Comment 15

[Jason Orendorff [:jorendorff]]

All these comments are good, just a few notes:

(In reply to comment #14)
> I just realized test_property_cache (now a method factored from it) could use a
> common snapshot() for the two BRANCH_EXIT guards. Should it?

Yes, I think so, but I'll do it in a second changeset...

> Is lir->ins2i(LIR_and, ok_ins, 1) necessary? Woulda thunk ok_ins resulted in 0
> or 1.

Sure, in a perfect world, but we've been treating (JSBool) 2 and -74 as success forever, right?

This code was there before and has just been factored out; I don't want to change it in the same patch as the rest of these changes.

> uses subjunctive, but the next sentence begins "And we must cope if... is
> different than ..." -- use "would have to cope if... were different from ..."
> ("other than" vs. "different from" Jacques Barzun style point bonus! ;-)

Guilty as charged. Changed to "if the run-time type ... differed from" to avoid the subjunctive "were", which sounds hyper-correct to me.

>     // These two cases are actually errors and would normally have bumped us
>     // off trace already.
> 
> Why didn't we get bumped off trace already, then? Or is this comment saying
> that we test late where we could test earlier, to consolidate the tests?

I've changed them to JS_ASSERTs--these cases can't happen.

>      * Find obj2 if relevant.  If entry->adding(), then entry's TAG does not
>      * contain accurate PROTOBITS and SCOPEBITS, and a cache hit indicates obj2
> 
> is the TAG inaccurate or just all zeroes (not useful but not incorrect)?

It's just all zeroes--and actually that allows me to simplify the code a bit.

> Why does test_property_cache take a JSPropCacheEntry out param and fill it in,
> when callers (AFAICT) only use entry.vword after?

I ...don't know. Andreas did that part. :) If it's not necessary, I can change it back, need to look.

Comment 16

[Jason Orendorff [:jorendorff]]

bz noticed that the patch doesn't work. Embarrassingly my new trace-test doesn't actually test what it looks like. It only loops RUNLOOP times, and changing that to RUNLOOP+10 made it fail!

One-line fix:
-    guard(true, lir->ins_eq0(ok_ins), STATUS_EXIT);
+    guard(false, lir->ins_eq0(ok_ins), STATUS_EXIT);

Comment 17

[Jason Orendorff [:jorendorff]]

Attachment: v4

I was able to change test_property_cache back the way it was with no trouble.

Comment 18

[Jason Orendorff [:jorendorff]]

Bugzilla's interdiff works between v3 and v4.

v4 is a smaller patch by 8KB.  I think most of that is not changing test_property_cache and all its call sites.

Comment 19

[Brendan Eich [:brendan]]

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

Looks good, thanks.

/be

Comment 20

[Jason Orendorff [:jorendorff]]

http://hg.mozilla.org/tracemonkey/rev/0184d3d87db9

Comment 21

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/0184d3d87db9

Comment 22

[Bob Clary [:bc] (inactive)]

trace-test testNativeSetter