1627569 - 6.57 - 31.91% about_newtab_with_snippets (linux64-*, macosx1014-64-shippable, windows10-64-*, windows7-32-shippable) regression on push 7a21216d200a80d14e6a4ccd2008f6404d3c79dd (Thu April 2)

Status: RESOLVED FIXED

(Firefox :: Messaging System, defect, P1)

Description

[Marian Raiciof [:marauder]]

6.57 - 31.91% about_newtab_with_snippets (linux64-shippable, linux64-shippable-qr, macosx1014-64-shippable, windows10-64-shippable, windows10-64-shippable-qr, windows7-32-shippable) regression on push 7a21216d200a80d14e6a4ccd2008f6404d3c79dd (Thu April 2 2020)

Talos has detected a Firefox performance regression from push:

https://hg.mozilla.org/integration/autoland/pushloghtml?changeset=7a21216d200a80d14e6a4ccd2008f6404d3c79dd

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

Regressions:

32% about_newtab_with_snippets macosx1014-64-shippable opt e10s stylo 70.54 -> 93.05
30% about_newtab_with_snippets macosx1014-64-shippable opt e10s stylo 70.56 -> 91.62
12% about_newtab_with_snippets linux64-shippable opt e10s stylo 69.54 -> 77.72
9% about_newtab_with_snippets linux64-shippable-qr opt e10s stylo 71.77 -> 78.38
8% about_newtab_with_snippets windows10-64-shippable-qr opt e10s stylo 60.64 -> 65.73
7% about_newtab_with_snippets windows10-64-shippable opt e10s stylo 60.83 -> 65.37
7% about_newtab_with_snippets windows7-32-shippable opt e10s stylo 59.27 -> 63.17

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

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 Talos jobs in a pushlog format.

To learn more about the regressing test(s), please see: https://wiki.mozilla.org/TestEngineering/Performance/Talos

For information on reproducing and debugging the regression, either on try or locally, see: https://wiki.mozilla.org/TestEngineering/Performance/Talos/Running

*** Please let us know your plans within 3 business days, or the offending patch(es) will be backed out! ***

Our wiki page outlines the common responses and expectations: https://wiki.mozilla.org/TestEngineering/Performance/Talos/RegressionBugsHandling

Comment 1

[Ed Lee :Mardak]

Looks like the regression started with 16.9.0 react-dom on mac about_newtab_with_snippets opt e10s stylo with 3 runs:

16. 8.6: 69.75 70.50 71.48
16. 9.0: 84.75 90.96 91.23
16.10.2: 83.95 85.72 90.71
16.11.0: 88.17 88.71 90.48
16.12.0: 87.24 90.30 93.13
16.13.1: 70.50 70.75 74.50

And perhaps the regression was fixed in a newer version 16.13.1. I guess we can switch to the newer version or revert to 16.8.6?

https://github.com/facebook/react/blob/master/CHANGELOG.md#16131-march-19-2020

https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&searchStr=talos&fromchange=6d77b61b88260ce248005175586a4638cbbb87f1&tochange=8e6b35f6cd3ecdd44f73a19ee213d5af00b06c2e&author=edilee%40gmail.com

andreio, any idea what's actually regressing "with_snippets" (but potentially not regressing other newtab talos tests)?

I also ran some regression range talos runs of https://github.com/facebook/react/compare/v16.8.6...v16.9.0 with 500+ commits, and seems to be between:

git lg 353e0ee47..fd601fb21 packages/react-dom/
* fce15f14d - don't fire missing act() warnings for react-art (#15975) (10 months ago) <Sunil Pai>
* e1c5e8720 - warn if passive effects get queued outside of an act() call. (#15763) (10 months ago) <Sunil Pai>
* 6568a7993 - [Scheduler] requestPaint (#15960) (10 months ago) <Andrew Clark>
* dc298fdf9 - [Flare] Refinements to useEvent hook (#15955) (10 months ago) <Dominic Gannaway>
* a5ed2f98f - [Flare] Guard against stateNode being null (#15952) (10 months ago) <Dominic Gannaway>
* 34ce57ae7 - [Flare] Refine flow type annotations (#15950) (10 months ago) <Dominic Gannaway>
* 4f92fbce5 - [Flare] Move createEvent back to React object (#15943) (10 months ago) <Dominic Gannaway>
* 175111de7 - Lazily initialize dependencies object (#15944) (10 months ago) <Andrew Clark>
* 720db4cbe - [Flare] Add useEvent hook implementation (#15927) (10 months ago) <Dominic Gannaway>
* ff91bfa58 - [act] reset scope depth on synchronous errors (#15937) (10 months ago) <Sunil Pai>
* 76864f7ff - Add SuspenseList Component (#15902) (10 months ago) <Sebastian Markbåge>
* b62ae1642 - [Flare] Rename createEventComponent -> createEvent (#15929) (10 months ago) <Dominic Gannaway>
* f4e1ac8ca - [Flare] Press events include defaultPrevented (#15916) (10 months ago) <Nicolas Gallagher>
* 689beef6f - [Flare] Move unstable_createEventComponent to ReactDOM (#15890) (10 months ago) <Dominic Gannaway>

I'm also trying to see what was changed in 16.13 that might have fixed the regression…

Comment 2

[Ed Lee :Mardak]

Attachment: Bug 1627569 - Update to react/dom 16.13.1 to fix talos regression r?andreio!

Comment 3

[Ed Lee :Mardak]

Here's some local runs finding the 16.12.0 -> 16.13.1 improvement:

$ git lg b66e86d95..24f824250 packages/react-dom/
71.75 * edeea0720 - Remove toString of dangerouslySetInnerHTML (#17773) (3 months ago) <Sebastian Markbåge>
73.25 * 85d9655d6 - [react-dom] Refactor event priority handling to its own module (#17678) (4 months ago) <Dominic Gannaway>
72.50 * dbc46ac63 - [react-interactions] Rename test + fix master (#17679) (4 months ago) <Dominic Gannaway>
73.25 * e7494c86c - [react-interactions] Remove batchedUpdates from responder lifecycles (#17659) (4 months ago) <Dominic Gannaway>
78.73 * 9fe103124 - [react-interactions] Rename Flare APIs to deprecated and remove from RN (#17644) (4 months ago) <Dominic Gannaway>

And finding the 16.8.6 -> 16.9.0 regression:

$ git lg 353e0ee47..fd601fb21 packages/react-dom/
77.00 * fce15f14d - don't fire missing act() warnings for react-art (#15975) (10 months ago) <Sunil Pai>
76.50 * 4f92fbce5 - [Flare] Move createEvent back to React object (#15943) (10 months ago) <Dominic Gannaway>
77.25 * 175111de7 - Lazily initialize dependencies object (#15944) (10 months ago) <Andrew Clark>
77.00 * 720db4cbe - [Flare] Add useEvent hook implementation (#15927) (10 months ago) <Dominic Gannaway>
72.00 * ff91bfa58 - [act] reset scope depth on synchronous errors (#15937) (10 months ago) <Sunil Pai>
73.00 * 76864f7ff - Add SuspenseList Component (#15902) (10 months ago) <Sebastian Markbåge>
72.00 * 689beef6f - [Flare] Move unstable_createEventComponent to ReactDOM (#15890) (10 months ago) <Dominic Gannaway>

At a glance, the regressing bug and improvement changes are unrelated to each other…

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d900213c3b03
Update to react/dom 16.13.1 to fix talos regression r=andreio

Comment 5

[Andrei Oprea [:andreio]]

(In reply to Ed Lee :Mardak from comment #1)

andreio, any idea what's actually regressing "with_snippets" (but potentially not regressing other newtab talos tests)?

That talos test also includes the DS layout. It renders Pocket stories. I think that's the difference.

Comment 6

[Ed Lee :Mardak]

Looks like my earlier bisecting was insufficient looking only at packages/react-dom changes. These should be the actual regression and improvement ranges as it goes by directly adjacent commits:

improvement in 16.13.0:
7dc974542 [Flight] Chunks API (#17398)
      "value": 72.49829930005188
9354dd275 Make HostComponent inexact (#17412)
      "value": 72.74957626996914
4c270375e Favor fallthrough switch instead of case statements for work tags (#17648)
      "value": 72.2495733775972
6fef7c47a Add a regression test for switching from Fragment to a component (#17647)
      "value": 81.7496223556337
9fe103124 [react-interactions] Rename Flare APIs to deprecated and remove from RN (#17644)
      "value": 80.24653838779838

regression in 16.9.0:
6ff4c9de1 [Flare] Press: fix stale deactivation region state (#15931)
      "value": 77.74642849044015
7a4c3e3b7 Make global names more obscure (#15941)
      "value": 78.0
270dc2e4d Add forwards and backwards options to SuspenseList (#15918)
      "value": 77.0
5368f7316 [Flare] Fix keyboard keyup regression (#15938)
      "value": 72.99324293474369
3d0af2aea Don't consider require-like calls to be likely HOCs (#15940)
      "value": 73.48489779814429

So at least both the improvement commit https://github.com/facebook/react/commit/4c270375e and regressing commit https://github.com/facebook/react/commit/270dc2e4d are touching similar files packages/react-reconciler/src/ReactFiber*.js

Here's a script for future reference:

for I in 7dc974542 6ff4c9de1
  do for J in `seq 0 4`
    do (
      cd ~/Development/react/
      git rh
      git co $I~$J
      git sh --oneline | head -1 >> ~/mozilla-central/tmp
      yarn
      yarn build react-dom/index --type=UMD_PROD
      cp build/dist/react-dom.production.min.js ~/mozilla-central/browser/components/newtab/vendor/react-dom.js
    )
    ./mach talos-test --activeTests about_newtab_with_snippets
    grep value testing/mozharness/build/local.json | tail -1 >> tmp
  done
  echo >> tmp
done

Comment 7

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/d900213c3b03

Comment 8

[Marian Raiciof [:marauder]]

== Change summary for alert #25547 (as of Mon, 06 Apr 2020 23:50:17 GMT) ==

Improvements:

19% about_newtab_with_snippets macosx1014-64-shippable opt e10s stylo 90.66 -> 73.62
6% about_newtab_with_snippets windows7-32-shippable opt e10s stylo 63.83 -> 59.81
6% about_newtab_with_snippets windows10-64-shippable-qr opt e10s stylo 65.62 -> 61.50
6% about_newtab_with_snippets windows10-64-shippable opt e10s stylo 65.86 -> 61.73

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=25547

Thanks for the fix!

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Beta approval when you get a chance.

Comment 10

[Ed Lee :Mardak]

Comment on attachment 9138439 [details]
Bug 1627569 - Update to react/dom 16.13.1 to fix talos regression r?andreio!

Beta/Release Uplift Approval Request

• User impact if declined: Performance regression on new tab page
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Minor version bump of vendored react files
• String changes made/needed:

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9138439 [details]
Bug 1627569 - Update to react/dom 16.13.1 to fix talos regression r?andreio!

Fixes a perf regression on the new tab page. Approved for 76.0b3.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/713627b96457