1376591 - SafeBrowsing.readPrefs causes jank 2s after startup

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect, P1)

Description

[Florian Quèze [:florian]]

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

Comment 1

[Thomas Nguyen (:tnguyen)]

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

Comment 2

[François Marier [:francois]]

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

Comment 3

[Thomas Nguyen (:tnguyen)]

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

Comment 4

[Henry Chang [:hchang]]

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

Comment 5

[Thomas Nguyen (:tnguyen)]

(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 put into additional requestIdleCallback make it better

Comment 6

[Thomas Nguyen (:tnguyen)]

(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

Comment 7

[François Marier [:francois]]

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.

Comment 9

[Thomas Nguyen (:tnguyen)]

Attachment: Bug 1376591 - Move SafeBrowsing.init() after startup and put it into IdleDispatch Callback

We split SafeBrowsing.init() into smaller tasks and put them into requestIdleCallback

Review commit: https://reviewboard.mozilla.org/r/153528/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/153528/

Comment 10

[Thomas Nguyen (:tnguyen)]

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

Comment 11

[Thomas Nguyen (:tnguyen)]

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

Comment 12

[Thomas Nguyen (:tnguyen)]

(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

Comment 13

[François Marier [:francois]]

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

Comment 14

[Thomas Nguyen (:tnguyen)]

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

Comment 15

[Thomas Nguyen (:tnguyen)]

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

Comment 16

[François Marier [:francois]]

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

Comment 17

[Thomas Nguyen (:tnguyen)]

Comment on attachment 8882497 [details]
Bug 1376591 - Move SafeBrowsing.init() after startup and put it into IdleDispatch Callback

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/153528/diff/1-2/

Comment 18

[Thomas Nguyen (:tnguyen)]

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 put the init to better place

Comment 19

[Thomas Nguyen (:tnguyen)]

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 20

[Florian Quèze [:florian]]

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 21

[Thomas Nguyen (:tnguyen)]

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 22

[Thomas Nguyen (:tnguyen)]

Comment on attachment 8882497 [details]
Bug 1376591 - Move SafeBrowsing.init() after startup and put it into IdleDispatch Callback

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/153528/diff/2-3/

Comment 23

[Florian Quèze [:florian]]

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!

Comment 24

[Thomas Nguyen (:tnguyen)]

Comment on attachment 8882497 [details]
Bug 1376591 - Move SafeBrowsing.init() after startup and put it into IdleDispatch Callback

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/153528/diff/3-4/

Comment 25

[Thomas Nguyen (:tnguyen)]

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

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0ee4a1c1a69e
Move SafeBrowsing.init() after startup and put it into IdleDispatch Callback r=florian

Comment 27

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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