(no longer active) Assignee
	Description • 7 years ago

In the profile from bug 1312373, it takes 1.6 *seconds*.  In my session startup <https://perfht.ml/2nqGAqR>, it takes nearly 2 *seconds*.

	(no longer active) Assignee
	Comment 1 • 7 years ago

This was done in bug 960941, guessing it wouldn't have too much of a performance impact...

	(no longer active) Assignee
	Comment 2 • 7 years ago

So this is one option!  In practice, the list of items on the new tab page doesn't change that much during every session except for very new profiles.  This completely eliminates the time this function takes down to about 6ms in my local testing.

	Mike Conley (:mconley) (:⚙️)
	Comment 3 • 7 years ago

Comment on attachment 8851444 [details] [diff] [review]
Get the list of top site URLs for generating the thumbnails only once every session

I'm actually going to redirect this r? to markh, who I believe is more familiar with the direct responsibilities of this code.

	Mark Hammond [:markh] [:mhammond]
	Comment 4 • 7 years ago

Comment on attachment 8851444 [details] [diff] [review]
Get the list of top site URLs for generating the thumbnails only once every session

Review of attachment 8851444 [details] [diff] [review]:
-----------------------------------------------------------------

As you mention, this will impact new profiles in undesirable ways - these new users will no longer see thumbnails appear as they visit sites, which I'd really prefer to avoid. I *do* think it would be OK to throttle this to, say, once every minute or so - that would solve the session restore problem and would leave reasonable semantics in place for new profiles.

The session restore case shows another optimization opportunity - IIUC, this function will fire once for every tab being loaded - however, after that function returns we will decline to capture all but one of the restored tabs as _shouldCapture() will return false unless it is the current tab. Simply calling _shouldCapture() before _topSiteURLs() will still be inefficient as it involves messages between processes.

If we duplicate the |(aBrowser != gBrowser.selectedBrowser)| check from shouldCapture() before we call _topSiteURLs(), we end up with only 2 calls to _topSiteURLs() on session restore (one for a TabSelect event for the current tab, and another when the nsIWebProgressListener.STATE_STOP listener for that same tab fires) - which is still one more than we want - so it's probably best to just implement "only refresh once per minute" semantics, which should end up with the desired result of only 1 such query executed at startup.

Related: the thumbnails code also has a 1 hour expiration timer to remove old stale thumbnails, and for better or worse, that timer is implemented via nsUpdateTimerManager, which persists the last time the timer fired. This means that if the profile has been shutdown for longer than an hour we run the expiration very soon after startup. Only refreshing these values once per minute should also avoid that one additional call in the startup cases - but I wonder if it makes sense to file a bug for that expiration - no real good can come from expiring thumbnails from disk while the browser is still warming up.

	(no longer active) Assignee
	Comment 5 • 7 years ago

(In reply to Mark Hammond [:markh] from comment #4)
> Comment on attachment 8851444 [details] [diff] [review]
> Get the list of top site URLs for generating the thumbnails only once every
> session
> 
> Review of attachment 8851444 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> As you mention, this will impact new profiles in undesirable ways - these
> new users will no longer see thumbnails appear as they visit sites, which
> I'd really prefer to avoid. I *do* think it would be OK to throttle this to,
> say, once every minute or so - that would solve the session restore problem
> and would leave reasonable semantics in place for new profiles.
> 
> The session restore case shows another optimization opportunity - IIUC, this
> function will fire once for every tab being loaded - however, after that
> function returns we will decline to capture all but one of the restored tabs
> as _shouldCapture() will return false unless it is the current tab. Simply
> calling _shouldCapture() before _topSiteURLs() will still be inefficient as
> it involves messages between processes.
> 
> If we duplicate the |(aBrowser != gBrowser.selectedBrowser)| check from
> shouldCapture() before we call _topSiteURLs(), we end up with only 2 calls
> to _topSiteURLs() on session restore (one for a TabSelect event for the
> current tab, and another when the nsIWebProgressListener.STATE_STOP listener
> for that same tab fires) - which is still one more than we want - so it's
> probably best to just implement "only refresh once per minute" semantics,
> which should end up with the desired result of only 1 such query executed at
> startup.

Fair enough!

> Related: the thumbnails code also has a 1 hour expiration timer to remove
> old stale thumbnails, and for better or worse, that timer is implemented via
> nsUpdateTimerManager, which persists the last time the timer fired. This
> means that if the profile has been shutdown for longer than an hour we run
> the expiration very soon after startup. Only refreshing these values once
> per minute should also avoid that one additional call in the startup cases -
> but I wonder if it makes sense to file a bug for that expiration - no real
> good can come from expiring thumbnails from disk while the browser is still
> warming up.

Filed bug 1353619.

	(no longer active) Assignee
	Comment 6 • 7 years ago

The test changes are needed in order to address some test failures that this patch somehow causes which cause the executeSoon() function to not be found and tests timing out as a result.

	Mark Hammond [:markh] [:mhammond]
	Comment 7 • 7 years ago

Comment on attachment 8855146 [details] [diff] [review]
Avoid recomputing gBrowserThumbnails._topSiteURLs for 1 minute after computing it to speed up session restore

Review of attachment 8855146 [details] [diff] [review]:
-----------------------------------------------------------------

Interesting approach re the timer - I probably would have just kept the Date.now() we last read the values, but this works! I agree the test changes are strange, but meh. Thanks!

	(no longer active) Assignee
	Comment 8 • 7 years ago

This function was running *so* often that I wanted to ensure that the getter is memoized on the common path at all times.  Relying on Date.now() would have forced me to make the getter a function that would check the last read time so I thought the extra code complexity of the timer approach would be justifiable.  :-)

	Pulsebot
	Comment 9 • 7 years ago

Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/228a6487d0c1
Avoid recomputing gBrowserThumbnails._topSiteURLs for 1 minute after computing it to speed up session restore; r=markh

	Phil Ringnalda (:philor)
	Comment 10 • 7 years ago

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/6c9a0b2f7553aeb879c578597d37308679ab5328 for eslint failures, https://treeherder.mozilla.org/logviewer.html#?job_id=89414912&repo=mozilla-inbound

	Phil Ringnalda (:philor)
	Comment 11 • 7 years ago

Should have waited another couple of minutes, I could have said "for actual failures, https://treeherder.mozilla.org/logviewer.html#?job_id=89420050&repo=mozilla-inbound" instead of just eslint.

	(no longer active) Assignee
	Comment 12 • 7 years ago

I have no idea why previously making this change made these tests pass, I must have made a mistake and tested the wrong working tree or something.  :-(

Now I realize that the issue is something else entirely.  The executeSoon() failures were a red herring, there were caused by the callback being called over and over and finally being called after the window being closed and the test object setup in browser-test.js got torn down hence executeSoon was no longer found on the global object!  It could be that I got really unlucky since there is an extremely unlikely race condition that can make the test pass, see below!

The issue here is that these tests open a tab, and essentially rely on the 1 second timer in gBrowserThumbnails to capture a thumbnail from the tab.  But my patch here caches the set of top URLs which makes the newly opened tab not be captured immediately before the test times out!  The fix is simple, by refactoring the contents of the current notify() method into a clearTopSiteURLCache() method and calling it after opening the tab in these tests fixes the issue.

	Mark Hammond [:markh] [:mhammond]
	Comment 14 • 7 years ago

Comment on attachment 8856181 [details] [diff] [review]
Avoid recomputing gBrowserThumbnails._topSiteURLs for 1 minute after computing it to speed up session restore

Review of attachment 8856181 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser-thumbnails.js
@@ +84,5 @@
>        Services.prefs.getBoolPref(this.PREF_DISK_CACHE_SSL);
>    },
>  
> +  clearTopSiteURLCache: function Thumbnails_clearTopSiteURLCache() {
> +    this._topSiteURLsRefreshTimer = null;

When this is called via tests, ISTM there might be 2 timers in-flight. It would probably be better to have notify() set |this._topSiteURLsRefreshTimer| to null, and cancel the timer here if it is non-null?

	(no longer active) Assignee
	Comment 15 • 7 years ago

(In reply to Mark Hammond [:markh] from comment #14)
> Comment on attachment 8856181 [details] [diff] [review]
> Avoid recomputing gBrowserThumbnails._topSiteURLs for 1 minute after
> computing it to speed up session restore
> 
> Review of attachment 8856181 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/browser-thumbnails.js
> @@ +84,5 @@
> >        Services.prefs.getBoolPref(this.PREF_DISK_CACHE_SSL);
> >    },
> >  
> > +  clearTopSiteURLCache: function Thumbnails_clearTopSiteURLCache() {
> > +    this._topSiteURLsRefreshTimer = null;
> 
> When this is called via tests, ISTM there might be 2 timers in-flight. It
> would probably be better to have notify() set
> |this._topSiteURLsRefreshTimer| to null, and cancel the timer here if it is
> non-null?

Good idea.

	Pulsebot
	Comment 16 • 7 years ago

Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/be41d5b2663f
Avoid recomputing gBrowserThumbnails._topSiteURLs for 1 minute after computing it to speed up session restore; r=markh

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 17 • 7 years ago
bugherder

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

	:Gijs (he/him)
	Comment 18 • 7 years ago

FWIW, in my experience the actual new tab page code is also suboptimal. It registers a bunch of places observers in multiple places ( browser/components/newtab/PlacesObserver.jsm and toolkit/modules/NewTabUtils.jsm ). It's possible life could be improved by not spending so much time reacting to *every* history/bookmark change.

	(no longer active) Assignee
	Comment 19 • 7 years ago

(In reply to :Gijs from comment #18)
> FWIW, in my experience the actual new tab page code is also suboptimal. It
> registers a bunch of places observers in multiple places (
> browser/components/newtab/PlacesObserver.jsm and
> toolkit/modules/NewTabUtils.jsm ). It's possible life could be improved by
> not spending so much time reacting to *every* history/bookmark change.

Mike, in case you want to add it to the list of areas to be profiled.  I have also anecdotally seen code related to the new tab page come up in profiles but to be honest I have never managed to delve into the details in depth...  needinfo to bring it to your attention.  :-)

	Mike Conley (:mconley) (:⚙️)
	Comment 20 • 7 years ago

(In reply to :Ehsan Akhgari (super long backlog, slow to respond) from comment #19)
> Mike, in case you want to add it to the list of areas to be profiled.  I
> have also anecdotally seen code related to the new tab page come up in
> profiles but to be honest I have never managed to delve into the details in
> depth...  needinfo to bring it to your attention.  :-)

Yes, thanks. Part of the complexity is that the Activity Stream are hoping to replace about:newtab also as part of 57. Things like bug 1353013 are important both with or without Activity Stream, and have been noted. I'm also going to be filing a bug for this timer that's registered here: http://searchfox.org/mozilla-central/rev/d8496d0a1f6ebef57fe39b9b204475b0eccfb94c/browser/base/content/newtab/page.js#97-104, but again, we'll need to evaluate it in the context of Activity Stream's likely ship time.

Certainly though, if you see more about:newtab samples show up in your profiles, send them my way.