If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

TM: consolidate object creation on trace

RESOLVED WONTFIX

Status

()

Core
JavaScript Engine
RESOLVED WONTFIX
8 years ago
4 years ago

People

(Reporter: gal, Assigned: gal)

Tracking

Trunk
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

Comment hidden (empty)
(Assignee)

Comment 1

8 years ago
Created attachment 396644 [details] [diff] [review]
patch
Assignee: general → gal
(Assignee)

Updated

8 years ago
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
(Assignee)

Comment 3

8 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.
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...
Created attachment 397430 [details] [diff] [review]
free bonus patch
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.
Attachment #396644 - Flags: review?(jorendorff) → review+
(Assignee)

Comment 7

8 years ago
http://hg.mozilla.org/tracemonkey/rev/0f4edc6ddc39
Whiteboard: fixed-in-tracemonkey
(Assignee)

Comment 8

8 years ago
Follow-up compilation fix. bonus patch broke quickstubs.

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

Comment 9

8 years ago
I am going to get it right any day now.

http://hg.mozilla.org/tracemonkey/rev/c773f60c47f7
(Assignee)

Comment 10

8 years ago
Backed out.

http://hg.mozilla.org/tracemonkey/rev/c771fb215673
Why the backout? Is it going to land again?
(Assignee)

Updated

8 years ago
Whiteboard: fixed-in-tracemonkey
(Assignee)

Comment 12

8 years ago
Your bonus patch broke quickstubs in exciting ways.
http://hg.mozilla.org/mozilla-central/rev/0f4edc6ddc39 -- is this fixed?
Obsolete with the removal of tracejit.
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.