Fix static constructor explosion from Bug 948638

NEW
Assigned to

Status

()

Core
JavaScript Engine
5 years ago
5 years ago

People

(Reporter: sstangl, Assigned: sstangl)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 years ago
Created attachment 8346176 [details] [diff] [review]
Make JSID_VOID and JSID_EMPTY constexpr.

Bug 948638 changed jsid to always be a struct. Per Bug 948638 Comment 5, this increased the number of static constructors. The increase appears to be in the UnifiedBindings files. This patch returns the number of static constructors back to the previous amount.
Attachment #8346176 - Flags: review?(jwalden+bmo)

Comment 1

5 years ago
Comment on attachment 8346176 [details] [diff] [review]
Make JSID_VOID and JSID_EMPTY constexpr.

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

::: js/public/Id.h
@@ +148,5 @@
>  
>  #undef id
>  
> +MOZ_CONSTEXPR_VAR jsid JSID_VOID = { size_t(JSID_TYPE_VOID) };
> +MOZ_CONSTEXPR_VAR jsid JSID_EMPTY = { size_t(JSID_TYPE_OBJECT) };

I'm not totally sure about constexpr across linkage boundaries like this.  I'd feel safer if we had

struct jsid
{
    size_t asBits;

#if !defined(_MSC_VER) && !defined(__sparc)
    // jsid must be POD so that MSVC and SPARC will pass it and return it by
    // value.  (See also bug 689101 and bug 737344, for this problem with
    // respect to JS::Value, which also has a comment like this next to it.)
  private:
#endif
    MOZ_CONSTEXPR jsid(size_t bits) : asBits(bits) {}

  public:
    static MOZ_CONSTEXPR jsid voidId() { return jsid(JSID_TYPE_VOID); }
    static MOZ_CONSTEXPR jsid emptyId() { return jsid(JSID_TYPE_EMPTY); }

    bool operator==(const jsid rhs) const { return asBits == rhs.asBits; }
    bool operator!=(const jsid rhs) const { return asBits != rhs.asBits; }
};

and then replaced all JSID_VOID with that.  A much bigger patch, to be sure, but one I think I'd feel better about.
Which compilers support constexpr at this point?

If constexpr prevents you from taking the address of a variable, and MSVC and clang at least support constexpr, I think I'd be OK with the patch as-is.
(Assignee)

Comment 3

5 years ago
According to http://msdn.microsoft.com/en-us/library/hh567368.aspx, MSVC doesn't support constexpr.

Comment 4

5 years ago
MSVC is better about doing constexpr-ing of simple-enough non-default constructors, that it usually isn't a problem on the static initializer front.  However, if it's using JSID_VOID, that just *is* a reference, and I'm not aware that -- even if it's const -- MSVC can see through that to inline/pre-initialize from the numbers that would be written from the const data.
(Assignee)

Comment 5

5 years ago
Created attachment 8346248 [details] [diff] [review]
jsid::voidId() and jsid::emptyId()

This is the patch from Waldo's suggestion. I haven't been able to test it just yet -- just leaving it here so I don't forget.
Yeah, this approach is definitely unobjectionable.

>+    MOZ_CONSTEXPR jsid(size_t bits) : asBits(bits) {}

Please make this constructor explicit or private.
I think we should name them VoidId and EmptyID. Eventually we should think about putting them in JS:: so they can be used by the browser like their cousins JS::UndefinedValue.

Comment 8

5 years ago
Except that we want to get rid of jsid entirely, eventually, and replace it with JS::PropertyKey, so it's not really worth polishing.  :-)

Updated

5 years ago
Attachment #8346176 - Flags: review?(jwalden+bmo)
You are right! Sorry, I forgot about that.
(Assignee)

Comment 10

5 years ago
Created attachment 8347474 [details] [diff] [review]
jsid::VoidID() and jsid::EmptyId().

Verified that this fixes the explosion.
Attachment #8346176 - Attachment is obsolete: true
Attachment #8346248 - Attachment is obsolete: true
Attachment #8347474 - Flags: review?(jwalden+bmo)
Comment on attachment 8347474 [details] [diff] [review]
jsid::VoidID() and jsid::EmptyId().

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

Looks fine, except for the =default bit.  Does this actually compile on Windows as it is here?

::: js/public/Id.h
@@ +37,5 @@
>  struct jsid
>  {
>      size_t asBits;
> +
> +    jsid() = default;

Uh, this isn't supported with MSVC until MSVC 2013.  Are you certain this doesn't need ifdefs of some sort?

@@ +49,5 @@
> +    MOZ_CONSTEXPR jsid(size_t bits) : asBits(bits) {}
> +
> +  public:
> +    static MOZ_CONSTEXPR jsid VoidId() { return jsid(JSID_TYPE_VOID); }
> +    static MOZ_CONSTEXPR jsid EmptyId() { return jsid(JSID_TYPE_OBJECT); }

Add a #include "mozilla/Attributes.h", please, rather than bootlegging it.
Attachment #8347474 - Flags: review?(jwalden+bmo) → review+
Oh, the canonical casing would be voidId and emptyId.
I backed this out in http://hg.mozilla.org/integration/mozilla-inbound/rev/1c6081f57d57 on the theory that this patch somehow broke every single b2g test.

The tests were passing on the previous push, but starting with this bug's push, every single reftest started failing like this: https://tbpl.mozilla.org/php/getParsedLog.php?id=32059136&tree=Mozilla-Inbound and mochitests all (except m5?) started failing like this: https://tbpl.mozilla.org/php/getParsedLog.php?id=32059625&tree=Mozilla-Inbound
(In reply to Wes Kocher (:KWierso) from comment #14)
> I backed this out in
> http://hg.mozilla.org/integration/mozilla-inbound/rev/1c6081f57d57 on the
> theory that this patch somehow broke every single b2g test.
> 
> The tests were passing on the previous push, but starting with this bug's
> push, every single reftest started failing like this:
> https://tbpl.mozilla.org/php/getParsedLog.php?id=32059136&tree=Mozilla-
> Inbound and mochitests all (except m5?) started failing like this:
> https://tbpl.mozilla.org/php/getParsedLog.php?id=32059625&tree=Mozilla-
> Inbound

Everything seems to have started working again with this backed out, so that theory is correct...
(Assignee)

Comment 16

5 years ago
It almost looks like B2G processes started dying from OOM. This should be fun.
You need to log in before you can comment on or make changes to this bug.