Closed Bug 465377 Opened 13 years ago Closed 13 years ago

useless duplication of Exception native

Categories

(Core :: JavaScript Engine, enhancement)

enhancement
Not set
normal

Tracking

()

VERIFIED FIXED

People

(Reporter: igor, Assigned: igor)

Details

(Keywords: testcase, verified1.9.1, Whiteboard: fixed-in-tracemonkey)

Attachments

(2 files, 2 obsolete files)

js/src/jsexn.cpp contains the following code:

/*
 * All *Error constructors share the same JSClass, js_ErrorClass.  But each
 * constructor function for an *Error class must have a distinct native 'call'
 * function pointer, in order for instanceof to work properly across multiple
 * standard class sets.  See jsfun.c:fun_hasInstance.
 */
#define MAKE_EXCEPTION_CTOR(name)                                             \
static JSBool                                                                 \
name(JSContext *cx, JSObject *obj, uintN argc, jsval *argv, jsval *rval)      \
{                                                                             \
    return Exception(cx, obj, argc, argv, rval);                              \
}

This comments are long stalled since the code does not rely on unique JSFunction.native. Instead the errors are differentiated using separated JSFunction instances. Thus the code can well use the single Exception for JsFunction.native.
The test case covers various instanceof and !instanceof relations among Error objects.
Attached patch v1 (obsolete) — Splinter Review
The patch replaces JSExnSpec tables and the corresponding natives with exception index -> JSProto linear map, eliminates unnecessary atomization of exception property names in js_InitExceptionClasses and replaces LocalRootScope API with tvr-based ones. With the latter change the unsafe LocalRootScope API is only used in jsxml.cpp and can be deoptimized for simpler code.
Attachment #348638 - Flags: review?(crowder)
The patch shrinks stripped optimized jsexn.o by 700 bytes on Linux x64.
Comment on attachment 348638 [details] [diff] [review]
v1

I could be mistaken:

Unconditionally pushed,
+    JS_PUSH_TEMP_ROOT(cx, JS_ARRAY_LENGTH(roots), roots, &tvr);

... did some stuff ...

+        if (!proto)
+            goto bad;

... jumped to ...

+  out:
+    JS_POP_TEMP_ROOT(cx, &tvr);
+    return error_proto;
+
+  bad:
+    error_proto = NULL;
+    goto out;

Jumped too far?  What about using C++? AutoTempValueRooters or whatever?  If not, isn't there a CODE_MUST_FLOW_TO hint we can give static-analysis?
Attachment #348638 - Flags: review?(crowder) → review-
Woops, I missed the bad: goto out.  Still, I think the code would be nicer with C++ rooters.  Thanks to mrbkap for the heads-up.
Attached patch v2 - with autoroots (obsolete) — Splinter Review
That was a good idea to use JSAutoTempRoot. It decreased the source complexity without any changes in the generated code. The new version of the patch also fixes more comments that refers to Exception.prototype instead of Error.prototype .
Attachment #348638 - Attachment is obsolete: true
Attachment #348730 - Flags: review?(crowder)
Attached patch v2bSplinter Review
The new version of the patch comes with better text in new comments.
Attachment #348730 - Attachment is obsolete: true
Attachment #348731 - Flags: review?(crowder)
Attachment #348730 - Flags: review?(crowder)
Comment on attachment 348731 [details] [diff] [review]
v2b

Nice.
Attachment #348731 - Flags: review?(crowder) → review+
Comment on attachment 348731 [details] [diff] [review]
v2b

The change cleanups code shrinking the executable by 700 bytes.
Attachment #348731 - Flags: approval1.9.1?
Attachment #348731 - Flags: approval1.9.1? → approval1.9.1+
Comment on attachment 348731 [details] [diff] [review]
v2b

a191=beltzner, assuming you've tested it out in the tracemonkey tree
pushed to tracemonkey repo - http://hg.mozilla.org/tracemonkey/rev/4aaecbf945af
Whiteboard: fixed-in-tracemonkey
merged to mc
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Keywords: testcase
http://hg.mozilla.org/mozilla-central/rev/b94abe190b3e
/cvsroot/mozilla/js/tests/js1_5/Error/regress-465377.js,v  <--  regress-465377.js
initial revision: 1.1
Flags: in-testsuite+
Flags: in-litmus-
v 1.9.1, 1.9.2
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.