Ionuț Goldan [:igoldan] Reporter
	Description • 7 years ago

Talos has detected a Firefox performance regression from push:

https://hg.mozilla.org/integration/autoland/pushloghtml?changeset=665843c6cfd1a3049128597afbef8a68bcc1e086

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

Regressions:

  4%  displaylist_mutate windows7-32 pgo e10s stylo     4,594.14 -> 4,785.61
  4%  displaylist_mutate windows10-64 pgo e10s stylo    4,032.12 -> 4,175.96

Improvements:

 10%  tsvgr_opacity windows7-32 pgo e10s stylo     111.50 -> 100.61
  8%  tsvgr_opacity windows10-64 pgo e10s stylo    107.71 -> 99.00
  2%  rasterflood_gradient windows7-32 pgo e10s stylo1,334.75 -> 1,361.58


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

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

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

	Ionuț Goldan [:igoldan] Reporter
	Comment 1 • 7 years ago

:miko Can we do something to fix these perf regressions? Or should we consider accepting them?

	Miko Mynttinen Assignee
	Comment 2 • 7 years ago

Changes that landed with bug 1442190 specifically targeted opacity display item painting performance, as shown on by Talos tests.

It seems that these changes also added some overhead, most likely to display list iteration, where we now allocate and return a pair (display item pointer and start/end/item marker) instead of just a pointer. There is still plenty of room to improve this code, so I would prefer to accept this performance regression for now, and improve things in a follow-up bug.

	Ryan VanderMeulen [:RyanVM]
	Comment 3 • 7 years ago

What kind of timeframe do you expect that follow-up work to take place in? Can we use this bug to track improving that performance? :)

	Miko Mynttinen Assignee
	Comment 4 • 7 years ago

(In reply to Ryan VanderMeulen [:RyanVM] from comment #3)
> What kind of timeframe do you expect that follow-up work to take place in?
> Can we use this bug to track improving that performance? :)

Bug 1439292 tracks the work we are doing to reduce (and hopefully later eliminate) the use of inactive layers, which have a huge performance overhead. Bug 1442190 covered the opacity item flattening, and next up are the transform items. This will probably cause a lot of refactoring around this same code, which is why I think these optimizations should be postponed for a while. Timeframe is probably >1 month due to my upcoming PTO.

I will do some profiling later this week to see if there are some immediate easy wins here, such as, reducing the allocations for temporary items, or having a separate code path for display lists without opacity items.

	Ryan VanderMeulen [:RyanVM]
	Comment 5 • 7 years ago

That sounds good, thanks for the info. Let's leave the bug open for now pending that profiling work to see if there's any low-risk low-hanging fruit we can land to help mitigate the regression for now.

	Milan Sreckovic [:milan] (needinfo for best results)
	Comment 6 • 7 years ago

Not sure we need to track this.

	Marion Daly [:mdaly]
	Comment 7 • 7 years ago

Update?

	Miko Mynttinen Assignee
	Comment 8 • 7 years ago

Sadly, I haven't been able to look into this yet. I have been busy fixing the other issues from this same change.

	Ionuț Goldan [:igoldan] Reporter
	Comment 9 • 6 years ago

Clearing needinfo? based on comment 8.

	Miko Mynttinen Assignee
	Comment 10 • 6 years ago

Sorry for keeping this open for so long, I just got back from PTO.

The changes in bug 1460624 give me around 2% improvement in displaylist_mutate test (locally on MBP). I tried testing a couple of other micro-optimizations, but they yielded results that were within the benchmark error margin (<0.5-1%).

As mentioned in comment 4, I'm going to start working on transform flattening next, which involves touching this code. As such, I'm closing this as WONTFIX.

	Ryan VanderMeulen [:RyanVM]
	Comment 11 • 6 years ago

Thanks for following up.