SafeBrowsing.readPrefs causes jank 2s after startup

RESOLVED FIXED in Firefox 56

Status

()

defect
P1
normal
RESOLVED FIXED
2 years ago
2 years 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

2 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
Assignee

Comment 1

2 years 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: → 1354476
Assignee

Comment 3

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

2 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
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

2 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)
Assignee

Comment 11

2 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
Assignee

Updated

2 years ago
Attachment #8882497 - Flags: review?(francois)
Assignee

Updated

2 years ago
Depends on: 1297614
Assignee

Updated

2 years ago
Depends on: 1377559
Assignee

Updated

2 years ago
No longer depends on: 1377559
Assignee

Updated

2 years ago
Depends on: 1377559
Assignee

Updated

2 years ago
Whiteboard: #sbv4-m8
Assignee

Updated

2 years ago
Whiteboard: #sbv4-m8 → #sbv4-m8
Assignee

Comment 12

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

2 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.
Assignee

Comment 15

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

2 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
Reporter

Comment 20

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

Comment 21

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

Comment 23

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

Updated

2 years ago
Keywords: checkin-needed

Comment 26

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