465377 - useless duplication of Exception native

Status: VERIFIED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Igor Bukanov]

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.

Comment 1

[Igor Bukanov]

Attachment: regression test case to assert various instanceof relations for Error objects

The test case covers various instanceof and !instanceof relations among Error objects.

Comment 2

[Igor Bukanov]

Attachment: v1

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.

Comment 3

[Igor Bukanov]

The patch shrinks stripped optimized jsexn.o by 700 bytes on Linux x64.

Comment 4

[Brian Crowder]

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?

Comment 5

[Brian Crowder]

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.

Comment 6

[Igor Bukanov]

Attachment: v2 - with autoroots

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 .

Comment 7

[Igor Bukanov]

Attachment: v2b

The new version of the patch comes with better text in new comments.

Comment 8

[Brian Crowder]

Comment on attachment 348731 [details] [diff] [review]
v2b

Nice.

Comment 9

[Igor Bukanov]

Comment on attachment 348731 [details] [diff] [review]
v2b

The change cleanups code shrinking the executable by 700 bytes.

Comment 10

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 348731 [details] [diff] [review]
v2b

a191=beltzner, assuming you've tested it out in the tracemonkey tree

Comment 11

[Igor Bukanov]

pushed to tracemonkey repo - http://hg.mozilla.org/tracemonkey/rev/4aaecbf945af

Comment 12

[Robert Sayre]

merged to mc

Comment 13

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/bd3959dce3ac

Comment 14

[Bob Clary [:bc] (inactive)]

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

Comment 15

[Bob Clary [:bc] (inactive)]

v 1.9.1, 1.9.2