Closed
Bug 1441826
Opened 7 years ago
Closed 7 years ago
0.11 - 0.14% installer size (osx-cross, windows2012-32, windows2012-64) regression on push addf903ba0158f04e78b57ac98856d3ef291b02e (Fri Feb 23 2018)
Categories
(Core :: WebRTC: Signaling, defect, P3)
Core
WebRTC: Signaling
Tracking
()
RESOLVED
WORKSFORME
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox59 | --- | unaffected |
firefox60 | --- | fixed |
firefox61 | --- | fixed |
People
(Reporter: igoldan, Unassigned)
References
Details
(Keywords: regression)
We have detected a build metrics regression from push:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=addf903ba0158f04e78b57ac98856d3ef291b02e
As author of one of the patches included in that push, we need your help to address this regression.
Regressions:
0% installer size windows2012-64 opt 60,810,370.33 -> 60,893,671.75
0% installer size windows2012-32 opt 55,415,202.88 -> 55,495,524.00
0% installer size osx-cross opt 67,243,778.54 -> 67,315,849.75
You can find links to graphs and comparison views for each of the above tests at: https://treeherder.mozilla.org/perf.html#/alerts?id=11806
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 | ||
Comment 1•7 years ago
|
||
This is a wicked regression which is obvious on osx-cross. What's strange about it is that it seems caused by bug 1379265. But that bug was backed out and is still backed out as we're speaking.
I have a feeling this tracking bug will be closed as INVALID, but I want to know what happened in this case.
:bholley Can you shed some light?
Flags: needinfo?(bobbyholley)
Comment 2•7 years ago
|
||
(In reply to Ionuț Goldan [:igoldan], Performance Sheriffing from comment #1)
> This is a wicked regression which is obvious on osx-cross. What's strange
> about it is that it seems caused by bug 1379265. But that bug was backed out
> and is still backed out as we're speaking.
> I have a feeling this tracking bug will be closed as INVALID, but I want to
> know what happened in this case.
>
> :bholley Can you shed some light?
It's seems plausible to me that bug 1379265 could add the ~80k to the binary that we're seeing here. If that is the case, there's unlikely to be much we can do about it - it looks like a lot of code was added.
As for confirming the source of the regression, you're saying that codesize increased with bug 1379265, but then we didn't see a corresponding decrease when bug 1379265 was backed out? If so, I would suggest the following investigation:
(1) Confirm with a try push that backing out bug 1379265 immediately after it landed eliminates the regression.
(2) Confirm with a try push that backing out bug 1379265 immediately before it was backed out on m-c eliminates the regression.
(3) Bisect with try pushes to find the rev where the backout stops working.
Flags: needinfo?(bobbyholley)
Reporter | ||
Comment 3•7 years ago
|
||
(In reply to Bobby Holley (:bholley) from comment #2)
> (3) Bisect with try pushes to find the rev where the backout stops working.
I don't understand this step. What are we looking for while doing this?
Reporter | ||
Comment 4•7 years ago
|
||
It looks like bug 1379265 added another static constructors on multiple Linux platforms. Please address these.
== Change summary for alert #11806 (as of Wed, 28 Feb 2018 12:10:59 GMT) ==
Regressions:
1% compiler_metrics num_static_constructors linux32 pgo 134.00 -> 135.00
1% compiler_metrics num_static_constructors linux64 pgo 134.00 -> 135.00
1% compiler_metrics num_static_constructors linux32 opt 134.00 -> 135.00
1% compiler_metrics num_static_constructors linux64 opt 131.50 -> 132.33
0% compiler_metrics num_static_constructors linux32 debug 244.00 -> 245.0
For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=11806
Flags: needinfo?(dminor)
Reporter | ||
Updated•7 years ago
|
Component: Untriaged → WebRTC: Signaling
Product: Firefox → Core
Comment 5•7 years ago
|
||
Bug 1379265 adds a new SDP parser with the intention of running it in parallel with the existing one while it is being developed and debugged. Eventually we'll remove the older SDP parser, but in the short term, size regressions are unfortunately expected.
If backing out the changes from Bug 1379265 did not fix the problem then it seems likely that something else was the cause.
Flags: needinfo?(dminor)
Comment 7•7 years ago
|
||
(In reply to Ionuț Goldan [:igoldan], Performance Sheriffing from comment #3)
> (In reply to Bobby Holley (:bholley) from comment #2)
> > (3) Bisect with try pushes to find the rev where the backout stops working.
>
> I don't understand this step. What are we looking for while doing this?
So.
In filing this bug, you're saying that landing bug 1379265 caused a codesize regression. That means that the revision in which it landed has measurably more codesize than the revision before. So rewinding to the revision at which it landed, reverting the patch, and pushing it to try should result in the old/lower codesize number. That means that a backout "works" at that revision. The point of step (1) is to verify this.
You also say that the backout that landed on mozilla-central (for unrelated reasons) does not reduce codesize. So rewinding to the revision immediately before the backout, redoing the backout, and pushing to try should result in the new/higher codesize number. That means a backout "doesn't work" at that revision. The point of step (2) is to verify this.
So this means we have revision X where the backout works, and revision Z where it doesn't. So it would be interesting to bisect the revisions between X and Z to determine whether there's a specific revision Y (such that X < Y < Z) where the backout "stops working" (i.e. no longer produces a binary with the old/lower codesize). This is step (3).
This might be interesting because revision Y might be a separate codesize regression that was hidden by the backout of at Z. It might also be noise, but it's worth having a look assuming you have time.
Flags: needinfo?(bobbyholley)
Reporter | ||
Comment 8•7 years ago
|
||
Thanks for explaining this. I did the first 2 steps.
Step (1) shows immediate backout takes effect [1].
Step (2) unfortunately also takes effect [2] and our expectations don't hold. So something happened in this big patch [3] which "officially" did the backout from inbound.
[1] https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=6a28c7e1eece&newProject=try&newRevision=12588a084d63&framework=2&filter=installer%20size
[2] https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=6a28c7e1eece&newProject=try&newRevision=4225f74c4ea3&framework=2&filter=installer%20size
[3] https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=a6329ea89ef83aa59f0706ed8a07529c097a6ce6&tochange=1f44de15ffdb1c0ac9c2719efa2bbe12ea75d06b
Comment 9•7 years ago
|
||
Ahah! So that suggests that there was another codesize regression in that merge from inbound, and canceled out the backout of the rsdparsa stuff.
My bet is that it's the WebMIDI code, which looks substantial. If that's the case, there's not too much we can do about it, though it's really good to understand, and probably worth commenting in the WebMIDI bug about the codesize impact. Can you verify that from the graphs on inbound in that time period?
Reporter | ||
Comment 10•7 years ago
|
||
The WebMIDI bug (bug 1201590) does look like its regressive. It landed on autoland first, but caused a small 40KBytes increase.
I'm doing a bisect on inbound, to precisely see the effects.
Updated•7 years ago
|
Rank: 25
Priority: -- → P3
Reporter | ||
Comment 11•7 years ago
|
||
This push [1] won us perf improvements and some regressions. I guess we can live with the latter.
== Change summary for alert #12135 (as of Tue, 13 Mar 2018 18:02:40 GMT) ==
Regressions:
5% Strings PerfCompressWhitespace windows7-32 opt 255,287.46 -> 267,421.33
3% Strings PerfHasRTLCharsJA windows7-32 opt 341,128.25 -> 352,138.25
Improvements:
7% Strings PerfHasRTLCharsRU windows10-64 opt 879,862.79 -> 819,750.42
5% Strings PerfHasRTLCharsRU windows7-32 opt 869,812.08 -> 824,711.92
5% Strings PerfHasRTLCharsDE windows10-64 opt 887,728.00 -> 843,026.17
5% Strings PerfHasRTLCharsExample3 windows10-64 opt 15,850.62 -> 15,132.58
4% Strings PerfHasRTLCharsDE windows7-32 opt 876,522.75 -> 841,301.17
For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=12135
[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1379265#c71
Comment 12•7 years ago
|
||
Is this bug actionable?
status-firefox59:
--- → unaffected
status-firefox60:
--- → affected
status-firefox61:
--- → affected
status-firefox-esr52:
--- → unaffected
Flags: needinfo?(igoldan)
Reporter | ||
Comment 13•7 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM] from comment #12)
> Is this bug actionable?
Good point: not anymore. Very big perf improvements landed meanwhile and basically resolved all regressions noted in this bug.
Flags: needinfo?(igoldan)
Reporter | ||
Updated•7 years ago
|
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → WORKSFORME
Updated•7 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•