1451278 - Remove ConstExpr* variants that worked around the lack of C++14 constexpr

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P3)

Description

[Masatoshi Kimura [:emk]]

Now we can use C++14 constexpr everywhere.

Comment 1

[Nicholas Nethercote [inactive]]

Oh, there's the IsAscii stuff as well, which is outside MFBT.

Comment 2

[Masatoshi Kimura [:emk]]

Oops, I splitted the IsAscii bit right now. By the way, are you already working on? I have a patch.

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: Bug 1451278 - Remove ConstExpr hash functions.

Review commit: https://reviewboard.mozilla.org/r/233858/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/233858/

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: Bug 1451278 - Remove NS_ConstExprIsAscii() functions.

Review commit: https://reviewboard.mozilla.org/r/233860/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/233860/

Comment 5

[Nicholas Nethercote [inactive]]

emk: oh, sorry! I didn't see that comment until just now. Hopefully my patch looks the same as yours?

Comment 6

[Nicholas Nethercote [inactive]]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bcd8e01989d987268cfb6beb7f86e948eae3730d

Note that the spidermonkey-sm-rust-bindings-linux64/debug job is failing due to these changes. That's a tier 2 job, I don't know if it's a blocker or not.

Comment 8

[Masatoshi Kimura [:emk]]

Comment on attachment 8965173 [details]
Bug 1451278 - Remove ConstExpr hash functions.

https://reviewboard.mozilla.org/r/233858/#review239524

::: mfbt/HashFunctions.h:217
(Diff revision 1)
>  template<typename T>
> -uint32_t
> +constexpr uint32_t
>  HashUntilZero(const T* aStr)
>  {
>    uint32_t hash = 0;
> -  for (T c; (c = *aStr); aStr++) {
> +  for (T c = 0; (c = *aStr); aStr++) {

`for (; T c = *aStr; aStr++) {` is possible, although I'm not sure if this is better than a dummy initial value.

Comment 9

[Nathan Froyd [:froydnj]]

Comment on attachment 8965173 [details]
Bug 1451278 - Remove ConstExpr hash functions.

https://reviewboard.mozilla.org/r/233858/#review240594

::: mfbt/HashFunctions.h:217
(Diff revision 1)
>  template<typename T>
> -uint32_t
> +constexpr uint32_t
>  HashUntilZero(const T* aStr)
>  {
>    uint32_t hash = 0;
> -  for (T c; (c = *aStr); aStr++) {
> +  for (T c = 0; (c = *aStr); aStr++) {

I like Kimura-san's version slightly better than the current code or the proposed patch, even if it made me do a double-take: no questions about whether `c` is accessed uninitialized or whether a `0` character is tacked on to the beginning.  Up to you, though.

Comment 10

[Nathan Froyd [:froydnj]]

Comment on attachment 8965174 [details]
Bug 1451278 - Remove NS_ConstExprIsAscii() functions.

https://reviewboard.mozilla.org/r/233860/#review240596

::: xpcom/base/nsCRTGlue.h:107
(Diff revision 1)
>  constexpr bool
> -NS_ConstExprIsAscii(const char16_t* aString)
> +NS_IsAscii(const char16_t* aString)

Because I don't yet have a good intuition about these kind of functions, do you know what happens if the compiler can't constant-fold these away?  Do the functions always get inlined at the point of use, or do they get treated in a similar fashion to templated functions, where every compilation unit gets its own copy, and then only one gets kept at the end?  The latter seems OK, the former not so much...

Ah, the standard says "[A function or static data member declared with the `constexpr` specifier is implicitly an inline function or variable ](http://eel.is/c++draft/dcl.constexpr#1).  That doesn't seem particularly desirable in this case...

Comment 11

[Nicholas Nethercote [inactive]]

> Because I don't yet have a good intuition about these kind of functions, do
> you know what happens if the compiler can't constant-fold these away?  Do
> the functions always get inlined at the point of use, or do they get treated
> in a similar fashion to templated functions, where every compilation unit
> gets its own copy, and then only one gets kept at the end? 

My experience is that if the compiler cannot compute a constexpr expression at compile-time it gives a compile error. (I think that's what you're asking?) I.e. unlike a lot of other things (such as 'inline'), 'constexpr' is a requirement that this be evaluated at compile time, not just a suggestion.

Comment 12

[Nathan Froyd [:froydnj]]

(In reply to Nicholas Nethercote [:njn] from comment #11)
> > Because I don't yet have a good intuition about these kind of functions, do
> > you know what happens if the compiler can't constant-fold these away?  Do
> > the functions always get inlined at the point of use, or do they get treated
> > in a similar fashion to templated functions, where every compilation unit
> > gets its own copy, and then only one gets kept at the end? 
> 
> My experience is that if the compiler cannot compute a constexpr expression
> at compile-time it gives a compile error. (I think that's what you're
> asking?) I.e. unlike a lot of other things (such as 'inline'), 'constexpr'
> is a requirement that this be evaluated at compile time, not just a
> suggestion.

I don't see any way that, say, attachment 8966312 [details] [diff] [review] (bug 1452619) could work if that was the case.  Maybe we're not using the same terminology to describe what we want?

If `constexpr` meant that the compiler was *required* to do compile-time evaluation (and error if it could not), wouldn't that force us into precisely the world you're trying to remove here?  That is, we'd *have* to have separate `constexpr` functions and non-`constexpr` functions, and always use the correct one, lest we have compile-time errors.

Comment 13

[Masatoshi Kimura [:emk]]

constexpr variables are required to be evaluated at compile-time, but constexpr functions are not. If onstexpr functions are not evaluated at compile-time, it will be treated like inline functions.

Comment 14

[Nicholas Nethercote [inactive]]

Thank you for the clarification, emk!

Comment 15

[Masatoshi Kimura [:emk]]

Comment on attachment 8965174 [details]
Bug 1451278 - Remove NS_ConstExprIsAscii() functions.

https://reviewboard.mozilla.org/r/233860/#review242350

::: commit-message-5c33f:3
(Diff revision 1)
> +Bug 1451278 - Remove NS_ConstExprIsAscii() functions. r=froydnj
> +
> +MozReview-Commit-ID: 9KfPrXPYJaI

I added one reference to NS_ConstExprIsAscii here
https://hg.mozilla.org/integration/autoland/rev/4d0b4650e438#l1.43
to land bug 1445601 before this bug. Please do not forget replace it when landing this patch.

Comment 16

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5944ccd5060dc8c363b0b46bf6473348c7a09c13
Bug 1451278 - Remove ConstExpr hash functions. r=froydnj

https://hg.mozilla.org/integration/mozilla-inbound/rev/ac685df07bfc81d80f81e40d7cac55cbf56d84c1
Bug 1451278 - Remove NS_ConstExprIsAscii() functions. r=froydnj

Comment 17

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/5944ccd5060d
https://hg.mozilla.org/mozilla-central/rev/ac685df07bfc

Comment 18

[Eliza Balazs [:ebalazs_]]

Backed out 2 changesets (bug 1451278) for breaking windows pgo builds

Backout: https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=6a87ef9c1acc5f6af848c00b8b9f608444ff8a72

Comment 19

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/6a87ef9c1acc5f6af848c00b8b9f608444ff8a72

Push with jobs stuck in a retry loop: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=ac685df07bfc81d80f81e40d7cac55cbf56d84c1&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable&filter-resultStatus=success&filter-searchStr=windows%20pgo

Comment 20

[Masatoshi Kimura [:emk]]

I reopened bug 1451535 and relanded the NS_IsAscii part because it did not break PGO.

Comment 21

[Nicholas Nethercote [inactive]]

I opened bug 1480660 to land the first patch.