Closed Bug 1374631 Opened 8 years ago Closed 7 years ago

Switch generated data to "constexpr" instead of "const" where possible

Categories

(Toolkit :: Telemetry, enhancement, P3)

Product:
Toolkit
Component:
Telemetry
Type:
enhancement
Points:
1

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: gfritzsche, Assigned: so61pi.re, Mentored)

References

(Blocks 1 open bug)

Details

(Whiteboard: [measurement:client] [lang=c++] [good next bug])

Attachments

(1 file, 2 obsolete files)

Bug 1374631 - Switch telemetry generated code to constexpr instead of const. r?chutten
46 bytes, text/x-phabricator-request
chutten
: review+
Details | Review
With bug 1374419 solved, there are still multiple places where we generate C++ data as "const" arrays from Python scripts, which could be "constexpr": https://dxr.mozilla.org/mozilla-central/search?q=path%3Atoolkit%2Fcomponents%2Ftelemetry+ext%3Apy+regexp%3A%22const+%22&redirect=false Changing those to `constexpr` doesn't have any downside for us, as long as it builds.
Summary: Improve static_asserts in TelemetryHistogram.cpp → Switch generated data to "constexpr" instead of "const" where possible
Hello Georg! I am new to open source development and would like to take on this bug. I think I have the required information for building mozilla on my system and running unit tests. Could you assign me to the bug so I can get started?
I'm not Georg, but I do an adequate impression of him when he's unavailable :)
Assignee: nobody → jain98
Thanks Chris!
Hey Chris, while building mozilla I'm getting an error that says "*** MSYS make is not supported. Stop." I tried searching online but did not get a clear answer. One of the answers said something about MSYS not recognizing the make command. Would you know how I can fix this?
Flags: needinfo?(chutten)
Was that comment meant for me?
MSYS... I guess that means you're building on Windows? For that you would benefit from using the mozillabuild shell as documented in the getting started directions: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Windows_Prerequisites#Installing_the_build_prerequisites Once you have it installed you need to be within the mozilla build shell before you run `mach build`. There should be shortcuts in your mozilla-build install folder. If you have further problems please do let me know. If you are on IRC and would like more prompt assistance, there are people on the #introduction channel that would be happy to help you in real time: https://wiki.mozilla.org/Irc
Flags: needinfo?(chutten)
I was actually using mozilla build shell when I got this error.
Hmm, I'm afraid I don't know what might be causing that. All I can find about that is some ancient bugs that likely aren't terribly relevant (such as bug 1074956 which recommends exporting MAKE=mozmake and ensuring you have the latest version of mozilla build)...
I did see that bug earlier. I'll give it another shot today.
This bug didn't see activity for a while. I'll unassign, this is free to work on now.
Assignee: jain98 → nobody
I'm willing to take up this bug, I've already submitted a few patches and my build works fine. As I see it, all I need to do is change all the array const declarations to constexpr. Anything else?
Flags: needinfo?(chutten)
I'm sorry for not picking up on this earlier, Vedant! Yes, that should be about right. So long as it compiles and passes tests, const->constexpr within the python files in toolkit/components/telemetry are what we're asking for. Are you still wishing to work on this?
Flags: needinfo?(chutten) → needinfo?(vedantc98)
Definitely. I'll submit a patch, and meanwhile I can assign myself if that's not a problem?
Flags: needinfo?(vedantc98)
Comment on attachment 8912785 [details] Bug 1374631 - Changed const to constexpr in generated data. https://reviewboard.mozilla.org/r/184104/#review189286 The code looks good! I've sent this patch to our try servers to test that it builds and works on a variety of platforms. Once the results are in, we can see about getting this landed.
Attachment #8912785 - Flags: review?(chutten) → review+
The try push will publish its results here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7f8af7b50dd0ba0b25379edfb1ed4b5502c0d375
Assignee: nobody → vedantc98
Status: NEW → ASSIGNED
Hrm. Turns out the trychooser in mozreview allows me to specify invalid platform designations. Whoops. Here's a retry: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2734e02c8739ee3110111fadf3d6d31c03f3491e
Looks like we're getting some build failures. Seems as though static constexpr char* constexpr ProcessIDToString[5] = { in particular is causing some problems. (see, for example, https://treeherder.mozilla.org/logviewer.html#?job_id=133615609&repo=try&lineNumber=8881 ) When you performed your local test build, did this not come up?
Flags: needinfo?(vedantc98)
The build worked locally, I don't see why it's failing on the try server. I'll run another local build on that branch, but it shouldn't return anything different.
Flags: needinfo?(vedantc98)
Changed generated data to `constexpr` instead of `const` wherever possible. Couldn't switch to constexpr in the following lines: [1] At https://dxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/gen-process-data.py#49, the compiler throws a warning as follows: " ISO C++11 does not allow conversion from string literal to 'char *const' ". [2] At https://dxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/gen-scalar-data.py#65, the build breaks, since a variable defined with constexpr cannot be of a non-literal type.
Pushed by chutten@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/986214ae3837 Changed const to constexpr in generated data. r=chutten
Backout by archaeopteryx@coole-files.de: https://hg.mozilla.org/integration/autoland/rev/a3e747cce4f2 Backed out changeset 986214ae3837 on suspicion of causing xpcshell bustage in Windows builds. r=backout on a CLOSED TREE
Sorry, this had to be backed out on suspicion of causing xpcshell bustage in Windows builds: https://hg.mozilla.org/integration/autoland/rev/a3e747cce4f21b760d005c9879f0dd7ccf48d129 Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=986214ae38377d90e0a0534668c6be08e3ca87da&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable Failure log example: https://treeherder.mozilla.org/logviewer.html#?job_id=138201907&repo=autoland ..\testing\xpcshell\selftest.py::XPCShellTestsTests::testAddTaskSkip TEST-UNEXPECTED-FAIL ..\testing\xpcshell\selftest.py::XPCShellTestsTests::testAddTaskSkipAll TEST-UNEXPECTED-FAIL ..\testing\xpcshell\selftest.py::XPCShellTestsTests::testAddTaskStackTrace TEST-UNEXPECTED-FAIL etc. If it is revealed to be from a different bug or issue, I will comment in this bug.
Flags: needinfo?(vedantc98)
I presume that, since we've heard nothing in a few days, this did indeed appear to be the problem with the Windows builds?
Flags: needinfo?(aryx.bugmail)
No, this issue hasn't hit again, so it really seems to be triggered by the code for this bug.
Flags: needinfo?(aryx.bugmail)
Chris, do you have any idea what could be causing this problem? The try build ran fine, and so did my local build.
Flags: needinfo?(vedantc98)
Can you point me to the try build that succeeded? The one attached to mozreview wasn't the most recent, I think.
Flags: needinfo?(vedantc98)
I can't find the link to the previous try build, is there any way to look for previous try requests on treeherder?
Flags: needinfo?(vedantc98)
Here's the try build link: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9f0acf36acee6fb641205359532c173bba3fd521 It does fail on windows builds, but I think we assumed it was due to some other code.
Ah yes, I remember that discussion now. "I/O operation on a closed file" is certainly not related. It seems as though I shouldn't have blamed that for the test failures in xpcshell selftest that followed. For now, to get this patch landed, we can echo compiler guards around the constexpr code to ensure the plain const syntax is used on Windows. We can then file a follow-up bug to investigate why we can't use constexpr on Windows... or, at least, why we can't use it in these places. #ifdef XPWIN <the old const stuff> #else <the new constexpr stuff> #endif
Do you have the time to make this change? Have you gotten stuck and can I help?
Flags: needinfo?(vedantc98)
Popping this back into the Mentored pool. Steps to solve this bug: 0) Volunteer here on the bug so I can answer any questions and assign the bug to you 1) Dig into the existing patch and change it to only use constexpr on non-Windows builds 2) Post your patch up here so we can get it reviewed and I can push it to try so it can be built on all of our supported platforms 3) File a new bug for figuring out why we can't use constexpr in this way on Windows
Assignee: vedantc98 → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(vedantc98)
Hiya Chris, I'm looking to take this on (take on this). A couple of questions to start: Is this the most recent patch? [ref comment 21] https://reviewboard.mozilla.org/r/184104/diff/3/ Am I okay to work from that? (i.e pulling it and attempting to build) I'm building on Linux, there seems to have been some Windows specific build errors. If the most recent patch builds fine for me, am I okay to simply continue, or is the stuff referenced in your comment (33) something that needs doing? Cheers.
Please do pull it down to work on! I've assigned the bug to you. Since the build failures only show on Windows, you can either find a windows VM to build on (painful, slow) or post your patches to mozreview where I can push them to build on our windows build machines using our try servers (less painful, but I may be not terribly quick about it myself :S). Either way, the strange Windows behaviour needs to either be worked around or addressed. Without a local Windows build, and a Windows Expert to hold our hand, I don't hold out much hope of figuring out _why_ it's broken, so I'll settle for leaving Windows behind for now and filing a follow-up bug to address the deficiency later. Sound good?
Assignee: nobody → samathy
Status: NEW → ASSIGNED
I've pulled down the previous work done by Vedant and finished patching all the occurrences of 'const' in the tool kit/components/telemetry python files. Building succeed. I didn't build from scratch, I've built FF before in this directory. Will the changes to the python files be reflected my build, or do I need to clobber and rebuild from scratch? I'm hoping the build system is clever enough to have executed the scripts and rebuilt the components affected, but I'm not certain. Are there any more Python scripts that need editing as part of this bug, or just those in toolkit/components/telemetry ?
Changes to the python files ought to be reflected in an incremental build, but it's probably best to double check with a clobber in this case. I know we had some problems around the build not recognizing them properly in the past. Only the ones in toolkit/components/telemetry are relevant to this bug, so if you've guarded those against Windows and it builds, please submit your patch for review so I may push it up to try and check if it builds on Windows.
Heres the patches including the #ifdef blocks. Builds fine on Linux.
Awesome. I've sent it to build on non-Linux platforms and to run the unit tests. You can follow along in this rather obscure interface over here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3a0e7811617e2e37dcf4bdca0a9fb3363a5c6d16 (The tl;dr is that green is good and orange or red is bad) If it comes out clean (or clean except for intermittent failures unrelated to your patch) I'll give this a proper review and we'll get it landed!
Oh, hey, looks like we have some linting errors. We pass changed python files through flake8 (you can run it locally using ./mach lint -wo), and this time it seems to be tripping on the long lines (https://treeherder.mozilla.org/#/jobs?repo=try&revision=3a0e7811617e2e37dcf4bdca0a9fb3363a5c6d16&selectedJob=150577895) I'm not sure if the build will continue with the style errors or not. Can you provide a patch that passes linting? I'm sorry I didn't mention this before.
Also, it looks as though you were using XPWIN as the define name. The define that identifies windows is XP_WIN (with an underscore). I have stopped the try builds for now and await your updated patch. We're nearly there, I think!
Updated the formatting to pass linter checks and corrected XPWIN to XP_WIN. Apologies for the double commit.
Attachment #8912785 - Attachment is obsolete: true
Looks like its still failing on Windows. A quick Google of the error shows MSVC doesnt like the variable length arrays. https://stackoverflow.com/questions/33423502/expression-did-not-evaluate-to-a-constant-c#33423538 That sounds about right, gen_histogram_data.py spits out some variable length arrays. Did this build on Windows okay before we started changing to constexpr? Or did const fail too?
The static asserts are concerned with gHistogramInfos which has been constexpr since bug 1374419. Your patch remove the constexpr-ness from gHistogramInfos on Windows, which is the cause of the compile error. Anything that is currently constexpr should remain constexpr, as it is currently building without error on Windows. It's some of these other things that complain on Windows if we make them constexpr. And I'm not sure which ones they are. I think we're getting closer. I think we can get this working with a few small changes: 1) Return gHistogramInfos to being constexpr and without #ifdef compile guards. 2) Squash all of your commits together into one 3) Submit that one commit for review.
Whats the best way of squashing several commits together? Apparently there isnt one canonical command to do that with mercurial. Bearing in mind I have several 'bookmarks' which have been committed too.
Use `hg histedit`. It is the rough equivalent of `git rebase -i`. If you have a modern version of Mercurial, running `hg histedit` will select an appropriate range of commits for editing automatically. If you have an older version, you may need to manually specify the base changeset to edit. If you run `mach mercurial-setup`, it will prompt you to enable `hg wip` or `hg show work`. Either of these can be used to quickly find the base changeset to edit.
I think thats what we want. Its certainly squashed the commits, I just hope I ended up with the right ones!
Comment on attachment 8935483 [details] Bug 1374631 - Switch generated data to "constexpr" instead of "const" where possible https://reviewboard.mozilla.org/r/206384/#review212550 ::: toolkit/components/telemetry/gen_event_enum.py:75 (Diff revision 4) > if cpp_guard: > print("#endif", file=output) > > print("};\n", file=output) > > - print("const uint32_t EventCount = %d;\n" % index, file=output) > + print("#ifdef XP_WIN\nconstexpr uint32_t EventCount = %d;\n#else\n\ You've put a constexpr inside the XP_WIN #ifdef. Is this on purpose? (in this case, a single uint32_t, it should be fine) ::: toolkit/components/telemetry/gen_histogram_data.py:163 (Diff revision 4) > # This generates static data to avoid costly initialization of histograms > # (especially exponential ones which require log and exp calls) at runtime. > # The format must exactly match that required in histogram.cc, which is > # 0, buckets..., INT_MAX. Additionally, the list ends in a 0 to aid asserts > # that validate that the length of the ranges list is correct.U cache miss. > - print("const int gHistogramBucketLowerBounds[] = {", file=output) > + print("#ifdef XP_WIN\nconst int gHistogramBucketLowerBounds[] = {\n#else\n\ You didn't change the non-windows decl to be constexpr ::: toolkit/components/telemetry/gen_histogram_data.py:185 (Diff revision 4) > > if offset > 32767: > raise Exception('Histogram offsets exceeded maximum value for an int16_t.') > > - print("const int16_t gHistogramBucketLowerBoundIndex[] = {", file=output) > + print("#ifdef XP_WIN\nconst int16_t gHistogramBucketLowerBoundIndex[] = {\n#else\n\ > +const int16_t gHistogramBucketLowerBoundIndex[] = {\n#endif", file=output) Ditto here, it is const whether it's windows or not. not-windows should be constexpr, no? ::: toolkit/components/telemetry/gen_process_data.py:50 (Diff revision 4) > for i, (name, value) in enumerate(processes.iteritems()): > p(" /* %d: ProcessID::%s = */ %s," % (i, to_enum_label(name), value['gecko_enum'])) > p("};") > p("") > - p("static const char* const ProcessIDToString[%d] = {" % len(processes)) > + p("#ifdef XP_WIN\nstatic const char* const ProcessIDToString[%d] = {\n#else\n\ > +static const char* const ProcessIDToString[%d] = {\n#endif\n" % (len(processes), len(processes))) And here too ::: toolkit/components/telemetry/shared_telemetry_utils.py:96 (Diff revision 4) > return "'\\''" > else: > return "'%s'" % s > return ", ".join(map(toCChar, string)) > > - f.write("const char %s[] = {\n" % name) > + f.write("#ifdef XP_WIN\nconstexpr char %s[] = {\n#else\nconstexpr char %s[] = {\n#endif\n" This may succeed in a Windows build, but if it's constexpr in both windows and not-windows cases, we don't need the cases.
For fun I've thrown this at try to see if the windows constexprs will succeed or not. We'll find out! https://treeherder.mozilla.org/#/jobs?repo=try&revision=c0de3c55b8fddd21e880b02a9a0c219e36c798e7
Gah, I must have un-done earlier work when copy-pasting stuff for the XP_WIN #ifdefs. Incoming commit again! This one is correct, I promise ;)
I believe you :) I'm at my end-of-day, which is also my end-of-week, so I might not be able to check in on this until Monday... which is when I board a plane to go to the Austin All Hands (a Mozilla conference and work week, essentially). Which is to say I might be a bit delayed getting back to this :S I have set myself a ni? so I'll be nagged by bugzilla if I am more than a little delayed. In the meantime, browse around see if you find any other bugs you'd like to start working on in the meantime? :)
Flags: needinfo?(chutten)
No worries, I'm not in a rush and I've got a couple of other bugs on the go anyway. Enjoy your weekend and the All Hands!
Comment on attachment 8935483 [details] Bug 1374631 - Switch generated data to "constexpr" instead of "const" where possible https://reviewboard.mozilla.org/r/206384/#review213246 Looks good and green on try. Let's land this.
Attachment #8935483 - Flags: review+
Pushed by chutten@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/49f68b1f4c2e Switch generated data to "constexpr" instead of "const" where possible r=chutten
Blocks: 1425073
This is on its way to landing... for good this time, I trust :) I've filed bug 1425073 to look into why the first attempt failed.
Flags: needinfo?(chutten)
It was green on try, not sure what changed. Samathy, you can leave this with me. I now have a Windows build set up and I should be able to find out what is going on early next week.
Flags: needinfo?(chutten)
Okay. Thanks a lot for sticking with me Chris, you've been really helpful.
Ohhh, now I see. It's everything _but_ the Windows build that's failing this time. It looks like it's having problems around the constexpr in gen_process_data.py. Specifically static constexpr char* const ProcessIDToString I think we can leave that one to the follow-up bug as well. Samathy, do you want to submit a version of the patch with ProcessIDToString left the way it currently is?
Flags: needinfo?(samathy)
I won't be able to do that until mid january since I'm away from my build machine. I'll pick it back up when I get back. Leaving the needinfo in the hope that I get reminded about it until its done.
Hiya, I'm back from my break. Reopened the review request with 'static constexpr char* const ProcessIDToString' set back to the way it was for all platforms.
Welcome back! Looks good from here, I've sent it to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=95964e9cbd45061c0bcee0ca00580f4d6009afb8 Did you perchance rebase your work atop the latest code from mozilla central? When we submit it, autoland might reject your patch if it has to do merge resolution.
constexpr does not imply const (since C++14). So `static constexpr char* const` should have been `static constexpr const char* const`.
:emk, thank you for that insight! I'll just go ahead and flag you for r? on the patch... :)
Attachment #8935483 - Flags: review?(VYV03354)
Comment on attachment 8935483 [details] Bug 1374631 - Switch generated data to "constexpr" instead of "const" where possible https://reviewboard.mozilla.org/r/206384/#review219128 Please use `static constexpr const char* const ProcessIDToString` on non-Windows at <https://hg.mozilla.org/mozilla-central/annotate/e4107773cffb1baefd5446666fce22c4d6eb0517/toolkit/components/telemetry/gen_process_data.py#l49>, as I said above. ::: toolkit/components/telemetry/gen_histogram_data.py:49 (Diff revision 6) > label_count = 0 > keys_table = [] > keys_count = 0 > > print("constexpr HistogramInfo gHistogramInfos[] = {", file=output) > + Could you revert this empty line?
Attachment #8935483 - Flags: review?(VYV03354)
Samathy, are you still looking into this? We seem to be really close!
Flags: needinfo?(samathy)
Flags: needinfo?(samathy)
Assignee: samathy → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(samathy)
Update telemetry python scripts that generate C++ code to generate constexpr instead of const. However, we still use const for Windows code.
Do we still have to exclude Windows even though we have migrated to clang-cl? Maybe `#ifdef XP_WIN` could be changed to `#if defined(_MSC_VER) && defined(__clang__)`?
Sorry, `#if defined(_MSC_VER) && !defined(__clang__)` ^
(In reply to Masatoshi Kimura [:emk] from comment #77) > Do we still have to exclude Windows even though we have migrated to clang-cl? Does that mean clang-cl and MSVC could both be used to build FireFox on Windows? Or we only support clang-cl for Windows build now?
Flags: needinfo?(VYV03354)
Currently we support both clang-cl and MSVC. We may drop support for MSVC in the future. Since we do not drop support for MSVC yet, my suggestion considers about MSVC. `#if defined(_MSC_VER) && !defined(__clang__)` means "if MSVC (but not clang-cl)".
Flags: needinfo?(VYV03354)
Thank you, I will update the patch accordingly.
I've updated the patch, please help to review it again :) .
I've pushed the patch to try so we can see how CI likes building and running xpcshell tests on your change. https://treeherder.mozilla.org/#/jobs?repo=try&revision=969fa78adb73c18c44c0a6ed9d275fa0dabc5753
try is green. :emk, do you think this patch is ready to land?
Flags: needinfo?(VYV03354)
(In reply to Chris H-C :chutten from comment #84) > try is green. :emk, do you think this patch is ready to land? Yes, looks good.
Flags: needinfo?(VYV03354)
Then I'll go ahead and mark this for inclusion in the next Nightly. Thank you for your contribution, Thi Huynh! By now you might have an idea of what you'd like to work on next? Or is there something maybe I can help you find?
Keywords: checkin-needed
Attachment #8935483 - Attachment is obsolete: true
Assignee: nobody → so61pi.re
Comment on attachment 9001315 [details] Bug 1374631 - Switch telemetry generated code to constexpr instead of const. r?chutten Chris H-C :chutten has approved the revision.
Attachment #9001315 - Flags: review+
Pushed by rvandermeulen@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/caf561ef6351 Switch telemetry generated code to constexpr instead of const. r=chutten
Keywords: checkin-needed
(In reply to Chris H-C :chutten from comment #86) > Then I'll go ahead and mark this for inclusion in the next Nightly. > > Thank you for your contribution, Thi Huynh! By now you might have an idea of > what you'd like to work on next? Or is there something maybe I can help you > find? Thank you Chris. Could you help me on bug 1289094, I don't know if that bug is still relevant or not? I also don't know the use cases either so it's difficult for me to understand what needs to be done.
Flags: needinfo?(chutten)
https://hg.mozilla.org/mozilla-central/rev/caf561ef6351
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
I replied on bug 1289094. Clearing needinfo.
Flags: needinfo?(chutten)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: