Closed Bug 507573 Opened 11 years ago Closed 11 years ago

put activation cleanup

Categories

(Core :: JavaScript Engine, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
status1.9.2 --- beta1-fixed

People

(Reporter: igor, Assigned: igor)

References

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file)

I factor out this bug from the bug 495061 to minimize the patch there. Right now I am having troubles to reproduce try server failures with a full patch there so I decided to file a part of under a different bug to minimize the amount of changes there.

The idea for this bug is to hide js_Put(Call|Arguments)Object behind a single call and to cleanup there implementation.
Attached patch v1Splinter Review
The patch hides Put(Call|Arguments)Objects behind fp->putActivationObjects and cleanup various bits of Arguments object implementation to minimize the patch for the bug 495061.
Attachment #391873 - Flags: review?(brendan)
This fixes some of bug 507453, right?

/be
Blocks: 507453
Comment on attachment 391873 [details] [diff] [review]
v1

>@@ -126,16 +126,33 @@ struct JSStackFrame {
>     JSObject        *sharpArray;    /* scope for #n= initializer vars */
>     uint32          flags;          /* frame flags -- see below */
>     JSStackFrame    *dormantNext;   /* next dormant frame chain */
>     JSObject        *xmlNamespace;  /* null or default xml namespace in E4X */
>     JSStackFrame    *displaySave;   /* previous value of display entry for
>                                        script->staticLevel */
> 
>     inline void assertValidStackDepth(uintN depth);
>+
>+    JSBool putActivationObjects(JSContext *cx) {
>+        /*
>+         * The order of calls here is important as js_PutCallObject needs to
>+         * access argsobj.
>+         */
>+        JSBool ok;
>+        if (callobj) {
>+            ok = js_PutCallObject(cx, this);
>+            JS_ASSERT(!argsobj);
>+        } else if (argsobj) {
>+            ok = js_PutArgsObject(cx, this);
>+        } else {
>+            ok = JS_TRUE;
>+        }
>+        return ok;
>+    }

Is it necessary to use JSBool throughout to avoid conversion overhead? If so, can the internal js_Put*Object helpers be changed to have bool return type, etc.?

r=me either way, thanks.

/be
Attachment #391873 - Flags: review?(brendan) → review+
http://hg.mozilla.org/tracemonkey/rev/29f7e2f6319b - I kept JSBool as the result type for putActivationObjects. The function eventually calls DefineProperty API and that propagates JSBool.
Whiteboard: fixed-in-tracemonkey
Depends on: 508504
Depends on: 508512
http://hg.mozilla.org/tracemonkey/rev/459eaed1ac1f - this is a followup to fix bug 508512 caused by using ok = putActiovationObjects, not ok &= putActiovationObjects, in js_watch_set.
Comment on attachment 391873 [details] [diff] [review]
v1

>             if (str == ATOM_TO_STRING(atom)) {
>-                tinyid = ARGS_CALLEE;
>-                value = OBJECT_TO_JSVAL(fp->callee);
>+                if (TEST_OVERRIDE_BIT(fp, ARGS_CALLEE))
>+                    return true;
>+                v = INT_TO_JSVAL(fp->callee);

Sorry I missed this -- seems like OBJECT was mistyped as INT in the last + line. See bug 509301.

/be
Depends on: 509301
http://hg.mozilla.org/mozilla-central/rev/29f7e2f6319b
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Flags: wanted1.9.2+
the follow up in comment 5 is on 1.9.2 as well:

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/334ce688fe87

FTR
You need to log in before you can comment on or make changes to this bug.