Closed Bug 790349 Opened 7 years ago Closed 7 years ago

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


(Core :: JavaScript Engine, defect)

Not set





(Reporter: Waldo, Assigned: Waldo)




(5 files)

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.
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.
Attachment #660185 - Flags: review?(jorendorff)
This makes cx->names().foo work, or foo_ for keywords like delete, namespace, and so on.
Attachment #660187 - Flags: review?(jorendorff)
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.
Attachment #660188 - Flags: review?(wmccloskey)
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.
Attachment #660190 - Flags: review?(jorendorff)
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.
Attachment #660192 - Flags: review?(jorendorff)
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.
Attachment #660188 - Flags: review?(wmccloskey) → review+
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.
Attachment #660185 - Flags: review?(jorendorff) → review+
Attachment #660187 - Flags: review?(jorendorff) → review+
Attachment #660190 - Flags: review?(jorendorff) → review+
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 (!, 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.
Attachment #660192 - Flags: review?(jorendorff) → review+
Blocks: 659902
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: (billm, feel free to take a look at this if you want to see the final comment wording I landed)
Target Milestone: --- → mozilla18
No longer blocks: 659902
You need to log in before you can comment on or make changes to this bug.