4.76 - 7.02% compiler_metrics num_constructors (linux32, linux64) regression on push 29f80f1769fc66ca5cc390183f3b49eec160ad4f (Wed Sep 21 2016)

RESOLVED FIXED in Firefox 52

Status

()

Core
Audio/Video: Playback
P1
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: wlach, Assigned: froydnj)

Tracking

({regression})

Trunk
mozilla52
regression
Points:
---

Firefox Tracking Flags

(firefox52 fixed)

Details

Attachments

(1 attachment)

Hi Matt, I am filing this bug because it seems like a large increase in the # of constructors. I don't know much about this metric so please consult :nfroyd if you have any questions.

--

We have detected a build metrics regression from push 29f80f1769fc66ca5cc390183f3b49eec160ad4f. As author of one of the patches included in that push, we need your help to address this regression.

Regressions:

  7%  compiler_metrics num_constructors linux64 pgo       171 -> 183
  7%  compiler_metrics num_constructors linux32 opt       171 -> 183
  5%  compiler_metrics num_constructors linux32 debug     231 -> 242
  5%  compiler_metrics num_constructors linux64 debug     231 -> 242


You can find links to graphs and comparison views for each of the above tests at: https://treeherder.mozilla.org/perf.html#/alerts?id=3327

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
Nathan, any ideas on this?

I'm pretty sure I only added StaticRefPtrs, which shouldn't have constructors.
Hi Guys, can you please place this in a component group, something other than Untriaged, thanks.
I think it makes sense to move it to the same component as the bug that caused the regression.
Component: Untriaged → Audio/Video: Playback
Product: Firefox → Core
Priority: -- → P1
(Assignee)

Comment 4

a year ago
Created attachment 8797285 [details] [diff] [review]
don't define nsCString constants in VideoUtils.h

Turns out you weren't responsible, though I think sImageMap might have been
part of a static constructor:

https://hg.mozilla.org/integration/mozilla-inbound/rev/8b6401bcb115#l15.49

You were just including VideoUtils.h in more places, and since that helpfully
defines string constants in every compilation unit that includes it, you were
injecting static constructors in more places.

Let's stop doing that.
Attachment #8797285 - Flags: review?(matt.woodrow)
Comment on attachment 8797285 [details] [diff] [review]
don't define nsCString constants in VideoUtils.h

Review of attachment 8797285 [details] [diff] [review]:
-----------------------------------------------------------------

I think it makes sense for JW to review this.
Attachment #8797285 - Flags: review?(matt.woodrow) → review?(jwwang)
Attachment #8797285 - Flags: review?(jwwang) → review+

Comment 6

a year ago
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/19c62c33ce97
don't define nsCString constants in VideoUtils.h; r=mattwoodrow

Comment 7

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/19c62c33ce97
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Assignee: nobody → nfroyd
Version: unspecified → Trunk
You need to log in before you can comment on or make changes to this bug.