SafeBrowsing.readPrefs causes jank 2s after startup

RESOLVED FIXED in Firefox 56

Status

()

P1
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: florian, Assigned: tnguyen)

Tracking

(Blocks: 1 bug)

unspecified
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: #sbv4-m8)

Attachments

(1 attachment)

(Reporter)

Description

a year 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
(Assignee)

Comment 1

a year ago
I am looking at this to see if we could do anything to reduce the jank
Assignee: nobody → tnguyen
Status: NEW → ASSIGNED
Priority: -- → P1
If this is causing jank, it may be worth fixing bug 1354476 too.
See Also: → bug 1354476
(Assignee)

Comment 3

a year 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
(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 hidden (obsolete)
(Assignee)

Comment 6

a year 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
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.
Duplicate of this bug: 1375163
Comment hidden (mozreview-request)
(Assignee)

Comment 10

a year 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)
(Assignee)

Comment 11

a year 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
(Assignee)

Updated

a year ago
Attachment #8882497 - Flags: review?(francois)
(Assignee)

Updated

a year ago
Depends on: 1297614
(Assignee)

Updated

a year ago
Depends on: 1377559
(Assignee)

Updated

a year ago
No longer depends on: 1377559
(Assignee)

Updated

a year ago
Depends on: 1377559
(Assignee)

Updated

a year ago
Whiteboard: #sbv4-m8
(Assignee)

Updated

a year ago
Whiteboard: #sbv4-m8 → #sbv4-m8
(Assignee)

Comment 12

a year 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
(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.
(Assignee)

Comment 14

a year 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.
(Assignee)

Comment 15

a year 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?)
(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 hidden (mozreview-request)
Comment hidden (obsolete)
(Assignee)

Comment 19

a year 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
(Reporter)

Comment 20

a year 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.
Attachment #8882497 - Flags: review?(florian) → review-
(Assignee)

Comment 21

a year 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
Comment hidden (mozreview-request)
(Reporter)

Comment 23

a year 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!
Attachment #8882497 - Flags: review?(florian) → review+
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Keywords: checkin-needed

Comment 26

a year 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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0ee4a1c1a69e
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.