Florian Quèze [:florian] Reporter
	Description • 7 years ago

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

	Thomas Nguyen (:tnguyen) Assignee
	Comment 1 • 7 years ago

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

	François Marier [:francois]
	Comment 2 • 7 years ago

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

	Thomas Nguyen (:tnguyen) Assignee
	Comment 3 • 7 years ago

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

	Henry Chang [:hchang]
	Comment 4 • 7 years ago

(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.

	Thomas Nguyen (:tnguyen) Assignee
	Comment 6 • 7 years ago

(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

	François Marier [:francois]
	Comment 7 • 7 years ago

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.

	Thomas Nguyen (:tnguyen) Assignee
	Comment 10 • 7 years ago

(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)

	Thomas Nguyen (:tnguyen) Assignee
	Comment 11 • 7 years ago

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

	Thomas Nguyen (:tnguyen) Assignee
	Comment 12 • 7 years ago

(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

	François Marier [:francois]
	Comment 13 • 7 years ago

(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.

	Thomas Nguyen (:tnguyen) Assignee
	Comment 14 • 7 years ago

(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.

	Thomas Nguyen (:tnguyen) Assignee
	Comment 15 • 7 years ago

(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?)

	François Marier [:francois]
	Comment 16 • 7 years ago

(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.

	Thomas Nguyen (:tnguyen) Assignee
	Comment 19 • 7 years ago

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

	Florian Quèze [:florian] Reporter
	Comment 20 • 7 years ago
mozreview-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/#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.

	Thomas Nguyen (:tnguyen) Assignee
	Comment 21 • 7 years ago
mozreview-review-reply

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

	Florian Quèze [:florian] Reporter
	Comment 23 • 7 years ago
mozreview-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!

	Thomas Nguyen (:tnguyen) Assignee
	Comment 25 • 7 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3a8839edb79b7b6f830bdee041e4405d6aad4683

	Pulsebot
	Comment 26 • 7 years ago

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

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 27 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/0ee4a1c1a69e