499049 - Replace ATOM_* macros with inline methods on JSAtom

Status: VERIFIED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Jeff Walden [:Waldo]]

JSString* JSAtom::string() { return (JSString *) (uintptr_t(this) & ~4); }
jsid JSAtom::id() { return (jsid) this; }
jsval JSAtom::value() { return (jsval) this; }

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Patch

Never knew that atoms could be doubles as well as strings, that complicates things somewhat.  const also kills things a bit, too, so all the inlines have const-duplicates that delegate to the non-const versions.  Note that the double twist is the reason for JSAtom::asString rather than JSAtom::string as originally envisioned, for parallel naming convenience -- don't see enough win to induce a difference.

Strangely, this patch *reduces* the sizes of both libmozjs.dylib and shell/js from 1268360/1232216 to 1264136/1227992.  No idea why that's happening, would have expected macro->always_inline function to be identity.

Comment 2

[Jeff Walden [:Waldo]]

Attachment: Unbitrotted, atop patch for bug 497207

Comment 3

[Jason Orendorff [:jorendorff]]

Comment on attachment 385011 [details] [diff] [review]
Unbitrotted, atop patch for bug 497207


Phew. Well, having methods where `this` is really a jsval which never points to
any C++ object at all (thanks to tag bits) isn't going to win any prizes for
C++ style.

JSAtom isn't in the public API. Can we change our atom type to be a lightweight
C++ class wrapping a jsid or jsval? It doesn't *have* to be in this bug ...but
note that such a patch will have to touch all the same lines of code. So maybe
now is the time.

>+class JSAtom {
>+  public:
>+    JS_ALWAYS_INLINE bool isDouble() const;
>+    JS_ALWAYS_INLINE bool isDouble();
>+    JS_ALWAYS_INLINE jsdouble* asDouble() const;
>+    JS_ALWAYS_INLINE jsdouble* asDouble();
>+    JS_ALWAYS_INLINE bool isString() const;
>+    JS_ALWAYS_INLINE bool isString();
>+    JS_ALWAYS_INLINE JSString* asString() const;
>+    JS_ALWAYS_INLINE JSString* asString();
>+    JS_ALWAYS_INLINE jsid id() const;
>+    JS_ALWAYS_INLINE jsid id();
>+    JS_ALWAYS_INLINE jsval value() const;
>+    JS_ALWAYS_INLINE jsval value();
>+    JS_ALWAYS_INLINE JSHashNumber hash() const;
>+    JS_ALWAYS_INLINE JSHashNumber hash();
>+};

A comment on the eyebrow-raising nature of class JSAtom would be welcome.

I'd prefer to see the bodies of all these very short inline methods here, in
the body of class JSAtom. If it won't compile that way (or you just prefer this
way), never mind.

>+bool
>+JSAtom::isDouble()
>+{
>+    return JSVAL_IS_DOUBLE(value());
>+}
> 
>+bool
>+JSAtom::isDouble() const
>+{
>+    return const_cast<JSAtom *>(this)->isDouble();
>+}

The non-const methods and const_casts seem unnecessary. Why wouldn't this work:

    bool isDouble() const {
        return JSVAL_IS_DOUBLE(value());
    }

    ...

    jsid id() const {
        JS_ASSERT(isString());
        return jsid(this);
    }

    jsval value() const {
        return jsval(this);
    }

(Offtopic, I'm kinda surprised that jsval(ptr) works; I would've guessed C++
would want a reinterpret_cast there.)

Disorganized thoughts about naming conventions going forward:

ATOM_TO_STRING is different from ATOM_TO_JSID and ATOM_KEY in that it's a
downcast, and not inherently safe. It is nice to have a quiet visual reminder
that this is happening--the "as" in atom->asString() as opposed to atom->id()
and atom->value(). And atom->castToString() would be overdoing it.

However, ISTR that in discussing bug 497768 we considered having str->asFlat()
safely return NULL if the string is not flat (in the style of dynamic_cast). I
think we should avoid using "as" for both behaviors. What do you think?

If you feel that there's no need to have a naming convention that says
"not-inherently-safe downcast", then you could use atom->string() and
atom->number().

...

In JS_PrintTraceThingInfo, can you fit that call to js_PutEscapedString on one
line?

> * JSAtom.hash assumes that JSHashNumber is 32-bit even on 64-bit systems.

I would prefer "JSAtom::hash" in this comment.  We do say things like
JSClass.getProperty, but that dates back to C days.  We should use C++ syntax,
especially for C++-only features like methods.  However if you'd prefer that I
make a separate pass and change all those at once, that's ok.

>-            JS_ASSERT(ATOM_IS_STRING(pn->pn_atom));
>-            right->pn_op = js_IsIdentifier(ATOM_TO_STRING(pn->pn_atom))
>+            JS_ASSERT(pn->pn_atom->isString());
>+            right->pn_op = js_IsIdentifier(pn->pn_atom->asString())

You can delete the assertion here, since asString() asserts.

>@@ -312,23 +310,23 @@ js_PutArgsObject(JSContext *cx, JSStackF
>     rt = cx->runtime;
>-    ok &= js_GetProperty(cx, argsobj, ATOM_TO_JSID(rt->atomState.calleeAtom),
>+    ok &= js_GetProperty(cx, argsobj, rt->atomState.calleeAtom->id(),
>                          &rval);
>-    ok &= js_SetProperty(cx, argsobj, ATOM_TO_JSID(rt->atomState.calleeAtom),
>+    ok &= js_SetProperty(cx, argsobj, rt->atomState.calleeAtom->id(),
>                          &rval);
>-    ok &= js_GetProperty(cx, argsobj, ATOM_TO_JSID(rt->atomState.lengthAtom),
>+    ok &= js_GetProperty(cx, argsobj, rt->atomState.lengthAtom->id(),
>                          &rval);
>-    ok &= js_SetProperty(cx, argsobj, ATOM_TO_JSID(rt->atomState.lengthAtom),
>+    ok &= js_SetProperty(cx, argsobj, rt->atomState.lengthAtom->id(),
>                          &rval);

These will fit on one line now.

>         JS_ASSERT(GET_INDEX(pc2) < script->atomMap.length);                   \
>         atom = atoms[GET_INDEX(pc2)];                                         \
>-        rval = ATOM_KEY(atom);                                                \
>+        rval = atom->value();                                                \
>         MATCH_CODE                                                            \
>         pc2 += INDEX_LEN;                                                     \

That backslash is totally out of line.

> Object_p_propertyIsEnumerable(JSContext* cx, JSObject* obj, JSString *str)
> {
>-    jsid id = ATOM_TO_JSID(STRING_TO_JSVAL(str));
>+    jsid id = reinterpret_cast<JSAtom *>(STRING_TO_JSVAL(str))->id();

Uh, what? Shouldn't this call js_AtomizeString? Otherwise the result is not
necessarily a valid jsid, right? I *think* you have found a bug here; if so,
let's fix it.

>+    return OBJ_CHECK_ACCESS(cx, obj, atom->id(), JSACC_READ,
>                             vp, &attrs);

This fits on one line now (in CheckCtorGetAccess; same in CheckCtorSetAccess).

> /*
>- * Callers know that ATOM_IS_STRING(atom), and we leave it to the optimizer to
>- * common ATOM_TO_STRING(atom) here and near the call sites.
>- */
>+ * Callers know that atom->isString(), and we leave it to the optimizer to
>+ * common atom->isString() here and near the call sites.
>+ */
>-#define ATOM_IS_IDENTIFIER(atom) js_IsIdentifier(ATOM_TO_STRING(atom))
>+#define ATOM_IS_IDENTIFIER(atom) js_IsIdentifier(atom->asString())
> #define ATOM_IS_KEYWORD(atom)                                                 \
>-    (js_CheckKeyword(ATOM_TO_STRING(atom)->chars(),                           \
>-                     ATOM_TO_STRING(atom)->length()) != TOK_EOF)
>+    (js_CheckKeyword(atom->asString()->chars(),                               \
>+                     atom->asString()->length()) != TOK_EOF)

Any idea what that comment is talking about? Can we delete it?

Converting these two macros to inline functions is fodder for a follow-up bug...

>-                if (!QuoteString(&ss->sprinter, ATOM_TO_STRING(atom),
>+                if (!QuoteString(&ss->sprinter, atom->asString(),
>                                  DONT_ESCAPE))

(same code in two places, in Decompile) That would fit on one line.

>-        return ATOM_TO_STRING(pn->pn_atom)->length() != 0;
>+        return !pn->pn_atom->asString()->empty();

Nice.

In jstracer.cpp:
>-        JS_ASSERT(*mTypeMap != 0xCD);
>+        JS_ASSERT(*(uint8_t*)mTypeMap != 0xCD);
and:
>- * On exit, v_ins is the incremented unboxed value, and the appropriate
>+ * On exit, v_ins is the incremented jsval value, and the appropriate

Intended for another patch, maybe?

The comment change is fine for this or any patch, if it's a correction, but as
of a66962120961 it looks like "unboxed" is correct!

>-                sprop->id == ATOM_KEY(cx->runtime->atomState.lengthAtom)) {
>+                sprop->id == cx->runtime->atomState.lengthAtom->id()) {

Ack! It was a symptomless bug, but still, nice to have it fixed.

>-        JS_ASSERT(ATOM_IS_STRING(*atomp));
>-        str = ATOM_TO_STRING(*atomp);
>+        JS_ASSERT((*atomp)->isString());
>+        str = (*atomp)->asString();

The assertion is now redundant.

r=me with these comments addressed--but if you do choose to change how JSAtom works, that would be a big enough change that I would want to re-review.

Comment 4

[Brendan Eich [:brendan]]

Igor should have a look too, he did a bunch of earlier cleanup and optimization work on atom stuff.

/be

Comment 5

[Jeff Walden [:Waldo]]

Attachment: Updated to comments

(In reply to comment #3)
> Phew. Well, having methods where `this` is really a jsval which never points
> to any C++ object at all (thanks to tag bits) isn't going to win any prizes
> for C++ style.

Ha!


> JSAtom isn't in the public API. Can we change our atom type to be a
> lightweight C++ class wrapping a jsid or jsval? It doesn't *have* to be in
> this bug ...but note that such a patch will have to touch all the same lines
> of code. So maybe now is the time.

I thought about this, but it's more than I really want to have to do at this point, seeing as it would touch at least a few hundred more lines.


> A comment on the eyebrow-raising nature of class JSAtom would be welcome.

Done.


> I'd prefer to see the bodies of all these very short inline methods here, in
> the body of class JSAtom. If it won't compile that way (or you just prefer
> this way), never mind.

Doesn't compile, jsid and jsval aren't defined at this point unfortunately.  It might be possible to fix, but it's more effort than I want to undertake just right now.


> The non-const methods and const_casts seem unnecessary. Why wouldn't this
> work:

Hm, indeed it does, I thought I'd tried that and it had failed for some reason.


> (Offtopic, I'm kinda surprised that jsval(ptr) works; I would've guessed C++
> would want a reinterpret_cast there.)

Well, it's nothing different from uintptr_t(ptr), which I'd expect to work without reinterpret given how C worked.


> ATOM_TO_STRING is different from ATOM_TO_JSID and ATOM_KEY in that it's a
> downcast, and not inherently safe. It is nice to have a quiet visual reminder
> that this is happening--the "as" in atom->asString() as opposed to atom->id()
> and atom->value(). And atom->castToString() would be overdoing it.
> 
> However, ISTR that in discussing bug 497768 we considered having str->asFlat()
> safely return NULL if the string is not flat (in the style of dynamic_cast). I
> think we should avoid using "as" for both behaviors. What do you think?
> 
> If you feel that there's no need to have a naming convention that says
> "not-inherently-safe downcast", then you could use atom->string() and
> atom->number().

I believe we'd thought maybe asFlat would assert and then cast directly to JSFlatString.  I'm not really a fan of the maybe-return-null style, would rather have an is* check and return a guaranteed-safe pointer after that.  I like your idea to have number() parallel string(), avoiding the as-prefix, changed to that.


> In JS_PrintTraceThingInfo, can you fit that call to js_PutEscapedString on one
> line?

Done.

> I would prefer "JSAtom::hash" in this comment.

Can do, no particular preference on this.

> You can delete the assertion here, since asString() asserts.

Done.


To be clear, most of the changes I did with search-and-replace, hence the occasional line-by-line nits.


> These will fit on one line now.

Done.


> That backslash is totally out of line.

Search-and-replace relic.


> > Object_p_propertyIsEnumerable(JSContext* cx, JSObject* obj, JSString *str)
> > {
> >-    jsid id = ATOM_TO_JSID(STRING_TO_JSVAL(str));
> >+    jsid id = reinterpret_cast<JSAtom *>(STRING_TO_JSVAL(str))->id();
> 
> Uh, what? Shouldn't this call js_AtomizeString?

Hmm, quite, added a trace test for it.  (Also not noticed due to s&r!)


> This fits on one line now (in CheckCtorGetAccess; same in CheckCtorSetAccess).

Done.


> Any idea what that comment is talking about? Can we delete it?

Looks vestigial, deled.


> (same code in two places, in Decompile) That would fit on one line.

Done.


> >-        return ATOM_TO_STRING(pn->pn_atom)->length() != 0;
> >+        return !pn->pn_atom->asString()->empty();
> 
> Nice.

My search-and-replace-fu was insufficient to this task, hence why it's not merely a direct translation.


> Intended for another patch, maybe?

Yeah, this was bug 497207.  Rebasing really makes a mess of patches this huge when the same file gets trashed multiple times in rebasing (and you can't test-build the intermediate phases to be sure they work completely)...


> The assertion is now redundant.

Removed.

> r=me with these comments addressed--but if you do choose to change how JSAtom
> works, that would be a big enough change that I would want to re-review.

Not now, but maybe later.  Reposting the updated version for Igor to read...

Comment 6

[Igor Bukanov]

I do not think that refactoring JSAtom is worth the efforts. The observation is that JSAtom is either interned JSString or interned double. Only the former is used for id, the interned doubles are only for scripts. So if the atom table in JSScript will be replaced by jsval table with interned values, then JSAtom can be eliminated or replaced with just a specialized thing for interned strings.

Comment 7

[Jeff Walden [:Waldo]]

So you suggest changing JSAtomMap to

struct JSAtomMap {
    jsval* vals;
    size_t length;
};

and make JSScript tracing go through that set of vals, then?  I could be in favor of that, just want to be sure what exactly you're suggesting.

Comment 8

[Igor Bukanov]

(In reply to comment #7)
> So you suggest changing JSAtomMap to
> 
> struct JSAtomMap {
>     jsval* vals;
>     size_t length;
> };
> 
> and make JSScript tracing go through that set of vals, then?

Yes, this is what I had in mind. Even better would be to make JSAtom to mean strictly interned string, but that would require some extra chnages in the compiler to factor out the usage of interned double values as atoms.

Comment 9

[Nochum Sossonko [:Natch]]

These bugs are all part of a search I made for js bugs that are getting lost in transit:

http://tinyurl.com/jsDeadEndBugs

They all have a review+'ed, non-obsoleted patch and are not marked fixed-in-tracemonkey or checkin-needed but have not seen any activity in 300 days. Some of these got lost simply because the assignee/patch provider never requested a checkin, or just because they were forgotten about.

Comment 10

[Brendan Eich [:brendan]]

All done, IIRC as part of fatvals (bug 549143).

/be

Comment 11

[Jeff Walden [:Waldo]]

Really?  This bug was about atoms, not about jsval/js::Value, seems different to me.

Comment 12

[Brendan Eich [:brendan]]

(In reply to comment #11)
> Really?  This bug was about atoms, not about jsval/js::Value, seems different
> to me.

host-2-110:src brendaneich$ hg annotate jsatom.h | grep ATOM_TO_JSID
47546: ATOM_TO_JSID(JSAtom *atom)
47546:     return JSID_BITS(id) == JSID_BITS(ATOM_TO_JSID(atom));
host-2-110:src brendaneich$ hg log -r47546
changeset:   47546:9c869e64ee26
user:        Luke Wagner <[e-mail]>
date:        Wed Jul 14 23:19:36 2010 -0700
summary:     Bug 549143 - fatvals

host-2-110:src brendaneich$ hg annotate -r47546^ jsatom.h | grep ATOM_TO_JSID
host-2-110:src brendaneich$ 
host-2-110:src brendaneich$ hg diff -r47546{^,} jsatom.h | less
 . . .
You can run these commands as well as I can :-/.

/be