474501 - JSON literals shouldn't have prototype setters run during compilation

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Ben Turner (not reading bugmail, use the needinfo flag!)]

See bug 375250 comment 19. JSON literals shouldn't have prototype setters run during compilation. This bug blocks bug 375250 and has been deemed a P1 priority for beta 3.

Assigning to waldo per offline discussion.

Comment 1

[Damon Sicore (:damons)]

Jeff, as soon as you have an estimate on what it's going to take to resolve
this, can you update?  Just trying to get a realistic estimate on beta 3 wrap
up.

Comment 2

[Jeff Walden [:Waldo]]

I think this can be done by the end of the week without much trouble; biggest thing to figure out is exactly where to munge existing APIs to handle property-setting in object literals, because right now we go through several levels of nesting to finally get to the one place where properties are set, and I'm not where where the best place to insert a split is.

Comment 3

[Jeff Walden [:Waldo]]

Attachment: Et voilà

SPROP_SET stuff is left over from an earlier version of the patch; it's not relevant any more, but it is righteous and harmless.  I do, however, question whether it's really a worthwhile method to inline -- wouldn't be surprised if compilers ignored that.

Is there a reason we don't have JSPROP_DEFAULT_ATTRS yet?  The bare JSPROP_ENUMERATE is cringe-worthy.

Comment 4

[Brendan Eich [:brendan]]

Comment on attachment 359844 [details] [diff] [review]
Et voilà

>+                if (!OBJ_DEFINE_PROPERTY(cx, obj, id, rval, NULL, NULL, 0,

Need JSPROP_ENUMERATE here.


>+                                         NULL)) {

Feel free to avoid wrapping and bracing, given new tw=99 standard.

>+            if (!OBJ_DEFINE_PROPERTY(cx, obj, id, rval, NULL, NULL, 0, NULL))

Ditto (this is JSOP_ARRAYPUSH).

>-                return !!SPROP_SET(cx, sprop, obj, pobj, vp);
>+                return SPROP_SET(cx, sprop, obj, pobj, vp);

Taras take note -- we don't need !! here.

> #define SPROP_GET(cx,sprop,obj,obj2,vp)                                       \
>     (((sprop)->attrs & JSPROP_GETTER)                                         \
>      ? js_InternalGetOrSet(cx, obj, (sprop)->id,                              \
>                            OBJECT_TO_JSVAL((sprop)->getter), JSACC_READ,      \
>                            0, 0, vp)                                          \
>      : (sprop)->getter(cx, OBJ_THIS_OBJECT(cx,obj), SPROP_USERID(sprop), vp))

Please do SPROP_GET as a static inline helper, since you are doing SET.

> 
>-/*
>- * NB: SPROP_SET must not be called if (SPROP_HAS_STUB_SETTER(sprop) &&
>- * !(sprop->attrs & JSPROP_GETTER)).
>- */
>-#define SPROP_SET(cx,sprop,obj,obj2,vp)                                       \
>-    (((sprop)->attrs & JSPROP_SETTER)                                         \
>-     ? js_InternalGetOrSet(cx, obj, (sprop)->id,                              \
>-                           OBJECT_TO_JSVAL((sprop)->setter), JSACC_WRITE,     \
>-                           1, vp, vp)                                         \
>-     : ((sprop)->attrs & JSPROP_GETTER)                                       \
>-     ? (JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL,                    \
>-                             JSMSG_GETTER_ONLY, NULL), JS_FALSE)              \
>-     : (sprop)->setter(cx, OBJ_THIS_OBJECT(cx,obj), SPROP_USERID(sprop), vp))
>+static JS_INLINE JSBool
>+SPROP_SET(JSContext* cx, JSScopeProperty* sprop, JSObject* obj, JSObject* obj2,
>+          jsval* vp)
>+{
>+    JS_ASSERT(!(SPROP_HAS_STUB_SETTER(sprop) &&
>+                !(sprop->attrs & JSPROP_GETTER)));

Comment still applies but I guess the assertion speaks for itself.

Nice, I bless the ride-along change to inline SPROP_SET (and GET) and pray they don't backfire on some lame platform.

r=me with above addressed.

/be

Comment 5

[Jeff Walden [:Waldo]]

http://hg.mozilla.org/tracemonkey/rev/9c1020fb138a
http://hg.mozilla.org/tracemonkey/rev/3cfef5bd1da4
http://hg.mozilla.org/tracemonkey/rev/e6af5a3866fa

Fixed in TM.

Comment 6

[Jeff Walden [:Waldo]]

Bleh, had to back out because including __proto__ in an object literal will then set the wrong property attributes on it.  Still thinking about how to make that work correctly...

Comment 7

[Brendan Eich [:brendan]]

Sorry, should have seen this coming. Settable __proto__ is something to remove, or at least confine. The object initialiser occurrence of __proto__:... is one confined form (no cycle can be created; useful for one-off prototype "beget" use-cases).

Best fix is to notice in the emitter that the property id is protoAtom and emit JSOP_DUP;JSOP_SETPROP;JSOP_POP instead of JSOP_INITPROP.

/be

Comment 8

[Brendan Eich [:brendan]]

(In reply to comment #7)
> Sorry, should have seen this coming. Settable __proto__ is something to remove,

I mean from the language! Just not yet. Another JS2 or Mozilla 2 breaking change.

/be

Comment 9

[Jeff Walden [:Waldo]]

Attachment: Partway there, possibly all the way outside the decompiler

Behold:

js> function() { var x = {x setter: 5}; };
function () {
    ({.x = 5);
    var x = {};
}


I may need to add countAtom to the specials list before this is all done, but I'm not familiar enough with it, its purpose, or its function to say for sure.

This fails a bunch of decompiler tests but otherwise, I think, may be okay.

Comment 10

[Brendan Eich [:brendan]]

__parent__ is read-only, no need to special-case it. Same with __count__.

Why change anything todo with setters or getters?

/be

Comment 11

[Jeff Walden [:Waldo]]

Attachment: Archival copy of latest version

I had a feeling slowifying setters and getters would be a simplification, but that turned out to be wrong.  Here's what I have now, but given this requires decompiler hacking, it seems the easy way forward for now is just to split in JSOP_INITPROP, so doing that momentarily...

Comment 12

[Jeff Walden [:Waldo]]

Attachment: Just split in JSOP_INITPROP

I don't believe we need to do anything special for tracing __proto__, because the entry passed to record_SetPropMiss should route us through js_AddProperty and do the right thing.  My minimal tests of creating an object literal in a loop seemed to verify this, at least.

Comment 13

[Brendan Eich [:brendan]]

Comment on attachment 360413 [details] [diff] [review]
Just split in JSOP_INITPROP

>@@ -6224,31 +6229,31 @@ js_Interpret(JSContext *cx)
>              * an object initialiser, not an array initialiser).
>              */
>             if (!js_CheckRedeclaration(cx, obj, id, JSPROP_INITIALIZER, NULL,
>                                        NULL)) {
>                 goto error;
>             }
> 
>             /*
>-             * If rval is a hole, do not call OBJ_SET_PROPERTY. In this case,
>+             * If rval is a hole, do not call OBJ_DEFINE_PROPERTY. In this case,
>              * obj must be an array, so if the current op is the last element
>              * initialiser, set the array length to one greater than id.
>              */
>             if (rval == JSVAL_HOLE) {
>                 JS_ASSERT(OBJ_IS_ARRAY(cx, obj));
>                 JS_ASSERT(JSID_IS_INT(id));
>                 JS_ASSERT((jsuint) JSID_TO_INT(id) < ARRAY_INIT_LIMIT);
>                 if ((JSOp) regs.pc[JSOP_INITELEM_LENGTH] == JSOP_ENDINIT &&
>                     !js_SetLengthProperty(cx, obj,
>                                           (jsuint) (JSID_TO_INT(id) + 1))) {
>                     goto error;
>                 }
>             } else {
>-                if (!OBJ_SET_PROPERTY(cx, obj, id, &rval))
>+                if (!OBJ_DEFINE_PROPERTY(cx, obj, id, rval, NULL, NULL, JSPROP_ENUMERATE, NULL))
>                     goto error;

Nice minimal patch -- but to make it look better, pull that js_SetLengthProperty param up and lose the braces. Could also hit js_CheckRedeclaration call. I'm in favor, but the conservative alternative is to wrap OBJ_DEFINE_PROPERTY before 80 and brace its goto error. Up to you.

>+#ifdef __cplusplus /* LiveConnect, you suck. */

Better to avoid insults and just cite a bug in a FIXME: -- jst knows the bug number I'm sure. Oh, found it: bug 442399.

r=me with above addressed.

/be

Comment 14

[Andreas Gal :gal]

Backed out.

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

This patch seriously changes our trace stats in DBG shell:

FAILED: 0
recorder: started(3661), aborted(2715), completed(1473), different header(20), trees trashed(4), slot promoted(0), unstable loop variable(157), breaks(2), returns(14), unstableInnerCalls(71)
monitor: triggered(3247325), exits(3247092), type mismatch(0), global mismatch(24)

Without the patch:

FAILED: 0
recorder: started(1332), aborted(381), completed(1422), different header(20), trees trashed(4), slot promoted(0), unstable loop variable(142), breaks(2), returns(14), unstableInnerCalls(71)
monitor: triggered(367813), exits(367575), type mismatch(0), global mismatch(24)

This needs some investigation.

Comment 15

[Jeff Walden [:Waldo]]

Attachment: Wrong shape => cache miss every time, cf. js_SetPropertyHelper

Comment 16

[Brendan Eich [:brendan]]

Comment on attachment 360678 [details] [diff] [review]
Wrong shape => cache miss every time, cf. js_SetPropertyHelper

>+    uint32_t shape = OBJ_SHAPE(obj);

Stick with uint32 -- we don't need incremental style mixing where it doesn't pay (contrast with bool).

I should have caught this, since js_SetPropertyHelper has the same need. r=me with _t elimination.

/be

Comment 17

[Jeff Walden [:Waldo]]

In TM with that change (finally):

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

Comment 18

[Robert Sayre]

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

Comment 19

[Eric Shepherd [:sheppy]]

tagged doc needed; waldo says to wait until he blogs about it on webtech.

Comment 20

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/b36fd215e558

Comment 21

[Eric Shepherd [:sheppy]]

As far as I can tell, this never got blogged about.  I'm still waiting before I write this up.

Comment 22

[Jeff Walden [:Waldo]]

Yikes, forgot to CC you on bug 485645 -- done that now.

Comment 23

[Eric Shepherd [:sheppy]]

This is now mentioned in all the appropriate places that I could find. While I was at it, I also did some cleanup work on:

https://developer.mozilla.org/En/New_in_JavaScript_1.8.1