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

RESOLVED FIXED in mozilla18



7 years ago
4 years ago


(Reporter: Waldo, Assigned: Waldo)


Dependency tree / graph

Firefox Tracking Flags

(Not tracked)


(Whiteboard: [js:t])


(2 attachments, 1 obsolete attachment)

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.
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).
Attachment #659421 - Flags: review?(jorendorff)
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.
Attachment #659422 - Attachment is obsolete: true
Attachment #659428 - Flags: review?(jorendorff)
Whiteboard: [js:t]
Blocks: 790349
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. (?)
Attachment #659421 - Flags: review?(jorendorff) → review+
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));
> +    return (&cx->runtime->atomState.undefinedAtom)[type];
> +}

Why have both this and TypeStrings?
Attachment #659428 - Flags: review?(jorendorff) → review+
Pushed once to m-c, backed out to investigate a leak regression, determined patches here weren't the cause, repushed:


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.
Target Milestone: --- → mozilla18
Blocks: 659902
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,
> #undef JS_PROTO
>     JSProto_LIMIT
> } JSProtoKey;

It seems you missed #undef-ing PROTOKEY_AND_INITIALIZER() macro, didn't it?
Depends on: 817323
Duplicate of this bug: 575416
You need to log in before you can comment on or make changes to this bug.