Closed Bug 1441112 Opened 2 years ago Closed 2 years ago

2 - 6.35% compiler_metrics num_static_constructors (linux32, linux64, linux64-noopt) regression on push 647ee5497377c03fef78e9ec1578b5d1acf64a82 (Thu Feb 22 2018)

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox-esr52 --- unaffected
firefox58 --- unaffected
firefox59 --- unaffected
firefox60 --- fixed

People

(Reporter: igoldan, Assigned: emk)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(1 file)

We have detected a build metrics regression from push:

https://hg.mozilla.org/integration/autoland/pushloghtml?changeset=647ee5497377c03fef78e9ec1578b5d1acf64a82

As author of one of the patches included in that push, we need your help to address this regression.

Regressions:

  6%  compiler_metrics num_static_constructors linux32 pgo      126.00 -> 134.00
  6%  compiler_metrics num_static_constructors linux64 pgo      126.00 -> 134.00
  6%  compiler_metrics num_static_constructors linux32 opt      126.00 -> 134.00
  2%  compiler_metrics num_static_constructors linux32 debug    239.00 -> 244.00
  2%  compiler_metrics num_static_constructors linux64-noopt debug 250.00 -> 255.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=11691

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
These regressions showed up after the Linux build bustage was fixed. I noticed 3 bugs landed during it. Please state whether your bug is related or not to the regressions.

The bugs in question are:
1439269
1428258
1439470
Flags: needinfo?(mh+mozilla)
Flags: needinfo?(hikezoe)
Flags: needinfo?(VYV03354)
I couldn't find any cause of regression at a glance. Can I compare build metrics on try?
Flags: needinfo?(VYV03354)
(In reply to Masatoshi Kimura [:emk] from comment #2)
> I couldn't find any cause of regression at a glance. Can I compare build
> metrics on try?

Yes, you can do that on Try. Make 2 pushes: one with your patch and one without it.
You can then use the Compare tool from Perfherder [1].

[1] https://treeherder.mozilla.org/perf.html#/comparechooser
This.

https://hg.mozilla.org/mozilla-central/rev/e3b034c3ac97
Flags: needinfo?(mh+mozilla)
Flags: needinfo?(hikezoe)
Flags: needinfo?(VYV03354)
OK, I did a Try bisect to properly find the source bug for this issue. It turns out it's bug 1428258.
:emk Please look over this regression.
Flags: needinfo?(VYV03354)
BTW, thanks Hiroyuki!
Flags: needinfo?(VYV03354)
This is the comparison view [1]. On the left side, there's the first stable changeset, which includes all bugs I listed in comment 1. On the right side, I backed out bug 1428258.

[1] https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=51536899affe&newProject=try&newRevision=66c04fd42a84&framework=2&filter=num_static_constructors
Apparently including "mozilla/FStream.h" regressed the static initializer count:
https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=15c9bf1670c2&newProject=try&newRevision=98c617cbb601&framework=2

But I don't really understand why it causes such regressions. On Linux, FStream.h is effectively nothing but three includes and two typedefs.
Flags: needinfo?(VYV03354)
Wow, I didn't know about a known defect of a standard header, <iostream>.
https://stackoverflow.com/questions/40159890/static-order-initialization-fiasco-iostream-and-c11
> The results of including <iostream> in a translation unit shall be as if <iostream> defined an instance of ios_base::Init with static storage duration. Similarly, the entire program shall behave as if there were at least one instance of ios_base::Init with static storage duration

I changed <iostream> to separate <istream> and <ostream>. This change certainly fixed the regression:
https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=b634e250e11c&newProject=try&newRevision=26c153eefe30&framework=2
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
Comment on attachment 8954718 [details]
Bug 1441112 - Stop including <iostream> in FStream.h.

https://reviewboard.mozilla.org/r/223846/#review229860

Yes, one of those subtle little gotchas in C++. :(  Thanks for investigating!
Attachment #8954718 - Flags: review?(nfroyd) → review+
Pushed by VYV03354@nifty.ne.jp:
https://hg.mozilla.org/integration/autoland/rev/c88a04f0291b
Stop including <iostream> in FStream.h. r=froydnj
https://hg.mozilla.org/mozilla-central/rev/c88a04f0291b
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Component: Untriaged → XPCOM
Product: Firefox → Core
Target Milestone: Firefox 60 → ---
Version: unspecified → Trunk
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.