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

RESOLVED FIXED in mozilla18

Status

()

RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: Waldo, Assigned: Waldo)

Tracking

Trunk
mozilla18
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [js:t])

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

6 years ago
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.
(Assignee)

Comment 1

6 years ago
Created attachment 659421 [details] [diff] [review]
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).
Attachment #659421 - Flags: review?(jorendorff)
(Assignee)

Comment 2

6 years ago
Created attachment 659422 [details] [diff] [review]
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.
(Assignee)

Comment 3

6 years ago
Created attachment 659428 [details] [diff] [review]
Oop, didn't qref
Attachment #659422 - Attachment is obsolete: true
Attachment #659428 - Flags: review?(jorendorff)
Whiteboard: [js:t]
(Assignee)

Updated

6 years ago
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+
(Assignee)

Comment 6

6 years ago
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
(Assignee)

Updated

6 years ago
Blocks: 659902
https://hg.mozilla.org/mozilla-central/rev/feee8986a75b
https://hg.mozilla.org/mozilla-central/rev/da61c2c5d796
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Comment 8

6 years ago
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?

Updated

6 years ago
Depends on: 817323
(Assignee)

Updated

3 years ago
Duplicate of this bug: 575416
You need to log in before you can comment on or make changes to this bug.