Andreas Gal :gal Assignee
	Description • 15 years ago

      No description provided.

	Mike Shaver (:shaver -- probably not reading bugmail closely)
	Comment 2 • 15 years ago

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.

	Andreas Gal :gal Assignee
	Comment 3 • 15 years ago

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.

	Jason Orendorff [:jorendorff]
	Comment 4 • 15 years ago

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...

	Jason Orendorff [:jorendorff]
	Comment 6 • 15 years ago

Comment on attachment 396644 [details] [diff] [review]
patch

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.

	Andreas Gal :gal Assignee
	Comment 7 • 15 years ago

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

	Andreas Gal :gal Assignee
	Comment 8 • 15 years ago

Follow-up compilation fix. bonus patch broke quickstubs.

https://bugzilla.mozilla.org/show_bug.cgi?id=512617

	Andreas Gal :gal Assignee
	Comment 9 • 15 years ago

I am going to get it right any day now.

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

	Andreas Gal :gal Assignee
	Comment 10 • 15 years ago

Backed out.

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

	Jason Orendorff [:jorendorff]
	Comment 11 • 15 years ago

Why the backout? Is it going to land again?

	Andreas Gal :gal Assignee
	Comment 12 • 15 years ago

Your bonus patch broke quickstubs in exciting ways.

	Blake Kaplan (:mrbkap) (inactive)
	Comment 13 • 15 years ago

http://hg.mozilla.org/mozilla-central/rev/0f4edc6ddc39 -- is this fixed?

	Ryan VanderMeulen [:RyanVM]
	Comment 14 • 13 years ago

Obsolete with the removal of tracejit.