78100 - Want mechanism for checking type of possibly-external JSStrings

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Mike Shaver (:shaver emeritus)]

Right now, we need to consult a hash table to determine if a given JSString is
of a specific external-string type (see bug 69468), and the information is Just
Sitting There for cheap access via gc_find_flags and the like.

I want to add

/*
 * Returns the external-string finalizer index for this string, or -1 if it
 * is an internal string.
 */
intN
JS_GetStringType(JSString *str);

to the API, unless someone has a better idea.

Comment 1

[Brendan Eich [:brendan]]

D'oh!  I should have added something like this along with the external string
stuff already.

/be

Comment 2

[Brendan Eich [:brendan]]

Don't forget a JSRuntime *rt param, at least (better than a cx param because
it's the specific type needed to find the GC heap and bookkeeping, and because
this is an infallible API).

/be

Comment 3

[Mike Shaver (:shaver emeritus)]

Gotcha.

Comment 4

[Mike Shaver (:shaver emeritus)]

Attachment: string typing and misc tidying

Comment 5

[Mike Shaver (:shaver emeritus)]

That diff also contains a fixed comment in jspubtd.h, which I meant to put in
with the JSRegExp_class.mark stuff.

Comment 6

[Mike Shaver (:shaver emeritus)]

Ulp.  Ignore the trailing plify_jsdhash.sed patch; I haven't fixed the rest of
the tree to use the proper PL_NewDHashTable name, though I think brendan has,
somewhere in his tree.

Comment 7

[Mike Shaver (:shaver emeritus)]

Attachment: better naming

Comment 8

[Mike Shaver (:shaver emeritus)]

Brendan and I discussed the naming, and decided to change it a little.  All else
remains the same.

Review for 0.9.1?

Comment 9

[Brendan Eich [:brendan]]

+    uint8 type = (*js_GetGCThingFlags(str)) & GCF_TYPEMASK;

Use uintN for locals and params.  Don't over-parenthesize
*js_GetGCThingFlags(str).

+uint8 *
+js_GetGCThingFlags(void *thing);
+

extern before prototype in .h files, please.

Wahhh, gratuitous comment change:

- * Function type for JSClass.mark and JSObjectOps.mark, called from the GC
- * to scan live GC-things reachable from obj's private data.  For each such
- * private thing, an implementation must call js_MarkGCThing(cx, thing, arg).
- * The trailing arg is used for GC_MARK_DEBUG-mode heap dumping and ref-path
- * tracing.
+ * Function type for JSClass.mark and JSObjectOps.mark, called from the GC to
+ * scan live GC-things reachable from obj's private data.  For each such
private
+ * thing, an implementation must call JS_MarkGCThing(cx, thing, name, arg).
+ * The trailing name and arg parameters are used for GC_MARK_DEBUG-mode heap
+ * dumping and ref-path tracing.

And it's less pretty now (more ragged-right, not consistent about filling each
line to column 79).  Thanks for the JS_ instead of js_MarkGCThing fix, though --
please undo the rest!

/be

Comment 10

[Mike Shaver (:shaver emeritus)]

I told emacs to make it fill to the 79th column, and I think it looks worse
because it did, which moved ``to'' from the second line to the first, and then
we couldn't bring ``private'' up to match.

There's more than just the JS_ change, though, and it's not gratuitous:
JS_MarkGCThing takes an additional "name" parameter, and the comment needs to
mention it as well.  I guess I can undo the complete filling of the first line,
to repair the indentation, but I think I want the rest of the comment change to
stay.

What say you?

Comment 11

[Brendan Eich [:brendan]]

Sure, sorry I missed the other wording fixes -- just go a little shy of column
79 to match the first two lines, or something.  Comment formatting AI (Emacs or
other) isn't quite up to NI yet.

/be

Comment 12

[Brendan Eich [:brendan]]

Shaver, I'm taking the liberty of moving this to 0.9.2 -- I didn't want to try
to refine your last attached patch, when you've probably already done that and
have it ready for r= and sr= in your tree.  Since you're traveling across the
milestone freeze, and since bug 69468 is targeted at 0.9.2 anyway, it seems like
we can wait a week or so.  Hope that's ok,

/be

Comment 13

[Mike Shaver (:shaver emeritus)]

Asa just told me that we shipped 0.9.2!  Who knew?

Comment 14

[Brendan Eich [:brendan]]

Argh -- we haven't landed this yet.  Let's do it straight away for 0.9.4.
shaver, should I take it (I can find my nit-picked version of your patch, or
reconstruct it) or will you do it?

/be

Comment 15

[Brendan Eich [:brendan]]

Attachment: my nit-picked version of shaver's patch

Comment 16

[Brendan Eich [:brendan]]

shaver's on the road, so I'll be checking this in.  jband, how about an r/sr=?
And remind me where this gets used in xpconnect?  The current uses of the
DOMStringTable hash table look minimal -- I don't see how
JS_GetExternalStringGCType helps remove any.  Perhaps once the patch for bug
69468 goes in?

/be

Comment 17

[John Bandhauer]

r/sr=jband

It doesn't look to me like integration changes need to be made to the current 
xpcstring code or the current patch to 69468. As I read it, he will later make 
use of this feature to avoid building wrappers around wrappers. He mentions the 
suboptimal hashtable lookup to avoid building wrappers around wrappers, but 
implements no protection for this at all in the current code or patches. I think 
he was counting on the change here and didn't bother to implement the throw away 
alternate solution.

Comment 18

[Brendan Eich [:brendan]]

Fix is in, thanks.

/be

Comment 19

[Phil Schwartau]

Marking Verified -