789635 - JSAtomState cleanups: move atoms out, don't have manually-listed one-offs

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

This is tedious reading/coding, but it'll move us closer to being able to say cx->names().undefined rather than cx->runtime->atomState.typeAtoms[JSTYPE_VOID], which is self-evidently worth doing.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Convert jsproto.tbl to higher-order format

Continuing the trend.  Sadly jsproto.tbl is public and the contents can't really be made private until we exorcise JSProtoKey from all APIs, so I got rid of it and replaced it with a jsprototypes.h header to contain the higher-order macro (with a JS_ prefix for poor-man's namespacing).

Comment 2

[Jeff Walden [:Waldo]]

Attachment: Do the actual JSAtomState cleanups

I tried to have one higher-order macro to enumerate all cached names, but I think you run into fundamental preprocessor expansion limitations, and in any case the atoms are fully enumerated in only two places, so who really cares.

Comment 3

[Jeff Walden [:Waldo]]

Attachment: Oop, didn't qref

Comment 4

[Jason Orendorff [:jorendorff]]

Comment on attachment 659421 [details] [diff] [review]
Convert jsproto.tbl to higher-order format

Review of attachment 659421 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsxml.h
@@ +22,5 @@
> +js_InitQNameClass(JSContext *cx, JSObject *obj);
> +#else
> +#define js_InitXMLClass js_InitNullClass
> +#define js_InitNamespaceClass js_InitNullClass
> +#define js_InitQNameClass js_InitNullClass

Doesn't seem relevant to this patch. (?)

Comment 5

[Jason Orendorff [:jorendorff]]

Comment on attachment 659428 [details] [diff] [review]
Oop, didn't qref

Review of attachment 659428 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great.

::: js/src/frontend/ParseNode.cpp
@@ +406,5 @@
>  const char *
>  Definition::kindString(Kind kind)
>  {
>      static const char *table[] = {
> +        js_var_str, js_const_str, js_let_str, js_function_str, "argument", "unknown"

Do you have a note from Taras for this?

(From our earlier conversation on IRC: someone on #perf, I think Taras, once told me that declaring `const char js_unknown_str[] = "unknown";` actually affects the number of relocations that have to happen on startup.)

::: js/src/jsatom.h
@@ +163,5 @@
> +extern const char js_case_str[];
> +extern const char js_catch_str[];
> +extern const char js_class_str[];
> +extern const char js_const_str[];
> +extern const char js_construct_str[];

Pico-nit: construct is an atom, so this one's redundant.

::: js/src/jsatominlines.h
@@ +158,5 @@
> +                     JSTYPE_LIMIT * sizeof(PropertyName *) <=
> +                     sizeof(JSAtomState));
> +    JS_STATIC_ASSERT(JSTYPE_VOID == 0);
> +    return (&cx->runtime->atomState.undefinedAtom)[type];
> +}

Why have both this and TypeStrings?

Comment 6

[Jeff Walden [:Waldo]]

Pushed once to m-c, backed out to investigate a leak regression, determined patches here weren't the cause, repushed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/feee8986a75b
https://hg.mozilla.org/integration/mozilla-inbound/rev/da61c2c5d796

I'm trying to contact taras about the "" versus variable thing; I'm pretty confident "" is no different from a static const char[], enough that I landed this as-is while waiting for his feedback.  At worst I can fix anything up separately from all this, but I really really really don't think there's an issue with this.

TypeName is PropertyName*, TypeStrings is const char* -- one can't substitute for the other.

Comment 7

[Graeme McCutcheon [:graememcc]]

https://hg.mozilla.org/mozilla-central/rev/feee8986a75b
https://hg.mozilla.org/mozilla-central/rev/da61c2c5d796

Comment 8

[KENZ (Kenji Irie)]

Comment on attachment 659421 [details] [diff] [review]
Convert jsproto.tbl to higher-order format

>diff --git a/js/src/jspubtd.h b/js/src/jspubtd.h
>--- a/js/src/jspubtd.h
>+++ b/js/src/jspubtd.h
>@@ -106,8 +108,8 @@ typedef enum JSType {
> 
> /* Dense index into cached prototypes and class atoms for standard objects. */
> typedef enum JSProtoKey {
>-#define JS_PROTO(name,code,init) JSProto_##name = code,
>-#include "jsproto.tbl"
>+#define PROTOKEY_AND_INITIALIZER(name,code,init) JSProto_##name = code,
>+    JS_FOR_EACH_PROTOTYPE(PROTOKEY_AND_INITIALIZER)
> #undef JS_PROTO
>     JSProto_LIMIT
> } JSProtoKey;

It seems you missed #undef-ing PROTOKEY_AND_INITIALIZER() macro, didn't it?