899309 - modify JS::Value and some helper functions to be constexpr-foldable

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: modify JS::Value and some helper functions to be constexpr-foldable

The WebIDL bindings include a lot of code that looks roughly like this:

static const struct {
  const char* c;
  jsval v;
} constant_defs[] = {
  ...
  { "foo", UINT_TO_JSVAL(42) },
  ...
};

Depending on the compiler, those UINT_TO_JSVAL calls either fold down to
constant bit patterns or they result in static constructors.  The static
constructors can be rather large and are quite pointless.  I'd like to
provide a helping hand for compilers that generate static constructors
by letting them know that several of the *_TO_JSVAL functions are
actually constexpr and thus it can fold things down to constant data
instead.

This patch is the result.  It is less ugly than I thought it would be, though it's
not the cleanest thing ever.  But the basic idea is simple:

1. Make the appropriate *_TO_JSVAL functions constexpr;
2. Iterate through functions they call, making them constexpr too;
3. Repeat step 2 until done.

Seeking feedback on whether this is a reasonable thing to do and whether
there's a more appropriate avenue to follow here.

Comment 2

[Luke Wagner [:luke]]

Comment on attachment 782780 [details] [diff] [review]
modify JS::Value and some helper functions to be constexpr-foldable

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

Great!  I can't wait until we can use constexpr unconditionally.  (Do you know when that will be by chance?)

::: js/public/Value.h
@@ +355,5 @@
> +/*
> + * We need at least 4.6 for constexpr; 4.5 supports defaulted functions
> + */
> +#  if MOZ_GCC_VERSION_AT_LEAST(4, 6, 0)
> +#    define JS_VALUE_SUPPORTS_CONSTANT_FOLDING

"supports constant folding" is a bit confusing; how about JS_VALUE_IS_CONSTEXPR ?

@@ +902,5 @@
>       */
> +#if defined(JS_VALUE_SUPPORTS_CONSTANT_FOLDING)
> +    Value() = default;
> +    Value(const Value& v) = default;
> +    JS_VALUE_CONSTEXPR Value(jsval_layout layout) : data(layout) {}

1. I know we can assume some basic level of C++11 everywhere now (e.g., I think we can use 'auto'); is the "= default" syntax available everywhere s.t. it doesn't need to be #ifdef'd?

2. I'd rather keep Value(jsval_layout) a private member (note friendship with IMPL_TO_JSVAL below).

Comment 3

[Nathan Froyd [:froydnj]]

(In reply to Luke Wagner [:luke] from comment #2)
> Great!  I can't wait until we can use constexpr unconditionally.  (Do you
> know when that will be by chance?)

Not sure.  It's not supported in MSVC 2012 and it's in the "planned" subset of features for the next versions of MSVC, which places it in 2014 at the earliest.  (And when we start requiring that is another matter...)

> @@ +902,5 @@
> >       */
> > +#if defined(JS_VALUE_SUPPORTS_CONSTANT_FOLDING)
> > +    Value() = default;
> > +    Value(const Value& v) = default;
> > +    JS_VALUE_CONSTEXPR Value(jsval_layout layout) : data(layout) {}
> 
> 1. I know we can assume some basic level of C++11 everywhere now (e.g., I
> think we can use 'auto'); is the "= default" syntax available everywhere
> s.t. it doesn't need to be #ifdef'd?

No.  Not coming to MSVC until later this year at the earliest.

> 2. I'd rather keep Value(jsval_layout) a private member (note friendship
> with IMPL_TO_JSVAL below).

Good point, I'll do that.

Comment 4

[Nathan Froyd [:froydnj]]

Attachment: modify JS::Value and some helper functions to be constexpr-foldable

Applied review feedback.  Try builds on the major platforms seem OK, should probably
do a sanity check on Android and B2G just to make sure:

https://tbpl.mozilla.org/?tree=Try&rev=cb67b87603ff

I'm not sure that I like having JS_VALUE_CONSTEXPR_VAR *and* MOZ_CONSTEXPR_VAR, but
doing things this way means that if MSVC does implement constexpr without implementing
defaulting constructors, things will still continue to work, rather than blowing up
mysteriously...I think.

Comment 5

[Nathan Froyd [:froydnj]]

Comment on attachment 783219 [details] [diff] [review]
modify JS::Value and some helper functions to be constexpr-foldable

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

::: js/public/Value.h
@@ +419,5 @@
>  INT32_TO_JSVAL_IMPL(int32_t i)
>  {
> +#if defined(JS_VALUE_IS_CONSTEXPR)
> +    return (jsval_layout) { .s.tag = JSVAL_TAG_INT32,
> +                            .s.payload.i32 = i };

I did not do an excellent job of testing the patch on 32-bit platforms; when I did, this gives syntax errors.  What this really ought to be is:

{ .s = { .tag = JSVAL_TAG_INT32, .payload = { .i32 = i } } }

Except GCC says "sorry, complex designated initializers are not supported."  So that's out.

How about using:

return BUILD_JSVAL(JSVAL_TAG_INT32, i);

unconditionally for this?  AFAICS, the conversion int32_t -> uint32_t is well-defined, so this shouldn't introduce problems.

Comment 6

[Nathan Froyd [:froydnj]]

Pinging Luke for comments on comment 5.

Comment 7

[Luke Wagner [:luke]]

Sounds good.

Comment 8

[Nathan Froyd [:froydnj]]

Apparently some evil force does not want this patch landed.  Yesterday's try builds (linux64, os x, win32):

https://tbpl.mozilla.org/?tree=Try&rev=a312fd280294

Today's try builds with the *exact same patch* on 64-bit, slightly modified to compile on 32-bit GCC:

https://tbpl.mozilla.org/?tree=Try&rev=6c805cfe771b

Now linux64 ICEs, along with linux b2g and android debug only (but not opt).  I do *not* understand this.

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1adc4b65b54b