Closed Bug 512617 Opened 13 years ago Closed 11 years ago

TM: consolidate object creation on trace


(Core :: JavaScript Engine, defect)

Not set





(Reporter: gal, Assigned: gal)




(2 files)

No description provided.
Attached patch patchSplinter Review
Assignee: general → gal
Attachment #396644 - Flags: review?(jorendorff)
Please, when filing bugs with such imperatives, take a few minutes to explain what motivates it and what your approach is, so that people don't have to reverse engineer it from the patch.
Blocks: 513291
We currently have several different implementations of the higher level of the object allocation path (filling in the slots and internal fields after NewGCThing). The attached patch unifies these paths into one function. It also avoids fetching the constructor's .prototype property by baking it in for permanent constructor values (and their properties) such as Object. For non-permanent .prototype values we use the value we saw at recording time, and guard that the value hasn't changed since then. The goal is to avoid .prototype lookups during object creation, and to reduce code clutter.
Types in jsbuiltins.h shouldn't have magic letters unless they are used for TRCINFO.

If you just want to call a helper function from trace, you only need CALLINFO.  TRCINFO is for attaching fast implementations like array_push to function objects like Array.prototype.push.

Attaching a patch to apply on top of yours...
Comment on attachment 396644 [details] [diff] [review]

I think you can actually just delete the CALLEE_PROTOTYPE and CALLEE_PARENT types altogether. Replace both with OBJECT wherever they appear.

And you can move js_String_tn to jstracer.cpp, just before the one place where it's used (TR::newString), make it static, and rename it to NewStringObject.

>    while (!(sprop = scope->lookup(ATOM_TO_JSID(atom)))) {
>        /* No ctor.prototype yet, inline and optimize fun_resolve's prototype code. */
>        JSObject* proto = js_NewObject(cx, OBJ_GET_CLASS(cx, ctor), NULL, OBJ_GET_PARENT(cx, ctor));
>        if (!proto)
>            ABORT_TRACE_ERROR("js_NewObject failed");
>        if (!js_SetClassPrototype(cx, ctor, proto, JSPROP_ENUMERATE | JSPROP_PERMANENT))
>            ABORT_TRACE_ERROR("js_SetClassPrototype failed");
>        if (!OBJ_IS_NATIVE(proto))
>            ABORT_TRACE("not-native prototype");
>    }

I couldn't read this until you explained it to me, :) so I'm reminding
you about it here in case you want to change it.

       20: 9167:    if (JSVAL_TAG(pval) != JSVAL_OBJECT)
    #####: 9168:        ABORT_TRACE("got primitive prototype from constructor");

Add a trace-test for this case, so we have it against future regressions?

        -: 9193:JSRecordingStatus
    #####: 9194:TraceRecorder::newObject(JSObject* ctor, uint32 argc, jsval* argv, jsval* rval)
        -: 9195:{
    #####: 9196:    JS_ASSERT(argc == 0);
        -: 9197:
    #####: 9198:    LIns* parent_ins = INS_CONSTOBJ(OBJ_GET_PARENT(cx, ctor));

Likewise.  I ran our whole test suite (trace-test and js/tests) and
apparently this didn't get called at all. It looks fine though.

r=me with these comments addressed, and comment 4.
Attachment #396644 - Flags: review?(jorendorff) → review+
Whiteboard: fixed-in-tracemonkey
Follow-up compilation fix. bonus patch broke quickstubs.
I am going to get it right any day now.
Why the backout? Is it going to land again?
Whiteboard: fixed-in-tracemonkey
Your bonus patch broke quickstubs in exciting ways.
Obsolete with the removal of tracejit.
Closed: 11 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.