1313651 - Make JS::NumberValue constexpr.

Status: NEW

(Core :: JavaScript Engine, enhancement, P3)

Description

[Tooru Fujisawa [:arai]]

separated from bug 1311088.

non-double JS::NumberValue can be made constexpr.

Comment 1

[(Away)]

Attachment: numbervalue

Is this what you had in mind?

Comment 2

[Tooru Fujisawa [:arai]]

Comment on attachment 8837428 [details] [diff] [review]
numbervalue

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

Yeah, thank you :)
there's another plan for it in bug 1313652, and I hadn't decided which way to go (sorry I should've linked it at first)

also, this may affect the binary size, since there are so many JS::NumberValue consumers in bindings.
can you check how much this change increases or reduces the binary size ?
if there's no critical issue in binary size (and performance, that I don't think it hits tho), this patch should be ready to land,
as long as this doesn't hit some compiler issue
(I saw PGO bustage caused by constexpr lately)

Comment 3

[(Away)]

> also, this may affect the binary size, since there are so many
> JS::NumberValue consumers in bindings.
> can you check how much this change increases or reduces the binary size ?
> if there's no critical issue in binary size (and performance, that I don't
> think it hits tho), this patch should be ready to land,
> as long as this doesn't hit some compiler issue
> (I saw PGO bustage caused by constexpr lately)

I got zero change with MSVC... (<aside> Recently I noticed that MSVC has trouble with NumberValue(uint32_t i) even though it's already marked constexpr. That specific overload is a big contributor to codesize because WebGL2RenderingContext.webidl has a huge table. I think maybe that function is too complex for MSVC to handle at compile-time. Also, we can't make WebGL2RenderingContext use one of the other overloads because they have some unfortunate enum values like 0xFFFFFFFF :( </aside>)

Also, that recent PGO bustage was from me, so I'm familiar with it. :-) Constexpr functions are fine; when MSVC blows up it's because of constexpr data.

Comment 4

[Tooru Fujisawa [:arai]]

(In reply to David Major [:dmajor] from comment #3)
> I got zero change with MSVC...

how about clang and gcc ?

> (<aside> Recently I noticed that MSVC has
> trouble with NumberValue(uint32_t i) even though it's already marked
> constexpr.That specific overload is a big contributor to codesize because
> WebGL2RenderingContext.webidl has a huge table. I think maybe that function
> is too complex for MSVC to handle at compile-time. Also, we can't make
> WebGL2RenderingContext use one of the other overloads because they have some
> unfortunate enum values like 0xFFFFFFFF :( </aside>)

any chance using Value::fromRawBits with raw bits helps?
of course it's not a good idea, but if it helps, there might be some point that doesn't abuse the API and also enables optimization, with maybe minor fix to JSAPI.

> Also, that recent PGO bustage was from me, so I'm familiar with it. :-)
> Constexpr functions are fine; when MSVC blows up it's because of constexpr
> data.

oh, good to hear that this won't affect the PGO issue :)

Comment 5

[Julien Cristau [:jcristau] (back August 18)]

Mass wontfix for bugs affecting firefox 52.