400687 - Using trace kind in JSGCThingCallback

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Igor Bukanov]

Currently JSGCThingCallback takes GC thing flags as an argument. It would be nice to replace that by trace kind so the xpconnect would not need to know about GCX_TYPE constants and remap them to the trace kind on its own duplicating the code in jsgc.c

Comment 1

[Igor Bukanov]

Attachment: v1

The patch changes JSGCThingCallback to accept the trace kind, not GC flags.

To father decouples GC internals from xpconnect the patch no longer exports js_GetGCThingFlags (it is a static function in jsgc.c now) but rather defines js_GetGCThingTraceKind that returns the trace kind for the GC thing.

To simplify the mapping between GC thing types and trace kinds the patch defines GC types using the trace constants so the mapping just needs to check for external strings. To minimize the number of checks when calling the external string finalizers their type indexes start from 0, not from GCX_EXTERNAL_STRING.

Comment 2

[Brendan Eich [:brendan]]

Comment on attachment 285733 [details] [diff] [review]
v1

> JS_PUBLIC_API(JSString *)
> JS_NewExternalString(JSContext *cx, jschar *chars, size_t length, intN type)
> {
>     JSString *str;
> 
>     CHECK_REQUEST(cx);
>-    JS_ASSERT(GCX_EXTERNAL_STRING <= type && type < (intN) GCX_NTYPES);
>+    JS_ASSERT(0 <= type && type < (intN) (GCX_NTYPES - GCX_EXTERNAL_STRING));

Use (uintN)type < (uintN)(GCX_NTYPES - GCX_EXTERNAL_STRING) instead.

> JS_PUBLIC_API(intN)
> JS_GetExternalStringGCType(JSRuntime *rt, JSString *str)
> {
>-    uint8 type = (uint8) (*js_GetGCThingFlags(str) & GCF_TYPEMASK);
>+    intN type;
> 
>-    if (type >= GCX_EXTERNAL_STRING)
>-        return (intN)type;
>-    JS_ASSERT(type == GCX_STRING);
>-    return -1;
>+    type = js_GetExternalStringGCType(str);
>+
>+    /*
>+     * For compatibility we cannot return an arbitrary negative number here.
>+     */
>+    return type < 0 ? -1 : type;

Nit: parenthesize condition in ternary expression.

Wouldn't it be better to make the lowest-level abstraction normalize its r.v. so you could just propagate type here? IOW put the ternary in js_GetExternalStringGCType.

>+    return type < JSTRACE_LIMIT ? type : JSTRACE_STRING;

Nit: parenthesize conditional in ternary expression.

>+intN
>+js_GetExternalStringGCType(JSString *str)
>+{
>+    uintN type;
>+
>+    /*
>+     * Check hat for non-external strings we will return a negative number.
>+     */

Out of order, but s/hat/that/ in the comment.

>>+static uint32
>+MapGCFlagsToTraceKind(uintN flags)
>+{
>+    uint32 type;
>+
>+    type = flags & GCF_TYPEMASK;
>+    JS_ASSERT(type < JSTRACE_LIMIT || type >= GCX_EXTERNAL_STRING);
>+    return type < JSTRACE_LIMIT ? type : JSTRACE_STRING;

Nit: parenthesize condition in ternary expression.

/be

Comment 3

[Igor Bukanov]

Attachment: v2

The new patch addresses the nits and makes js_GetExternalStringGCType to return -1 as suggested.

Comment 4

[Brendan Eich [:brendan]]

Comment on attachment 286071 [details] [diff] [review]
v2

r+a=me for after M9 branches or releases.

/be

Comment 5

[Igor Bukanov]

Attachment: v3

The new version fixes debug-only newborn root names to match GCX_TYPE ordering and starts GCX_EXTERNAL_STRING from JSTRACE_LIMIT (7), not 8, to remove unused element from JSContext.nebwborn. Without the patch JSContext.nebwborn[7] is not used.

Comment 6

[Brendan Eich [:brendan]]

Comment on attachment 286160 [details] [diff] [review]
v3

Thanks. Good thing someone is paying attention :-/.

/be

Comment 7

[Igor Bukanov]

Attachment: v4

The landing of bug 401687 requires to hand-merge the patch with xpconnect changes.

Comment 8

[Brendan Eich [:brendan]]

Comment on attachment 287411 [details] [diff] [review]
v4

Still good, I hear the tree will open soon (day or two).

/be

Comment 9

[Igor Bukanov]

I checked in the patch from comment 7 to CVS trunk:

Checking in js/src/jsapi.c;
/cvsroot/mozilla/js/src/jsapi.c,v  <--  jsapi.c
new revision: 3.367; previous revision: 3.366
done
Checking in js/src/jsatom.c;
/cvsroot/mozilla/js/src/jsatom.c,v  <--  jsatom.c
new revision: 3.110; previous revision: 3.109
done
Checking in js/src/jsgc.c;
/cvsroot/mozilla/js/src/jsgc.c,v  <--  jsgc.c
new revision: 3.252; previous revision: 3.251
done
Checking in js/src/jsgc.h;
/cvsroot/mozilla/js/src/jsgc.h,v  <--  jsgc.h
new revision: 3.88; previous revision: 3.87
done
Checking in js/src/jspubtd.h;
/cvsroot/mozilla/js/src/jspubtd.h,v  <--  jspubtd.h
new revision: 3.89; previous revision: 3.88
done
Checking in js/src/jsstr.c;
/cvsroot/mozilla/js/src/jsstr.c,v  <--  jsstr.c
new revision: 3.171; previous revision: 3.170
done
Checking in js/src/jsstr.h;
/cvsroot/mozilla/js/src/jsstr.h,v  <--  jsstr.h
new revision: 3.51; previous revision: 3.50
done
Checking in js/src/xpconnect/src/nsXPConnect.cpp;
/cvsroot/mozilla/js/src/xpconnect/src/nsXPConnect.cpp,v  <--  nsXPConnect.cpp
new revision: 1.142; previous revision: 1.141
done
Checking in js/src/xpconnect/src/xpcjsruntime.cpp;
/cvsroot/mozilla/js/src/xpconnect/src/xpcjsruntime.cpp,v  <--  xpcjsruntime.cpp
new revision: 1.66; previous revision: 1.65
done
Checking in js/src/xpconnect/src/xpcprivate.h;
/cvsroot/mozilla/js/src/xpconnect/src/xpcprivate.h,v  <--  xpcprivate.h
new revision: 1.241; previous revision: 1.240
done

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

So I want to try to back this one out to see if it's the cause of the Ts regression. Unfortunately I don't have commit access to these files :(

Comment 11

[:Gavin Sharp [email: [e-mail]]]

I backed the patch from comment 7 out.

Comment 12

[Reed Loden [:reed]]

Relanded.

Checking in js/src/jsapi.c;
/cvsroot/mozilla/js/src/jsapi.c,v  <--  jsapi.c
new revision: 3.369; previous revision: 3.368
done
Checking in js/src/jsatom.c;
/cvsroot/mozilla/js/src/jsatom.c,v  <--  jsatom.c
new revision: 3.112; previous revision: 3.111
done
Checking in js/src/jsgc.c;
/cvsroot/mozilla/js/src/jsgc.c,v  <--  jsgc.c
new revision: 3.254; previous revision: 3.253
done
Checking in js/src/jsgc.h;
/cvsroot/mozilla/js/src/jsgc.h,v  <--  jsgc.h
new revision: 3.90; previous revision: 3.89
done
Checking in js/src/jspubtd.h;
/cvsroot/mozilla/js/src/jspubtd.h,v  <--  jspubtd.h
new revision: 3.91; previous revision: 3.90
done
Checking in js/src/jsstr.c;
/cvsroot/mozilla/js/src/jsstr.c,v  <--  jsstr.c
new revision: 3.174; previous revision: 3.173
done
Checking in js/src/jsstr.h;
/cvsroot/mozilla/js/src/jsstr.h,v  <--  jsstr.h
new revision: 3.53; previous revision: 3.52
done
Checking in js/src/xpconnect/src/nsXPConnect.cpp;
/cvsroot/mozilla/js/src/xpconnect/src/nsXPConnect.cpp,v  <--  nsXPConnect.cpp
new revision: 1.145; previous revision: 1.144
done
Checking in js/src/xpconnect/src/xpcjsruntime.cpp;
/cvsroot/mozilla/js/src/xpconnect/src/xpcjsruntime.cpp,v  <--  xpcjsruntime.cpp
new revision: 1.68; previous revision: 1.67
done
Checking in js/src/xpconnect/src/xpcprivate.h;
/cvsroot/mozilla/js/src/xpconnect/src/xpcprivate.h,v  <--  xpcprivate.h
new revision: 1.244; previous revision: 1.243
done