1188636 - TPS score is measuring time spent in Thumbnails_delayedCapture/timeout

Status: RESOLVED WONTFIX

(Testing :: Talos, defect)

Description

[Benoit Girard (:BenWa)]

Thumbnail is an extra source of noise and has no business happenging during the tab switch:
http://people.mozilla.org/~bgirard/cleopatra/#report=d81c61fbf0e2a2c6bf6d31126bfa4ac2a2563103

Comment 1

[Benoit Girard (:BenWa)]

Relevant code:
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-thumbnails.js#106

We have a 1000ms timeout but we appear to have one per aBrowser, which AFAIK means per tab. During TPS we will switch to a tab and keep switching to the next tabs. When a tab is activated we delay the capture but we don't clear the other pending timeouts which means that the background tabs get their thumbnail timeout fire.

We don't seem to really snapshot anything because it fails the topSiteURLs test however we still spend 4ms computing the 'topSiteURLs' array which fires at mostly random times introducing noise. 4ms is significant noise for TPS.

Comment 2

[Benoit Girard (:BenWa)]

Hey :marhk,

I'm told you know about the thumbnail code. Is it intentional that the capture code timeout is per tab? If we don't want to thumbnail while you're doing heavy interaction with the browser IMO it shouldn't matter if it's with the current tab or another tab.

Somewhat related but get _topSiteURLs() takes over 4ms on a clean profile, see the profile in comment 0. All of that time is spent directly in the JS code which seems like a crazy amount of CPU time to compute this for a new profile.

Comment 3

[Mark Hammond [:markh] [:mhammond]]

(In reply to Benoit Girard (:BenWa) from comment #2)
> Hey :marhk,
> 
> I'm told you know about the thumbnail code. Is it intentional that the
> capture code timeout is per tab? If we don't want to thumbnail while you're
> doing heavy interaction with the browser IMO it shouldn't matter if it's
> with the current tab or another tab.
> 
> Somewhat related but get _topSiteURLs() takes over 4ms on a clean profile,
> see the profile in comment 0. All of that time is spent directly in the JS
> code which seems like a crazy amount of CPU time to compute this for a new
> profile.

Hey Benoit,
  While I know a little about the "background" thumbnailer, Tim and Drew were involved in this "foreground" thumbnailer so will hopefully have some insights.

Until then, my understanding is that the timeout isn't really about performance but more about ensuring the page has finished doing fancy things at load time so we get an accurate snapshot. It largely pre-dates our "background" thumbnailer, so it makes sense to me that we could decline to capture a page when the browser is under load under the assumption the background thumbnailer will get it if necessary.  And yeah, _topSiteURLs() performance sounds sub-optimal :(

Comment 4

[Drew Willcoxon :adw]

Yeah, it looks like the timeouts are per tab.  Tim would know more about the reason for the timeout than I do.  It sounds like there's room for improvement if we're not clearing the timeouts of tabs that you switch away from.

About topSiteURLs -- browser-thumbnails.js could probably cache it, have the newtab page notify it when it's potentially changed, and only then fetch it again.  It would potentially change on every page visit though.  Not sure why it's taking 4 ms on a new profile.  getLinks() only merges some arrays that should be mostly empty in that case.

I can take a look at all of this but I'm busy with other things right now, so I think basically Tim would have to ask me to work on it.

Comment 5

[Benoit Girard (:BenWa)]

(In reply to Drew Willcoxon :adw from comment #4)
> About topSiteURLs -- browser-thumbnails.js could probably cache it

It's 4ms for no good reason! Why cache it? This should be fixed properly.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

The foreground thumbnail was introduced before we even had the background thumbnailer. I'd happily get rid of it given that about:newtab can request missing images from the background thumbnailer. The problem as usual are Ctrl+Tab and Panorama, those do not interact with the bg thumbnailer. So if about:newtab is the only consumer that request bg captures and stores them, Ctrl+Tab and Panorama will have a bad time when encountering "pending" tabs, those that haven't been fully restored by sessionstore after restarting the browser. We wouldn't want to request bg captures for every tab and thus load every page twice.

I can't remember a lot about the code behind the _topSiteURLs getter. The newtab page team (content services nowadays) would probably be the right contact here?