Closed Bug 1454594 Opened 6 years ago Closed 6 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)

Unspecified
Linux
defect

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
Component: Untriaged → Graphics: WebRender
Product: Firefox → Core
:kats Please look over bug 1449982, to address the regressions above.
Flags: needinfo?(bugmail)
Assignee: nobody → bugmail
Flags: needinfo?(bugmail)
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: [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
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
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 kgupta@mozilla.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. [1])?

[1] https://searchfox.org/mozilla-central/rev/14578d6f2d2ec5246572827061ecefa60a40855d/gfx/layers/apz/src/AsyncPanZoomController.cpp#771
Flags: needinfo?(bugmail)
https://hg.mozilla.org/mozilla-central/rev/fbfbfe496fcb
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
(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)
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.