Closed Bug 910823 Opened 7 years ago Closed 6 years ago

Make static JSClass instances const

Categories

(Core :: JavaScript Engine, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: sunfish, Assigned: sunfish)

Details

Attachments

(3 files, 4 obsolete files)

Attached patch constify-class.patch (obsolete) — Splinter Review
The JavaScript module declares numerous Class instances in static memory. These instances are read-only, so they can be marked const. As with all major const patches, this requires additional const fixes throughout the tree, so the change is unfortunately pretty invasive, and may not be desirable for that reason. I also don't know if any of the APIs affected are published APIs which shouldn't be changed. Nevertheless, here's the patch.
Attachment #797394 - Flags: superreview?(jwalden+bmo)
Comment on attachment 797394 [details] [diff] [review]
constify-class.patch

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

(In reply to Dan Gohman [:sunfish] from comment #0)
> As with all major const patches, this requires additional const fixes
> throughout the tree, so the change is unfortunately pretty invasive, and may
> not be desirable for that reason.

We're fine with large/invasive changes, as long as there aren't any busted-up intermediate states.

> I also don't know if any of the APIs affected are published APIs which
> shouldn't be changed.

We haven't promised JSAPI stability across versions in quite awhile, so there's no issue there.  If you were changing/removing a super-fundamental API in a basically-incompatible way, it might be worth discussing in the js-engine newsgroup, so JSAPI users can comment on it.  But adding const here doesn't really make life non-trivially harder for anyone, so this seems like a totally fine thing to do without needing extra process.

Broadly this looks fine, but most of the dom/plugins changes are to the constness of NPClass.  That's not JS engine, that's something you want to ask other people about getting reviews for.  (Plugin code might be maintenance-y enough they might not take the change, but I don't know.)  Get rid of those parts and this seems reasonable.

::: gfx/skia/src/xml/SkJS.cpp
@@ -149,5 @@
>      return true;
>  #endif
>  }
>  
> -JSClass global_class = {

I think you can/should revert the changes in gfx/skia -- those files aren't actually built by us, and I'd be very surprised if they even did build if someone tried, as I think they work against a pretty old SpiderMonkey.  But if these changes are actually necessary to build the browser, fine.
Attachment #797394 - Flags: superreview?(jwalden+bmo) → superreview-
Attached patch updated per review feedback (obsolete) — Splinter Review
Ok. Here's the patch without the gfx/skia and NPClass changes.
Attachment #797394 - Attachment is obsolete: true
Attachment #798171 - Flags: superreview?(jwalden+bmo)
I'm really not happy about the const_casts you're adding.
(In reply to :Ms2ger from comment #3)
> I'm really not happy about the const_casts you're adding.

Ok. There are 2 situations, and I'll attempt to clarify them:

The first is nsXBLJSClass. nsXBLJSClass is a subclass of JSClass, and the code is already doing unchecked downcasts from JSClass* to nsXBLJSClass*. Since nsXBLJSClass instances are never const, it seems reasonable to assume that if the pointee is really an nsXBLJSClass, it's really a non-const nsXBLJSClass.

The problem is that while the JSClass in each nsXBLJSClass is conceptually const, C++ doesn't have "const inheritance". We could change the JSClass from being a base to being a const member, however we still need to be able to pass around JSClass*'s and "downcast" them to nsXBLJSClass*'s. With a base object, C++ does this for us. With a member object, we'd have to write a shady offsetof hack. I think the const_casts here are preferable, but I'm open to other opinions.

The second is JSClass*'s being stored in JSValues, as "private values". These const_casts are similar to existing const_cast usage in dom/bindings/BindingUtils.cpp and js/src/jsapi.cpp. An alternative would be to add support for pointers-to-const to the JSValue encode/decode methods, however that would still perform a const_cast behind the scenes, and would still require clients to use the right decode methods, with no built-in enforcement. I'm happy to implement this alternative if you like.

If you're still not happy, then I'll just drop this patch.
Comment on attachment 798171 [details] [diff] [review]
updated per review feedback

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

sNPObjectJSWrapperClass and sNPObjectMemberClass in dom/plugins/base/nsJSNPRuntime.cpp would, I assume, still need to be const.  Same for sNPObjectJSWrapperClass in dom/plugins/base/nsJSNPRuntime.h.  Other than that, the changes as listed by Bugzilla's interdiff look good to me.

Regarding desirability of the change.  I don't think we should be catering to obscure use of JSClass via inheritance.  That really never should have been possible, and I think it's an abuse for anyone to inherit from it.

Whether via offsetof or static_cast or whatever, we're still doing slightly-dodgy pointer arithmetic no matter what, so I don't see offsetof as inherently worse than static_cast<>.  Besides, we'd only do each one place anyway (which actually seems *better* than the current situation with static_cast<>s scattered many places):

class nsXBLJSClass
{
    ...
    const JSClass*
    toJSClass()
    {
      return reinterpret_cast<const JSClass*>(uintptr_t(this) + offsetof(nsXBLJSClass, jsClass));
    }

    static nsXBLJSClass*
    fromJSClass(const JSClass* c)
    {
        return reinterpet_cast<nsXBLJSClass*>(uintptr_t(c) - offsetof(nsXBLJSClass, jsClass));
    }
};

Or somesuch.

I'm sympathetic to Ms2ger's concern.  So, a question: how much work is it to add something like that (as a preliminary patch) to nsXBLJSClass and whichever other classes inherit from JSClass currently?  If we can do that first, then make all the JSClasses const as a followup patch, that seems ideal to me.  If it's a lot of work, then I'm not sure what exactly we should do -- it's fairly afield from what you're trying to do here, so I don't really want to request it from you specifically unless it's reasonably simple to do.

I'm fine with this patch as-is.  But it's true the const_cast<>s are not pleasant, and if that keeps people happier, maybe we should do that preliminary work, then do the constifying this bug wants atop that.
Attachment #798171 - Flags: superreview?(jwalden+bmo) → superreview+
Attached patch Updated per more review feedback (obsolete) — Splinter Review
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #5)
> Comment on attachment 798171 [details] [diff] [review]
> updated per review feedback
> 
> Review of attachment 798171 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> sNPObjectJSWrapperClass and sNPObjectMemberClass in
> dom/plugins/base/nsJSNPRuntime.cpp would, I assume, still need to be const. 
> Same for sNPObjectJSWrapperClass in dom/plugins/base/nsJSNPRuntime.h.  Other
> than that, the changes as listed by Bugzilla's interdiff look good to me.

Attached is an updated patch with just these changes, for now.

> Regarding desirability of the change.  I don't think we should be catering
> to obscure use of JSClass via inheritance.  That really never should have
> been possible, and I think it's an abuse for anyone to inherit from it.

It's keeping a free list of up to 64 nsXBLJSClass instances. This code dates back to bug 42530, and unfortunately there's no discussion of why the free list is needed. I don't understand the performance characteristics of this code to evaluate it myself.

But, I can say that taking out the free list makes it relatively easy to solve the constness problem, and it significantly simplifies the code besides :-). So that's another idea, fwiw.

> Whether via offsetof or static_cast or whatever, we're still doing
> slightly-dodgy pointer arithmetic no matter what, so I don't see offsetof as
> inherently worse than static_cast<>.  Besides, we'd only do each one place
> anyway (which actually seems *better* than the current situation with
> static_cast<>s scattered many places):
> 
> class nsXBLJSClass
> {
>     ...
>     const JSClass*
>     toJSClass()
>     {
>       return reinterpret_cast<const JSClass*>(uintptr_t(this) +
> offsetof(nsXBLJSClass, jsClass));
>     }
> 
>     static nsXBLJSClass*
>     fromJSClass(const JSClass* c)
>     {
>         return reinterpet_cast<nsXBLJSClass*>(uintptr_t(c) -
> offsetof(nsXBLJSClass, jsClass));
>     }

Unfortunately, this code is still casting away the const; it's just doing so implicitly rather than explicitly.

But one thing this approach does do is centralize this conversion so that it's done in one place instead of being scattered about, as you say. So, what if we kept the conversion centralized, but implemented with const_cast, like this:

     static nsXBLJSClass*
     fromJSClass(const JSClass* c)
     {
         return const_cast<nsXBLJSClass *>(static_cast<const nsXBLJSClass *>(c));
     }


Also, do you have an opinion about the second group of const_casts? The ones for setting a JSClass* as a "private value" in a JSValue?
Attachment #798171 - Attachment is obsolete: true
This patch introduces the nsXBLJSClass::fromJSClass member function based on your suggestion above. This provides a central place for the downcast, which is still basically unchecked, though a minimal JS_ASSERT is at least possible.
Attachment #801236 - Flags: review?(jwalden+bmo)
This patch uses the nsXBLJSClass::fromClass to perform the nsXBLJSClass const_cast in a centralized location, with comments, and a minimal assert. As discussed above, this is better than using offsetof, because the offsetof hack also casts away the const qualifier.

This patch also still has the "private value" const_casts, as discussed above.
Attachment #800481 - Attachment is obsolete: true
Attachment #801237 - Flags: review?(jwalden+bmo)
And here is a patch that works even.
Attachment #801236 - Attachment is obsolete: true
Attachment #801236 - Flags: review?(jwalden+bmo)
Attachment #801838 - Flags: review?(jwalden+bmo)
Comment on attachment 801838 [details] [diff] [review]
introduce nsXBLJSClass::fromJSClass

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

::: content/xbl/src/nsXBLBinding.cpp
@@ +138,5 @@
>    return 0;
>  }
>  
> +nsXBLJSClass*
> +nsXBLService::getClass(nsCString& k)

const nsCString&, I think.

@@ +145,5 @@
> +  return getClass(&key);
> +}
> +
> +nsXBLJSClass*
> +nsXBLService::getClass(nsCStringKey *k)

Are both these methods really necessary?  Why not just have a single const nsCString& overload?
Attachment #801838 - Flags: review?(jwalden+bmo) → review+
(In reply to Dan Gohman [:sunfish] from comment #6)
> But, I can say that taking out the free list makes it relatively easy to
> solve the constness problem, and it significantly simplifies the code
> besides :-). So that's another idea, fwiw.

Maybe someone motivated to fix XBL bugs will fix it, some day.  I won't hold my breath.  :-)

> Unfortunately, this code is still casting away the const; it's just doing so
> implicitly rather than explicitly.
> 
> But one thing this approach does do is centralize this conversion so that
> it's done in one place instead of being scattered about, as you say.

Yup.  I see this (beyond the centralization benefit) of perhaps being a stepping stone to having the JSClass as a const member of the class, which would eliminate the const_cast<> for good.  One step at a time.  :-)

> So, what if we kept the conversion centralized, but implemented with
> const_cast, like this:

Fine by me.

> Also, do you have an opinion about the second group of const_casts? The ones
> for setting a JSClass* as a "private value" in a JSValue?

The constness of a private pointer is entirely up to the user, so these const_cast<>s are fine if the user is being sensible.  But they do read a little weird, to be sure.  Maybe (followup) we should change the private APIs to be template<typename T>'d, so that you could use them with any pointer type, whether const or not.
Attachment #801237 - Flags: review?(jwalden+bmo) → review+
Oh, you should put a brief note on https://developer.mozilla.org/en-US/docs/SpiderMonkey/31 about this const-ification change.  No need to change the individual pages, maybe we'll do it organically over time or something, just like we're organically updating for <stdint.h> and bool in JSAPI.
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #10)
> Comment on attachment 801838 [details] [diff] [review]
> introduce nsXBLJSClass::fromJSClass
> 
> Review of attachment 801838 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/xbl/src/nsXBLBinding.cpp
> @@ +138,5 @@
> >    return 0;
> >  }
> >  
> > +nsXBLJSClass*
> > +nsXBLService::getClass(nsCString& k)
> 
> const nsCString&, I think.

Ok.

> @@ +145,5 @@
> > +  return getClass(&key);
> > +}
> > +
> > +nsXBLJSClass*
> > +nsXBLService::getClass(nsCStringKey *k)
> 
> Are both these methods really necessary?  Why not just have a single const
> nsCString& overload?

nsCStringKey is forward-declared in nsXBLService.h; the overload allows the MOZ_ASSERT in fromJSClass to call it without any extra #includes. And it does make one of getClass' callers cleaner:

-      nsCStringKey key(xblKey);

-      c = static_cast<nsXBLJSClass*>(nsXBLService::gClassTable->Get(&key));
+      c = nsXBLService::getClass(xblKey);

The other callers are sharing nsCStringKey instances across multiple calls, so I left them as is. That said, I'm happy to change it if you prefer.
FWIW, most static JSClass instances don't qualify for .rodata, but with this patch they do make it into .data.rel.ro (on Linux).
(In reply to Dan Gohman [:sunfish] from comment #13)
> nsCStringKey is forward-declared in nsXBLService.h; the overload allows the
> MOZ_ASSERT in fromJSClass to call it without any extra #includes. And it
> does make one of getClass' callers cleaner:
> 
> -      nsCStringKey key(xblKey);
> 
> -      c = static_cast<nsXBLJSClass*>(nsXBLService::gClassTable->Get(&key));
> +      c = nsXBLService::getClass(xblKey);
> 
> The other callers are sharing nsCStringKey instances across multiple calls,
> so I left them as is. That said, I'm happy to change it if you prefer.

If by "cleaner" you mean "no prefix *", I think we're fine with the prefix * at those sites.  One overload here is better than two.
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #16)
> (In reply to Dan Gohman [:sunfish] from comment #13)
> > nsCStringKey is forward-declared in nsXBLService.h; the overload allows the
> > MOZ_ASSERT in fromJSClass to call it without any extra #includes. And it
> > does make one of getClass' callers cleaner:
> > 
> > -      nsCStringKey key(xblKey);
> > 
> > -      c = static_cast<nsXBLJSClass*>(nsXBLService::gClassTable->Get(&key));
> > +      c = nsXBLService::getClass(xblKey);
> > 
> > The other callers are sharing nsCStringKey instances across multiple calls,
> > so I left them as is. That said, I'm happy to change it if you prefer.
> 
> If by "cleaner" you mean "no prefix *", I think we're fine with the prefix *
> at those sites.  One overload here is better than two.

By cleaner, I meant "one line instead of two", and "it's like a string-to-foo map API which actually accepts a string as a key argument". I'm not sure what you mean by "prefix *".
(In reply to Dan Gohman [:sunfish] from comment #17)
> (In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #16)
> > (In reply to Dan Gohman [:sunfish] from comment #13)
> > > nsCStringKey is forward-declared in nsXBLService.h; the overload allows the
> > > MOZ_ASSERT in fromJSClass to call it without any extra #includes. And it
> > > does make one of getClass' callers cleaner:
> > > 
> > > -      nsCStringKey key(xblKey);
> > > 
> > > -      c = static_cast<nsXBLJSClass*>(nsXBLService::gClassTable->Get(&key));
> > > +      c = nsXBLService::getClass(xblKey);
> > > 
> > > The other callers are sharing nsCStringKey instances across multiple calls,
> > > so I left them as is. That said, I'm happy to change it if you prefer.
> > 
> > If by "cleaner" you mean "no prefix *", I think we're fine with the prefix *
> > at those sites.  One overload here is better than two.
> 
> By cleaner, I meant "one line instead of two", and "it's like a
> string-to-foo map API which actually accepts a string as a key argument".
> I'm not sure what you mean by "prefix *".

Oh, perhaps you're thinking this is about overloading nsCString* vs nsCString&. It's actually about overloading nsCString& vs nsCStringKey*, which is different.

In case you prefer it this way, here's a patch which undoes the getClass change and just goes ahead and adds #include "nsHashtable.h" and does what it needs to do directly.
https://hg.mozilla.org/mozilla-central/rev/fed748b27845
https://hg.mozilla.org/mozilla-central/rev/03174045ef8d
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
(In reply to Dan Gohman [:sunfish] from comment #18)
 Oh, perhaps you're thinking this is about overloading nsCString* vs
> nsCString&. It's actually about overloading nsCString& vs nsCStringKey*,
> which is different.

Yes, this is what I was misreading/misthinking as.  :-\

I might be inclined to have only the nsXBLService::getClass(nsCStringKey *k) method (maybe taking const nsCStringKey& instead), but realistically this has had enough time on it already.  So let's leave it as is now.
You need to log in before you can comment on or make changes to this bug.