Make HashString(const char16_t*) `constexpr`

RESOLVED FIXED in Firefox 61

Status

()

enhancement
RESOLVED FIXED
Last year
Last year

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla61
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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
Attachment #8956689 - Flags: review?(nfroyd)
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.
Attachment #8956689 - Flags: review?(nfroyd)
Ah crap. There goes my plans for statically allocated static atoms.
froydnj: do you have any idea what the timeline for dropping those compilers might be?
Flags: needinfo?(nfroyd)
Getting off gcc 4.9 is waiting for sfink to port the sixgill plugin (used for hazard builds) to a newer version of gcc.
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.
HashUntilZero() is the hard one to rewrite with a single return statement. (Or HashKnownLength() could be used instead, but it's equally hard.)
> HashUntilZero() is the hard one to rewrite with a single return statement.

Although, if recursion is allowed...
(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.
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...
Flags: needinfo?(nfroyd)
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.
This works on GCC 4.9. I'm doing some try runs to work out if it's ok with MSVC
2015.
Attachment #8957012 - Flags: review?(nfroyd)
Attachment #8956689 - Attachment is obsolete: true
> 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 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.  :-(
Attachment #8957012 - Flags: review?(nfroyd) → review-
> 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?
Flags: needinfo?(jwalden+bmo)
> 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.
Flags: needinfo?(jwalden+bmo)
(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)
> (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!
We are going to drop support for MSVC 2015 very soon.
(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.
Flags: needinfo?(VYV03354)
Attachment #8957012 - Attachment is obsolete: true
Attachment #8958355 - Flags: review?(jwalden+bmo)
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.)
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.
(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.
Depends on: VS15.6, gcc-6.1
:glandium already answered the question.
Flags: needinfo?(VYV03354)
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.
Attachment #8958355 - Flags: review?(jwalden+bmo) → review-
> 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.
> 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.
Depends on: 1444271
No longer depends on: gcc-6.1
No longer depends on: VS15.6
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.
Attachment #8958355 - Flags: review- → review+
https://hg.mozilla.org/mozilla-central/rev/545fb6e48c79
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Depends on: 1451278
You need to log in before you can comment on or make changes to this bug.