Closed
Bug 1454594
Opened 7 years ago
Closed 7 years ago
0.75 - 0.75% compiler_metrics num_static_constructors (linux32, linux64) regression on push 3542146e997310c11ab000341cce6360623fce30 (Tue Apr 10 2018)
Categories
(Core :: Graphics: WebRender, defect, P2)
Tracking
()
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox59 | --- | unaffected |
firefox60 | --- | unaffected |
firefox61 | - | fixed |
People
(Reporter: igoldan, Assigned: kats)
References
Details
(Keywords: regression)
Attachments
(2 files)
We have detected a build metrics regression from push: https://hg.mozilla.org/integration/autoland/pushloghtml?changeset=3542146e997310c11ab000341cce6360623fce30 As author of one of the patches included in that push, we need your help to address this regression. Regressions: 1% compiler_metrics num_static_constructors linux32 pgo 133.00 -> 134.00 1% compiler_metrics num_static_constructors linux64 pgo 133.00 -> 134.00 You can find links to graphs and comparison views for each of the above tests at: https://treeherder.mozilla.org/perf.html#/alerts?id=12752 On the page above you can see an alert for each affected platform as well as a link to a graph showing the history of scores for this test. There is also a link to a treeherder page showing the jobs in a pushlog format. To learn more about the regressing test(s), please see: https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Automated_Performance_Testing_and_Sheriffing/Build_Metrics
Reporter | ||
Updated•7 years ago
|
Component: Untriaged → Graphics: WebRender
Product: Firefox → Core
Reporter | ||
Comment 1•7 years ago
|
||
:kats Please look over bug 1449982, to address the regressions above.
Flags: needinfo?(bugmail)
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → bugmail
Flags: needinfo?(bugmail)
Updated•7 years ago
|
Blocks: stage-wr-trains
Priority: -- → P2
Assignee | ||
Comment 2•7 years ago
|
||
I confirmed this is the coming from the sWindowIdMap in APZUpdater. I guess std::unordered_map doesn't have a constexpr constructor? Anyway, given that the sWindowIdMap will end up getting created in all processes (and not just the process in which APZ is running) this seems like a good thing to fix. Also note bug 1451469 will add another one of these. I'll probably fix them both in this bug.
Assignee | ||
Comment 3•7 years ago
|
||
Interestingly bug 1451469 did *not* increase the number of static constructors. I'm not sure why. Anyhow I wrote a patch to wrap the unordered_map in a UniquePtr and create it on first insertion. Hopefully that'll be sufficient: https://treeherder.mozilla.org/#/jobs?repo=try&revision=96ab5d581549c8752513d5dee38059136a08d505
Updated•7 years ago
|
status-firefox59:
--- → unaffected
status-firefox60:
--- → unaffected
status-firefox61:
--- → affected
status-firefox-esr52:
--- → unaffected
tracking-firefox61:
--- → -
Assignee | ||
Comment 4•7 years ago
|
||
Try push doesn't seem to have helped. Again, not sure why. I might have screwed up the "confirmation" in comment 2. Will try again once the trees are open.
Assignee | ||
Comment 5•7 years ago
|
||
I reconfirmed that the static constructor is coming from APZUpdater::sWindowIdMap: [1] is the baseline push at the revision which added that map, and shows 134.00 constructors. [2] is that same rev with sWindowIdLock commented out, and also shows 134.00 constructors. [3] is the same rev with sWindowIdMap commented out, and that shows 133.00 constructors. However wrapping it in a UniquePtr even at that revision doesn't fix the problem, so clearly I'm doing something wrong or not understanding the problem. At least I figured out how to reproduce this locally (basically run [4] on libxul.so) so I won't have to keep pushing to try. [1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=08001854a547a4670190caefd027b22b90205a30 [2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=5532a69775b5389a738496b359582148b9f51ede [3] https://treeherder.mozilla.org/#/jobs?repo=try&revision=c40433143c8ac52d2faeb072a1c753a7d478e64a [4] https://searchfox.org/mozilla-central/source/build/util/count_ctors.py#34
Comment 6•7 years ago
|
||
In the past we've had to use StaticRefPtr in similar situations [1] - perhaps we have to here as well? [1] https://searchfox.org/mozilla-central/rev/59a9a86553e9bfd9277202748ff791fd9bc0713b/gfx/layers/apz/util/CheckerboardReportService.h#57
Assignee | ||
Comment 7•7 years ago
|
||
Ah, you're right! We have to use StaticAutoPtr in this case instead of UniquePtr.
Assignee | ||
Comment 8•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=91ddd27a363a51b643c48e4e93ddad6255189b37
Assignee | ||
Comment 9•7 years ago
|
||
The try push seems to have worked, the number of constructors is 127.00 compared to the 128.00 in the m-c push it was based on. Note that in my local testing I discovered that only updating APZUpdater.cpp did not bring down the initializer count, so I think what's happening is that the initializers for APZSampler and APZUpdater are getting combined into one (maybe from unified builds?). That explains why APZUpdater increased the count but APZSampler didn't further increase it, and why I need to remove both in order to bring the count back down.
Comment hidden (mozreview-request) |
Comment 11•7 years ago
|
||
mozreview-review |
Comment on attachment 8969268 [details] Bug 1454594 - Avoid running unordered_map static initializers. https://reviewboard.mozilla.org/r/237998/#review244230
Attachment #8969268 -
Flags: review?(botond) → review+
Comment 12•7 years ago
|
||
Pushed by kgupta@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/fbfbfe496fcb Avoid running unordered_map static initializers. r=botond
Comment 13•7 years ago
|
||
On second thought... don't we need ClearOnShutdown calls to ensure these maps are actually destroyed, as with other uses of StaticAutoPtr (e.g. [1])? [1] https://searchfox.org/mozilla-central/rev/14578d6f2d2ec5246572827061ecefa60a40855d/gfx/layers/apz/src/AsyncPanZoomController.cpp#771
Flags: needinfo?(bugmail)
Comment 14•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/fbfbfe496fcb
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Assignee | ||
Comment 15•7 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #13) > On second thought... don't we need ClearOnShutdown calls to ensure these > maps are actually destroyed, as with other uses of StaticAutoPtr (e.g. [1])? Yeah, I guess so. ClearOnShutdown needs to be called on the main thread, so we have to either dispatch a task to the main thread to do it when we allocate the map, or we can just do it unconditionally somewhere in the startup codepath that runs on the main thread, and if WR isn't being used then it will just clear a null pointer (i.e. be a no-op). I'm not sure which is preferable but I'll pick whatever seems better.
Flags: needinfo?(bugmail)
Assignee | ||
Comment 16•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=59291935afe4f8d1e464d7d78922fab23bb70041
Comment 18•7 years ago
|
||
Comment on attachment 8970219 [details] [diff] [review] Follow-up for ClearOnShutdown Review of attachment 8970219 [details] [diff] [review]: ----------------------------------------------------------------- LGTM! (And thanks for reminding me that ClearOnShutdown needs to be run on the main thread. I got that wrong in my WIP patches in bug 1448439 (in which I introduce a new static object), will fix before posting for review.)
Attachment #8970219 -
Flags: review?(botond) → review+
Assignee | ||
Comment 19•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/77865327ee5700d7e5b688f0cbf1c95362b2ef17
You need to log in
before you can comment on or make changes to this bug.
Description
•