Closed Bug 1667938 Opened 4 years ago Closed 4 years ago

31.17 - 31.65% google-maps ContentfulSpeedIndex (android-hw-g5-7-0-arm7-api-16-shippable) regression on push 1d341cedbe03943ed786d8c831341549b4b070e5 (Fri September 25 2020)

Categories

(Remote Protocol :: Marionette, defect)

Product:
Remote Protocol
Component:
Marionette
Version:
Firefox 83
Type:
defect

Tracking

(firefox83 affected)

Status:
RESOLVED INVALID
Tracking Flags:
Tracking Status
firefox83 --- affected

People

(Reporter: Bebe, Unassigned)

References

(Regression)

Details

(Keywords: perf, perf-alert, regression)

Perfherder has detected a browsertime performance regression from push 1d341cedbe03943ed786d8c831341549b4b070e5. As author of one of the patches included in that push, we need your help to address this regression.

Regressions:

32% google-maps ContentfulSpeedIndex android-hw-g5-7-0-arm7-api-16-shippable opt cold 1,044.83 -> 1,375.50
31% google-maps ContentfulSpeedIndex android-hw-g5-7-0-arm7-api-16-shippable opt cold 1,050.79 -> 1,378.33

Improvements:

24% instagram ContentfulSpeedIndex android-hw-p2-8-0-android-aarch64-shippable opt cold 993.50 -> 757.75
12% allrecipes ContentfulSpeedIndex android-hw-g5-7-0-arm7-api-16-shippable opt cold 2,718.75 -> 2,398.67

Details of the alert can be found in the alert summary, including links to graphs and comparisons for each of the affected tests. Please follow our guide to handling regression bugs and let us know your plans within 3 business days, or the offending patch(es) will be backed out in accordance with our regression policy.

For more information on performance sheriffing please see our FAQ.

Component: Performance → Marionette

Again, this is similar to bug 1663668 and I'm fairly sure this is a failure again when trying to detect the regression range. Nothing in browsertime uses the WebDriver:NavigateTo command yet. So this cannot be a real regression.

Also this referenced bug actually fixes a broken navigation, which actually was never initiated.

Greg, can you please have a look?

Flags: needinfo?(gmierz2)

:whimboo, this regression seems to have occurred because of your patch: https://treeherder.mozilla.org/perf.html#/graphs?highlightAlerts=1&selected=2474232,1229070580&series=autoland,2474232,1,13&timerange=1209600&zoom=1601060985876,1601065501651,1003.15178522723,1426.0944246532424

The previous push was a lower value. I checked the condition profile and it did not update in between runs either.

:whimboo, do you think there might be something in selenium which uses this code?

Flags: needinfo?(gmierz2) → needinfo?(hskupin)

I would have to know which commands browsertime makes use of here. As I know all navigation happens via execute script, but not navigate. Other commands that make use of this navigation Promise are back, forward, refresh, and element click.

Note that under certain situations and especially a page load strategy of none we never actually triggered the real navigation! So I certainly expect a longer execution of the navigation related code. I'm baffled by the improvements through.

Flags: needinfo?(hskupin) → needinfo?(gmierz2)

Ah the improvements are wrongly classified though! :bebe, can you take a look at those and re-assign them? They are caused by this patch: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&revision=0304f3e4247fb80e8fe2875496ee3f6de5496cc7&selectedTaskRun=Dqv_GkTBQSWIHahMjOXmOQ.0

:whimboo, would a profile of the test help here? One thing to note is that only the cold pageload was affected by this change, the warm pageload didn't experience a regression.

I see that the wait function is really the only one that is used, I haven't been able to find anything else: https://github.com/sitespeedio/browsertime/blob/de710c143f9f49a4fa13c808906ac712406ca586/lib/core/seleniumRunner.js#L170

Flags: needinfo?(hskupin)
Flags: needinfo?(gmierz2)
Flags: needinfo?(fstrugariu)

(In reply to Greg Mierzwinski [:sparky] from comment #4)

:whimboo, would a profile of the test help here? One thing to note is that only the cold pageload was affected by this change, the warm pageload didn't experience a regression.

I think that getting some feedback from Andrew might be good here.

I see that the wait function is really the only one that is used, I haven't been able to find anything else: https://github.com/sitespeedio/browsertime/blob/de710c143f9f49a4fa13c808906ac712406ca586/lib/core/seleniumRunner.js#L170

None of this code uses the Webdriver specific navigation commands. So I'm still baffled.

Flags: needinfo?(hskupin) → needinfo?(acreskey)

It's interesting that none of the other visual metrics (e.g. firstVisualChange or speedIndex) are affected.
And loadtime also doesn't look shifted.

I also can't figure out what in browsertime would be affected by adding this callback.

Are the videos accessible?
I'm curious as to what we're actually looking at here.

Greg, you had mentioned this to me the other day -- we use none for the pageload strategy in CI
https://searchfox.org/mozilla-central/source/testing/raptor/raptor/browsertime/base.py#214

This is kind of interesting -- to my understanding that means we're not actually waiting for the document's load event. (Only the download of the document itself, and not sub resources).
Anyway, I guess that's a separate discussion. (I guess the 5000 ms wait is sufficient for all of our content?)

See Also: → 1668241

Thanks for the video links.

As Greg pointed out, the site isn't loading correctly in this test either before or after the change.

Can currently there is an animated loading indicator that runs throughout the video -- this is also a problem for visual metrics since there is no stabilization.

My suggestion is to close this as invalid.

Flags: needinfo?(acreskey)

Thanks :acreskey, +1 to closing this as invalid

Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → INVALID

Thank you both for figuring that out.

Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.