See this startup profile on a reasonably fast (1 year old i5 CPU, SSD drive) desktop computer running ubuntu: https://perf-html.io/public/cfe5c76c2f54845efece5f125c6c841c034e3a9c/calltree/?implementation=js&range=2.4996_2.6945&thread=0

I am looking at this to see if we could do anything to reduce the jank

If this is causing jank, it may be worth fixing bug 1354476 too.

Well, I see most of the things done in readPrefs are related to listmanager and update database. I would say we could put it in requestIdleCallback. We may put whole Safebrowsing.init() in requestIdleCallback which is mentioned in bug 1375163 if we could find a way to move around addMozEntries (to the first db service creation or even remove it, I am not sure) We just have to fix stuffs in LookUp which may be related to listmanager for example http://searchfox.org/mozilla-central/rev/fdb811340ac4e6b93703d72ee99217f3b1d250ac/toolkit/components/url-classifier/nsUrlClassifierDBService.cpp#1085

(In reply to Thomas Nguyen[:tnguyen] ni plz from comment #3) > Well, I see most of the things done in readPrefs are related to listmanager > and update database. > I would say we could put it in requestIdleCallback. We may put whole > Safebrowsing.init() in requestIdleCallback which is mentioned in bug 1375163 > if we could find a way to move around addMozEntries (to the first db service > creation or even remove it, I am not sure) > We just have to fix stuffs in LookUp which may be related to listmanager for > example > http://searchfox.org/mozilla-central/rev/ > fdb811340ac4e6b93703d72ee99217f3b1d250ac/toolkit/components/url-classifier/ > nsUrlClassifierDBService.cpp#1085 We can do Safebrowsing.init() in the idle cycle but it might still take too long (from the profiling data it takes 35ms which exceeds the 16ms refresh rate.) Most of the time is taken by initializing the listmanager, DBService and url formatting. Not sure how much it would help if we break the init() into small pieces.

(In reply to Henry Chang [:hchang] from comment #4) > (In reply to Thomas Nguyen[:tnguyen] ni plz from comment #3) > We can do Safebrowsing.init() in the idle cycle but it might still take > too long (from the profiling data it takes 35ms which exceeds the 16ms > refresh rate.) Most of the time is taken by initializing the listmanager, > DBService and url formatting. > > Not sure how much it would help if we break the init() into small pieces. I just intent to move addMozEntries out of Safebrowsing.init(). I think 35 ms (< 50ms) is fine as spec said. But maybe splitting the task and putting into additional requestIdleCallback make it better

After discussing this in person, we decided to split this initialization into two parts: 1- requestIdleCallback for just dbserviceinit (timeout of 2 seconds) 2- requestIdleCallback for the rest (timeout of 5 seconds) Felipe said that requestIdleCallback won't reintroduce the problems that led to us introducing the 2-second delay in bug 778855. It may run before 2 seconds are up, but it won't interfere with more important main thread work.

(In reply to François Marier [:francois] from comment #7) > After discussing this in person, we decided to split this initialization > into two parts: > > 1- requestIdleCallback for just dbserviceinit (timeout of 2 seconds) > 2- requestIdleCallback for the rest (timeout of 5 seconds) > > Felipe said that requestIdleCallback won't reintroduce the problems that led > to us introducing the 2-second delay in bug 778855. It may run before 2 > seconds are up, but it won't interfere with more important main thread work. I wrote a patch based on this idea, also removing the call of listmanager from dbserive lookup (I am afraid of we delay init SafeBrowsing too long then may have ignore some url look up at the beginning, eg : session restore)

In my new profile, it still takes 3ms (/11ms with fast computer) for the createInstance init http://bit.ly/2u7aPoi As far as I know, most of the codes in jsLib (in folder toolkit/components/url-classifier/content/moz ) are out of date. They were moved from extension when Safe Browsing became a part of Firefox Some of them are totally not used anywhere and some of them could be changed to native thing Maybe it's worth to do that

(In reply to François Marier [:francois] from comment #7) > After discussing this in person, we decided to split this initialization > into two parts: > > 1- requestIdleCallback for just dbserviceinit (timeout of 2 seconds) > 2- requestIdleCallback for the rest (timeout of 5 seconds) > > Felipe said that requestIdleCallback won't reintroduce the problems that led > to us introducing the 2-second delay in bug 778855. It may run before 2 > seconds are up, but it won't interfere with more important main thread work. Hmm, I see dbserviceinit did not take any time in the profile. Indeed, that is a service and will be initialized earlier when needed (loadURI for example), we may not have to take dbserviceinit into account here

(In reply to Thomas Nguyen[:tnguyen] ni plz from comment #12) > Hmm, I see dbserviceinit did not take any time in the profile. Indeed, that > is a service and will be initialized earlier when needed (loadURI for > example), we may not have to take dbserviceinit into account here Ok, let's address the dependent bugs and then hopefully we can mark this one as fixed afterwards.

(In reply to François Marier [:francois] from comment #13) > (In reply to Thomas Nguyen[:tnguyen] ni plz from comment #12) > > Hmm, I see dbserviceinit did not take any time in the profile. Indeed, that > > is a service and will be initialized earlier when needed (loadURI for > > example), we may not have to take dbserviceinit into account here > > Ok, let's address the dependent bugs and then hopefully we can mark this one > as fixed afterwards. I meant dbserviceinit is really important and the component manager will createInstance of it very early and automatically (when nsHttpChannel beginConnect, for example). That is unavoidable (even in the first paint, I have no idea exactly where because I don't know enough the details flow), unless we prevent dbserviceinit from being created early (it will be too risky because it may break our startup flow) The SafeBrowsing.Init() is not related to dbserviceinit, and presumably, should be moved after the first paint. In our current code, we settimeout 2s for that but 2s would not an exact number. After killing internal jank in bug 1297614 and 1377559, we should put in an observer of browser-delayed-startup-finished, of course, could put in requestIdleCallback The only problem I see is the addMozEntries will be called a bit late, but I guess it only has effect on testing things, and we all know all the data in mozEntries are actually safe.

(In reply to Thomas Nguyen[:tnguyen] ni plz from comment #14) > (In reply to François Marier [:francois] from comment #13) > > The SafeBrowsing.Init() is not related to dbserviceinit, and presumably, > should be moved after the first paint. In our current code, we settimeout 2s > for that but 2s would not an exact number. After killing internal jank in > bug 1297614 and 1377559, we should put in an observer of > browser-delayed-startup-finished, of course, could put in requestIdleCallback > The only problem I see is the addMozEntries will be called a bit late, but I > guess it only has effect on testing things, and we all know all the data in > mozEntries are actually safe. And I see SafeBrowsing.Init() is called multiple times. It would not cause any problem because we use this.initialized to prevent from being init again. I think it would be better to move SafeBrowsing.Init() to somewhere for app-starup only (for example nsBrowserGlue?)

(In reply to Thomas Nguyen[:tnguyen] ni plz from comment #14) > I meant dbserviceinit is really important and the component manager will > createInstance of it very early and automatically (when nsHttpChannel > beginConnect, for example). That is unavoidable (even in the first paint, I > have no idea exactly where because I don't know enough the details flow), > unless we prevent dbserviceinit from being created early (it will be too > risky because it may break our startup flow) Right. We need it as soon as we create a network channel. > The SafeBrowsing.Init() is not related to dbserviceinit, and presumably, > should be moved after the first paint. In our current code, we settimeout 2s > for that but 2s would not an exact number. After killing internal jank in > bug 1297614 and 1377559, we should put in an observer of > browser-delayed-startup-finished, of course, could put in requestIdleCallback Sounds like a good idea to try out. > The only problem I see is the addMozEntries will be called a bit late, but I > guess it only has effect on testing things, and we all know all the data in > mozEntries are actually safe. As long as we get these entries without a few seconds of starting up, our tests should pass. An idlecallback with a timeout of 5 seconds should work there.

Florian, could you please take a look? The internal janks will be killed in dependency bugs and the only thing I am doing here is putting the init to better place

Comment on attachment 8882497 [details] Bug 1376591 - Move SafeBrowsing.init() after startup and put it into IdleDispatch Callback https://reviewboard.mozilla.org/r/153528/#review160256 Thanks for the patch! This still doesn't move it completely off of startup, see my comment below. ::: commit-message-cc903:1 (Diff revision 2) > +Bug 1376591 - Move SafeBrowsing.init() after startup an put it into IdleCallback "an put" -> typo ::: browser/components/nsBrowserGlue.js:989 (Diff revision 2) > DirectoryLinksProvider.init(); > NewTabUtils.init(); > NewTabUtils.links.addProvider(DirectoryLinksProvider); > AboutNewTab.init(); > > + Services.tm.idleDispatchToMainThread(() => { You want to do this in the _onWindowsRestored method (near http://searchfox.org/mozilla-central/rev/f1472e5b57fea8d4a7bbdd81c44e46958fe3c1ce/browser/components/nsBrowserGlue.js#1196) rather than in _onFirstWindowLoaded.

Comment on attachment 8882497 [details] Bug 1376591 - Move SafeBrowsing.init() after startup and put it into IdleDispatch Callback https://reviewboard.mozilla.org/r/153528/#review160256 > You want to do this in the _onWindowsRestored method (near http://searchfox.org/mozilla-central/rev/f1472e5b57fea8d4a7bbdd81c44e46958fe3c1ce/browser/components/nsBrowserGlue.js#1196) rather than in _onFirstWindowLoaded. Ah, I see ContextualIdentityService did the similar thing, should be in _onFirstWindowLoaded Thanks for your review

Comment on attachment 8882497 [details] Bug 1376591 - Move SafeBrowsing.init() after startup and put it into IdleDispatch Callback https://reviewboard.mozilla.org/r/153528/#review160798 Looks good, thanks!

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/0ee4a1c1a69e Move SafeBrowsing.init() after startup and put it into IdleDispatch Callback r=florian