Closed Bug 1454594 Opened Last year Closed Last year
.75 - 0 .75% compiler _metrics num _static _constructors (linux32, linux64) regression on push 3542146e997310c11ab000341cce6360623fce30 (Tue Apr 10 2018)
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
Component: Untriaged → Graphics: WebRender
Product: Firefox → Core
:kats Please look over bug 1449982, to address the regressions above.
Assignee: nobody → bugmail
Priority: -- → P2
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.
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
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.
I reconfirmed that the static constructor is coming from APZUpdater::sWindowIdMap:  is the baseline push at the revision which added that map, and shows 134.00 constructors.  is that same rev with sWindowIdLock commented out, and also shows 134.00 constructors.  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  on libxul.so) so I won't have to keep pushing to try.  https://treeherder.mozilla.org/#/jobs?repo=try&revision=08001854a547a4670190caefd027b22b90205a30  https://treeherder.mozilla.org/#/jobs?repo=try&revision=5532a69775b5389a738496b359582148b9f51ede  https://treeherder.mozilla.org/#/jobs?repo=try&revision=c40433143c8ac52d2faeb072a1c753a7d478e64a  https://searchfox.org/mozilla-central/source/build/util/count_ctors.py#34
In the past we've had to use StaticRefPtr in similar situations  - perhaps we have to here as well?  https://searchfox.org/mozilla-central/rev/59a9a86553e9bfd9277202748ff791fd9bc0713b/gfx/layers/apz/util/CheckerboardReportService.h#57
Ah, you're right! We have to use StaticAutoPtr in this case instead of UniquePtr.
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 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+
Pushed by email@example.com: https://hg.mozilla.org/integration/autoland/rev/fbfbfe496fcb Avoid running unordered_map static initializers. r=botond
On second thought... don't we need ClearOnShutdown calls to ensure these maps are actually destroyed, as with other uses of StaticAutoPtr (e.g. )?  https://searchfox.org/mozilla-central/rev/14578d6f2d2ec5246572827061ecefa60a40855d/gfx/layers/apz/src/AsyncPanZoomController.cpp#771
(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. )? 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.
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+
You need to log in before you can comment on or make changes to this bug.