Closed
Bug 1443706
Opened 7 years ago
Closed 7 years ago
Make HashString(const char16_t*) `constexpr`
Categories
(Core :: MFBT, enhancement)
Core
MFBT
Tracking
()
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: n.nethercote, Assigned: n.nethercote)
References
Details
Attachments
(1 file, 2 obsolete files)
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 2•7 years ago
|
||
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)
![]() |
Assignee | |
Comment 3•7 years ago
|
||
Ah crap. There goes my plans for statically allocated static atoms.
![]() |
Assignee | |
Comment 4•7 years ago
|
||
froydnj: do you have any idea what the timeline for dropping those compilers might be?
Flags: needinfo?(nfroyd)
Comment 5•7 years ago
|
||
Getting off gcc 4.9 is waiting for sfink to port the sixgill plugin (used for hazard builds) to a newer version of gcc.
![]() |
Assignee | |
Comment 6•7 years ago
|
||
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.
![]() |
Assignee | |
Comment 7•7 years ago
|
||
HashUntilZero() is the hard one to rewrite with a single return statement. (Or HashKnownLength() could be used instead, but it's equally hard.)
![]() |
Assignee | |
Comment 8•7 years ago
|
||
> HashUntilZero() is the hard one to rewrite with a single return statement.
Although, if recursion is allowed...
![]() |
Assignee | |
Comment 9•7 years ago
|
||
(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•7 years ago
|
||
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)
Comment 11•7 years ago
|
||
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.
![]() |
Assignee | |
Comment 12•7 years ago
|
||
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)
![]() |
Assignee | |
Updated•7 years ago
|
Attachment #8956689 -
Attachment is obsolete: true
![]() |
Assignee | |
Comment 13•7 years ago
|
||
> 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•7 years ago
|
||
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-
![]() |
Assignee | |
Comment 15•7 years ago
|
||
> 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)
![]() |
Assignee | |
Comment 16•7 years ago
|
||
> 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)
![]() |
||
Comment 17•7 years ago
|
||
(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)
![]() |
Assignee | |
Comment 18•7 years ago
|
||
> (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!
![]() |
Assignee | |
Comment 20•7 years ago
|
||
(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)
Comment hidden (mozreview-request) |
![]() |
Assignee | |
Updated•7 years ago
|
Attachment #8957012 -
Attachment is obsolete: true
![]() |
Assignee | |
Updated•7 years ago
|
Attachment #8958355 -
Flags: review?(jwalden+bmo)
![]() |
Assignee | |
Comment 22•7 years ago
|
||
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•7 years ago
|
||
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.
![]() |
Assignee | |
Comment 24•7 years ago
|
||
(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.
![]() |
Assignee | |
Updated•7 years ago
|
Comment 26•7 years ago
|
||
mozreview-review |
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-
![]() |
Assignee | |
Comment 27•7 years ago
|
||
> 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.
![]() |
Assignee | |
Comment 28•7 years ago
|
||
> 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.
Updated•7 years ago
|
Comment 29•7 years ago
|
||
mozreview-review |
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+
![]() |
Assignee | |
Comment 30•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/545fb6e48c79d2704b5e5506a667b72d97a3d949 Bug 1443706 - Introduce ConstExprHashString(const char16_t*). r=jwalden
Comment 31•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/545fb6e48c79
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in
before you can comment on or make changes to this bug.
Description
•