790349 - Make cx->names().foo a Handle<PropertyName*> for the foo atom

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

cx->runtime->atomState.fooAtom is too verbose, particularly if you need a handle version for the API you're using.  Patch series to implement this coming up, assuming wifi holds.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: 1 - Define keywords with a higher-order macro

cx->names().foo falls over if foo is a keyword, so I need keywords to have an identifier, so that I can use, e.g. cx->names().delete_ instead for them.  At the same time I might as well make them a higher-order macro.

Comment 2

[Jeff Walden [:Waldo]]

Attachment: 2 - Allow atoms to be named by cx->names().foo (or foo_ if it's a keyword)

This makes cx->names().foo work, or foo_ for keywords like delete, namespace, and so on.

Comment 3

[Jeff Walden [:Waldo]]

Attachment: 3 - Implement FixedHeapPtr<T> to wrap raw pointers and implicitly convert to Handle<T*>

Because of C++'s don't-implicitly-convert-more-than-once requirement, you can't use cx->names().lineNumber, say, where a Handle<JSAtom*> or Handle<JSString*> or whatever is expected.  This could be handled (pun intended) with more of the same SFINAE-adapted constructors for Handle and so on, like we have to make Rooted<PropertyName*> work where Handle<JSAtom*> is expected.  But this is good enough to make progress for now; we can do the other bit later.

Comment 4

[Jeff Walden [:Waldo]]

Attachment: 4 - Make JSAtomState store FixedHeapPtr<PropertyName>

cx->runtime->emptyString really should just go away now; it never really had a good reason to exist.  But the double-conversion issue mentioned with the previous patch still exists, so there's a little bit of barrier to doing so.  One improvement at a time.

Comment 5

[Jeff Walden [:Waldo]]

Attachment: 5 - Change all fooAtom uses to just foo

I also tried to get rid of pointless roots with this change, when I saw them.  Sometimes a handle still needs to stick around, but usually it could just disappear.

The goofy offset code would be better replaced with pointer-to-member, but it seemed a bit dilatory to do that everywhere; I think I only touched one place and let the rest be.

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 660188 [details] [diff] [review]
3 - Implement FixedHeapPtr<T> to wrap raw pointers and implicitly convert to Handle<T*>

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

::: js/src/gc/Barrier.h
@@ +221,5 @@
> +/*
> + * FixedHeapPtr is designed for one very narrow case: replacing immutable raw
> + * pointers to GC-managed things, implicitly converting to a handle type for
> + * ease of use.  You *really* need to know what you're doing before you use
> + * this class!

Rather than the last sentence, please explain the constraints on this thing. Namely:
- Since we don't invoke an incremental write barrier on them, they must be immutable.
- Since we don't invoke a generational write barrier on them, they must never point into the nursery.
- Since we use fromMarkedLocation, they must always be traced via MarkRuntime.

Comment 7

[Jason Orendorff [:jorendorff]]

Comment on attachment 660185 [details] [diff] [review]
1 - Define keywords with a higher-order macro

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

::: js/src/frontend/TokenStream.cpp
@@ +55,1 @@
>  };

You could delete the blank line, if you want.

::: js/src/vm/Keywords.h
@@ +39,5 @@
> +    macro(true, true_,       TOK_TRUE,       JSOP_TRUE,      JSVERSION_DEFAULT) \
> +    macro(null, null,       TOK_NULL,       JSOP_NULL,      JSVERSION_DEFAULT) \
> +    /* ES5 Keywords. */ \
> +    macro(break, break_,      TOK_BREAK,      JSOP_NOP,       JSVERSION_DEFAULT) \
> +    macro(case, case_,       TOK_CASE,       JSOP_NOP,       JSVERSION_DEFAULT) \

Suggest either lining these up again, or losing the extra spaces.

Comment 8

[Jason Orendorff [:jorendorff]]

Comment on attachment 660192 [details] [diff] [review]
5 - Change all fooAtom uses to just foo

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

In RegExp.cpp, js_InitRegExpClass:
>         return NULL;
>     proto->setPrivate(NULL);
> 
>+    RootedPropertyName empty(cx, cx->names().empty);
>     RegExpObjectBuilder builder(cx, &proto->asRegExp());
>-    Rooted<JSAtom*> empty(cx, cx->runtime->emptyString);
>     if (!builder.build(empty, RegExpFlag(0)))
>         return NULL;

It seems like you missed a few opportunities to remove roots, like here.

In frontend/Praser.cpp, Parser::functionBody:
>-    Rooted<PropertyName*> arguments(context, context->runtime->atomState.argumentsAtom);
>+    Handle<PropertyName*> arguments = context->names().arguments;

Most other places you use HandlePropertyName instead. I don't care either way.

In jsapi.cpp, JS_EnumerateResolvedStandardClasses:
>     /* Check whether 'undefined' has been resolved and enumerate it if so. */
>-    Rooted<PropertyName*> name(cx, rt->atomState.undefined);
>+    Rooted<PropertyName*> name(cx, cx->names().undefined);

Another place where you can remove a root.

In methodjit/Compiler.cpp, mjit::Compiler::constructThis():
>-        jsid id = NameToId(cx->names().classPrototype);
>+        Rooted<jsid> id(cx, NameToId(cx->names().classPrototype));

And here.

Comment 9

[Jeff Walden [:Waldo]]

Landed, then partially backed out for a leak, then partially relanded, then debugged locally and the rest landed.  Here are the changes that actually stuck for each part:

http://hg.mozilla.org/integration/mozilla-inbound/rev/a3ac2d06b2b0 (billm, feel free to take a look at this if you want to see the final comment wording I landed)
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f0affefe76f
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a5d2ad4f9e5
https://hg.mozilla.org/integration/mozilla-inbound/rev/353ca9d39c44
https://hg.mozilla.org/integration/mozilla-inbound/rev/f27c74c5ac80

Comment 10

[Graeme McCutcheon [:graememcc]]

https://hg.mozilla.org/mozilla-central/rev/a3ac2d06b2b0
https://hg.mozilla.org/mozilla-central/rev/0f0affefe76f
https://hg.mozilla.org/mozilla-central/rev/5a5d2ad4f9e5
https://hg.mozilla.org/mozilla-central/rev/353ca9d39c44
https://hg.mozilla.org/mozilla-central/rev/f27c74c5ac80