1563700 - 4.76% tp5o responsiveness (linux64-shippable) regression on push 94f9177af77872ee8af4703a71dc6cc71e29d515 (Sat June 22 2019)

Status: RESOLVED FIXED

(Firefox :: Protections UI, defect)

Description

[Florin Strugariu [:Bebe]]

Talos has detected a Firefox performance regression from push:

https://hg.mozilla.org/integration/autoland/pushloghtml?changeset=94f9177af77872ee8af4703a71dc6cc71e29d515

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

Regressions:

5% tp5o responsiveness linux64-shippable opt e10s stylo 1.44 -> 1.51

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

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/Performance_sheriffing/Talos/Tests

For information on reproducing and debugging the regression, either on try or locally, see: https://wiki.mozilla.org/Performance_sheriffing/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/Performance_sheriffing/Talos/RegressionBugsHandling

Comment 1

[Johann Hofmann [:johannh]]

It's really unfortunate that this turned up 2 weeks after the patch landed :(

To be clear, bug 1549830 is introducing a feature that will inherently lead to more work being done after document unload. The feature is very high priority, so if we find that we can not further optimize this then we need to live with the perf regression.

With that said, I think there are several low-hanging fruits here. The first one could be moving the insertion code that happens after document unload into an idleCallback. I think we should try that first and see what Talos says. Erica, do you want to give that a try?

I'm not a big fan of just sprinkling idleCallbacks on top of things since we're not actually reducing CPU cycles, but, well, it's the cheapest thing we can do here.

:Bebe, do you have before/after profiles for these regressions?

Comment 2

[Florin Strugariu [:Bebe]]

Sorry for the long time it took to investigate this alert :(

Here you can find the profiles:
https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&searchStr=tp-p&tochange=94f9177af77872ee8af4703a71dc6cc71e29d515&fromchange=2b44c2b01908e15753c4c9a8fbb5b8242fded658

Comment 3

[Erica Wright [:ewright]]

Attachment: Bug 1563700 - Improve protections database performance.

Comment 4

[Erica Wright [:ewright]]

It seems to me that Johann's suggestion works, patch has been posted. We can hopefully batch the calls in a future bug to improve it further.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9970aca55d08
Improve protections database performance. r=johannh

Comment 6

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/9970aca55d08

Comment 7

[Ryan VanderMeulen [:RyanVM]]

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

Comment 8

[Erica Wright [:ewright]]

Comment on attachment 9076550 [details]
Bug 1563700 - Improve protections database performance.

Beta/Release Uplift Approval Request

• User impact if declined: Worse performance on page unload when saving the content blocking log to the content blocking database
• 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): It adds a simple requestIdleCallback to save the data, so waits for a free moment before saving. (note: has been verified with Talos)
• String changes made/needed: none

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9076550 [details]
Bug 1563700 - Improve protections database performance.

Fixes a perf regression in 69. Approved for 69.0b7.

Comment 10

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/releases/mozilla-beta/rev/57d5f2ae8afd