Closed Bug 1563700 Opened 3 months ago Closed 3 months ago

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

Categories

(Firefox :: Protections UI, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 70
Tracking Status
firefox-esr60 --- unaffected
firefox-esr68 --- unaffected
firefox68 --- unaffected
firefox69 --- fixed
firefox70 --- fixed

People

(Reporter: Bebe, Assigned: ewright)

References

(Blocks 1 open bug, Regression)

Details

(Keywords: perf, regression, talos-regression)

Attachments

(1 file)

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

Component: General → Tracking Protection
Flags: needinfo?(ewright)
Product: Testing → Firefox
Version: Version 3 → unspecified

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?

Flags: needinfo?(fstrugariu)

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.

Flags: needinfo?(ewright)
Pushed by ewright@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9970aca55d08
Improve protections database performance. r=johannh
Blocks: 1562138
No longer depends on: 1562138
Status: NEW → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 70
Assignee: nobody → ewright

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

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
Flags: needinfo?(ewright)
Attachment #9076550 - Flags: approval-mozilla-beta?

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

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

Attachment #9076550 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.