Closed Bug 789635 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: Waldo, Assigned: Waldo)

References

Details

(Whiteboard: [js:t])

Attachments

(2 files, 1 obsolete file)

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.
Attached patch Oop, didn't qrefSplinter Review
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));
> +    JS_STATIC_ASSERT(JSTYPE_VOID == 0);
> +    return (&cx->runtime->atomState.undefinedAtom)[type];
> +}

Why have both this and TypeStrings?
Attachment #659428 - Flags: review?(jorendorff) → review+
Blocks: 791837
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.
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,
>+    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?
Depends on: 817323
Duplicate of this bug: 575416
You need to log in before you can comment on or make changes to this bug.