Closed Bug 542797 Opened 12 years ago Closed 12 years ago

change obj_eval from JSNative to JSFastNative

Categories

(Core :: JavaScript Engine, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: luke, Assigned: luke)

References

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file, 2 obsolete files)

Attached patch do it (obsolete) — Splinter Review
Because obj_eval is a slow native, an extra JSStackFrame gets created which is immediately ignored by when js_Execute is called with down = cx->fp->down.  This requires creating an extra JSCallStack and taking the SlowVarObj path in js_Execute.  Also, its really tricky to reason about.  This patch makes obj_eval a fast native and removes some dead/confounding logic in obj_eval.

The patch is on top of the remove-var-obj patch in bug 535656.
Attachment #424025 - Flags: review?(mrbkap)
Attached patch freshend up, tweaked (obsolete) — Splinter Review
Assignee: general → lw
Attachment #424025 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #424429 - Flags: review?(mrbkap)
Attachment #424025 - Flags: review?(mrbkap)
Comment on attachment 424429 [details] [diff] [review]
freshend up, tweaked

I complained on IRC about using JS_ARGV and JS_THIS_OBJECT instead of hand-unrolling them.

Also (more substantially) js_CheckScopeChainValidity can fail.

It's nice to see that maze of declarations die.
Attachment #424429 - Flags: review?(mrbkap) → review-
Attached patch less wrongSplinter Review
Nice catch, thanks.
Attachment #424429 - Attachment is obsolete: true
Attachment #425584 - Flags: review?(mrbkap)
Comment on attachment 425584 [details] [diff] [review]
less wrong

Thanks!
Attachment #425584 - Flags: review?(mrbkap) → review+
Comment on attachment 425584 [details] [diff] [review]
less wrong

Nice goto elimination via RAII!

>     if (callbacks && callbacks->findObjectPrincipals) {
>-        principals = callbacks->findObjectPrincipals(cx, fp->callee());
>+        principals = callbacks->findObjectPrincipals(cx, callee);
>     } else {
>         principals = NULL;
>     }

Dunno why this was over-braced, but it need not be.

/be
Attachment #425584 - Flags: review+ → review?(mrbkap)
Attachment #425584 - Flags: review?(mrbkap) → review+
With less bracing:
http://hg.mozilla.org/tracemonkey/rev/e91417e33a53
Whiteboard: fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/e91417e33a53
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.