1443706 - Make HashString(const char16_t*) `constexpr`

Status: RESOLVED FIXED

(Core :: MFBT, enhancement)

Description

[Nicholas Nethercote [inactive]]

This requires making a bunch of other functions `constexpr` as well.

This will help with static allocation of static atoms (bug 1411469).

MozReview-Commit-ID: 3B5Zc1XYXNA

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Make HashString(const char16_t*) `constexpr`

Comment 2

[Nathan Froyd [:froydnj]]

Comment on attachment 8956689 [details] [diff] [review]
Make HashString(const char16_t*) `constexpr`

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

This use of generalized constexpr isn't supported by GCC 4.9 or MSVC 2015, both of which we still support. :(  You'd have to rewrite everything so that the constexpr functions only had a single return statement, which is probably going to get preeeeeeetty hairy.

Comment 3

[Nicholas Nethercote [inactive]]

Ah crap. There goes my plans for statically allocated static atoms.

Comment 4

[Nicholas Nethercote [inactive]]

froydnj: do you have any idea what the timeline for dropping those compilers might be?

Comment 5

[Mike Hommey [:glandium]]

Getting off gcc 4.9 is waiting for sfink to port the sixgill plugin (used for hazard builds) to a newer version of gcc.

Comment 6

[Nicholas Nethercote [inactive]]

I'm guessing MSVC 2015 will be the long pole here.

I'm vaguely contemplating the possibility of generating these hash values ahead of time (either during the build, or just once and committing them into the code somehow). Seems a bit ugly, though.

Comment 7

[Nicholas Nethercote [inactive]]

HashUntilZero() is the hard one to rewrite with a single return statement. (Or HashKnownLength() could be used instead, but it's equally hard.)

Comment 8

[Nicholas Nethercote [inactive]]

> HashUntilZero() is the hard one to rewrite with a single return statement.

Although, if recursion is allowed...

Comment 9

[Nicholas Nethercote [inactive]]

(In reply to Nicholas Nethercote [:njn] from comment #8)
> > HashUntilZero() is the hard one to rewrite with a single return statement.
> 
> Although, if recursion is allowed...

Ha! Preliminary testing indicates that it is allowed. I am experimenting.

BTW, do we use MSVC 2015 on any automation jobs? I did a test push of the attached patch and got build failures on various Linux jobs (e.g. "Bb" and "H") but I didn't get any Windows build failures.

Comment 10

[Nathan Froyd [:froydnj]]

Mike has already answered the question here.  I suspect that we can get off of MSVC 2015 faster than GCC 4.9, actually...

(In reply to Nicholas Nethercote [:njn] from comment #9)
> BTW, do we use MSVC 2015 on any automation jobs? I did a test push of the
> attached patch and got build failures on various Linux jobs (e.g. "Bb" and
> "H") but I didn't get any Windows build failures.

We don't.  We should probably set one up...

Comment 11

[Jeff Walden [:Waldo]]

Recursion's allowed in constexpr functions, but be careful about recurring too many times and hitting necessary compiler limits.

You would probably be best adding a *new* function that's constexpr and is specified to act through recursion so that people are ideally aware they should only use it on shortish strings.  (For a bit of a generous definition of shortish -- last I rememeber, gcc's recursion limit in constexpr evaluation was 512 or so.)  Adding a new limitation to HashString is probably not good for people who never gave much thought to whether it could be used on string of relatively unbounded length.

Comment 12

[Nicholas Nethercote [inactive]]

Attachment: Introduce ConstExprHashString(const char16_t*)

This works on GCC 4.9. I'm doing some try runs to work out if it's ok with MSVC
2015.

Comment 13

[Nicholas Nethercote [inactive]]

> You would probably be best adding a *new* function that's constexpr and is
> specified to act through recursion so that people are ideally aware they
> should only use it on shortish strings. 

That's exactly what I've done. And this new function is intended for use with static atoms, which are all short, e.g. 30 chars at most.

Comment 14

[Jeff Walden [:Waldo]]

Comment on attachment 8957012 [details] [diff] [review]
Introduce ConstExprHashString(const char16_t*)

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

::: mfbt/WrappingOperations.h
@@ +181,1 @@
>  WrappingMultiply(T aX, T aY)

Hate to burst a bubble, but this doesn't work with MSVC.  See the comments in mozilla::detail::WrappingMultiply::multiply.  As the comment notes, adding #pragmas to turn off the warning doesn't cut it, because the warnings show up for every place that calls the function constexpr with the wrong argument values.  If you can find a workaround, I'd love to know it -- but I'm not aware of one that can be deployed here.  :-(

Comment 15

[Nicholas Nethercote [inactive]]

> Hate to burst a bubble, but this doesn't work with MSVC.

Wait... I did a try push here that is green:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=78d38ca4927b51ae0abdef5d870a9973cd74c363

I see no sign of warning C4307 in the logs. What exactly is the problem?

Comment 16

[Nicholas Nethercote [inactive]]

> I see no sign of warning C4307 in the logs. What exactly is the problem?

Never mind, I'm getting it in a follow-up patch: https://taskcluster-artifacts.net/MKL5fO3XSI200TEFIKkq5w/0/public/logs/live_backing.log

Lots of lines like this:

> z:\build\build\src\xpcom\io\nsDirectoryServiceAtomList.h(7): warning C4307: '*': integral constant overflow

I'll try adding a macro wrapper for WrappingMultiply that pragma-disables the warning at callsites.

Comment 17

[(Away)]

(In reply to Nicholas Nethercote [:njn] from comment #16)
> I'll try adding a macro wrapper for WrappingMultiply that pragma-disables the warning at callsites.

Yes, this was going to be my suggestion.

(You may need to use the alternative form __pragma to play nice with macros: https://msdn.microsoft.com/en-us/library/d9x1s805.aspx#Anchor_3)

Comment 18

[Nicholas Nethercote [inactive]]

> (You may need to use the alternative form __pragma to play nice with macros:
> https://msdn.microsoft.com/en-us/library/d9x1s805.aspx#Anchor_3)

Thank you for the tip. Very helpful!

Comment 19

[Masatoshi Kimura [:emk]]

We are going to drop support for MSVC 2015 very soon.

Comment 20

[Nicholas Nethercote [inactive]]

(In reply to Masatoshi Kimura [:emk] from comment #19)
> We are going to drop support for MSVC 2015 very soon.

Is there a bug open for this? Thanks.

Comment 21

[Nicholas Nethercote [inactive]]

Attachment: Bug 1443706 - Introduce ConstExprHashString(const char16_t*).

This is a `constexpr` alternative to HashString(const char16_t*). We can't make
HashString(const char16_t*) itself `constexpr` because HashUntilZero(const T*)
isn't in a form that older compilers (like GCC 4.9 and MSVC 2015) allow to be
made `constexpr`. (The trick to satisfying those compilers is to use recursion
instead of iteration, to get the function into a single `return` statement.)

This requires making a bunch of other functions `constexpr` as well. It also
requires adding MOZ_{PUSH,POP}_DISABLE_INTEGRAL_CONSTANT_OVERFLOW_WARNING
macros to avoid some MSVC weirdness.

The introduction of RotateLeft5() partly undoes one of the patches from bug
1443342, but that's unavoidable.

This change will help with static allocation of static atoms (bug 1411469).

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

Comment 22

[Nicholas Nethercote [inactive]]

Waldo: I updated the patch to provide a workaround for the MSVC warnings issue, and I checked that it worked with the patch in bug 1411469.

I admit it is ugly, but I don't think it is disastrous -- because you only need to use the new macros in places where WrappingMultiply() is used in a `constexpr` context, such as the new ConstExprHashString() function added by bug 1411469. (It's not needed, for example, for the WrappingMultiply() calls that appear in the existing non-`constexpr` contexts in mfbt/tests/TestWrappingOperations.cpp.)

Comment 23

[Mike Hommey [:glandium]]

Note that bug 1424281 removed support for MSVC 2015, and bug 1444274 is not too far off. So if you can wait a bit, you could skip the workarounds.

Comment 24

[Nicholas Nethercote [inactive]]

(In reply to Mike Hommey [:glandium] from comment #23)
> Note that bug 1424281 removed support for MSVC 2015, and bug 1444274 is not
> too far off. So if you can wait a bit, you could skip the workarounds.

Excellent!

IIUC, I'll still need the macros that suppress the MSVC integer overflow warnings, but I won't need the ConstExprHashString() and ConstExprHashString() functions.

Comment 25

[Masatoshi Kimura [:emk]]

:glandium already answered the question.

Comment 26

[Jeff Walden [:Waldo]]

Comment on attachment 8958355 [details]
Bug 1443706 - Introduce ConstExprHashString(const char16_t*).

https://reviewboard.mozilla.org/r/227292/#review233266

I think I'd prefer if you introduced new (ugh) macros that offer the constexpr guarantee, incorporating `__pragma` to turn on/off the relevant warnings.  Imposing special macros on every constexpr-desiring user seems much worse than just having a separate function/macro/API to perform the task.  You'll probably have to duplicate the various detail::WrappingXHelper::compute functions as computeConstexpr, but oh well.  Adjacent implementations should make reasonably clear the correctness of the duplicated code.  And with the wrapping changes I just landed, you should only have to copy three or so lines of code for each helper to do it.

Comment 27

[Nicholas Nethercote [inactive]]

> I think I'd prefer if you introduced new (ugh) macros that offer the
> constexpr guarantee, incorporating `__pragma` to turn on/off the relevant
> warnings.

Unfortunately I don't think this will work. The warnings appear to be issued at the top of the constexpr call chain, in such a way that putting the pragmas immediately around the WrappingMultiply calls doesn't suppress them. E.g. in bug 1411469 the warnings occur at the ConstExprHashString() call sites, so the pragmas have to be placed at that level.

Comment 28

[Nicholas Nethercote [inactive]]

> E.g. in bug 1411469 the warnings occur at the ConstExprHashString() call
> sites, so the pragmas have to be placed at that level.

And I did try putting the pragmas at every conceivable place below the ConstExprHashString() call sites -- including around the entire WrappingMultiply.h file, and around the entire HashFunction.h file, as well as around individual functions -- and none of those worked.

Comment 29

[Jeff Walden [:Waldo]]

Comment on attachment 8958355 [details]
Bug 1443706 - Introduce ConstExprHashString(const char16_t*).

https://reviewboard.mozilla.org/r/227292/#review233528

Oh, right.  Ugh.  Burn everything to the ground.

Comment 30

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/545fb6e48c79d2704b5e5506a667b72d97a3d949
Bug 1443706 - Introduce ConstExprHashString(const char16_t*). r=jwalden

Comment 31

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/545fb6e48c79