Closed Bug 810209 Opened 7 years ago Closed 7 years ago

Initialize cookie service at startup

Categories

(Firefox OS Graveyard :: General, defect)

defect
Not set

Tracking

(blocking-basecamp:+, firefox18 fixed, firefox19 fixed)

RESOLVED FIXED
blocking-basecamp +
Tracking Status
firefox18 --- fixed
firefox19 --- fixed

People

(Reporter: jduell.mcbugs, Assigned: fabrice)

References

Details

Attachments

(1 file)

Right now we don't initialize the cookie service until we make our first genuine network request.  On desktop we usually hit the network immediately (at least for tab restore mode) but on B2G we often don't for quite some time.

I propose we put a simple call to instantiate the cookie service as part of startup, for two reasons:

1) it will shrink the window during which a user can delete an app and we "miss" deleting its cookies (bug 794978), making it nearly impossible for this to happen (you'd have to boot the phone and rush to delete an app before the async load of the SQLite db is complete: possible, but really unlikely, and I suspect good enough for V1)

2) it will make the first actual network request on the phone happen without the delay of the cookie database loading.

From JS this can be done just by

  var foo = Components.classes["@mozilla.org/cookieService;1"]

I don't know the B2G boot stuff well enough to know where is best place to put that code, though.  

Performance note:  instantiating the cookie service does a small bit of blocking main thread I/O (an fopen() of the sqlite database: that's part of the sqlite codebase, so not easy to refactor), then launches a read of the database on a separate thread.  So it should be fairly harmless to do at, say, the end of startup.

I've run this by both Taras and mconnor (cookie peer) and they're ok with it.
I propose we make this a blocker and no longer block B2G on 794978.
blocking-basecamp: --- → ?
Component: Gaia::System → General
This needs to be done in the parent process only, right?
Yes, parent-side only.  We instantiate a stub cookie service on the child, but that's cheap (just a malloc, really), so it can wait until first use.
Attached patch patchSplinter Review
This patch initialize the cookies as soon as possible, which is even before we launch the system app and the homescreen.
Assignee: nobody → fabrice
Attachment #680136 - Flags: review?(jduell.mcbugs)
Comment on attachment 680136 [details] [diff] [review]
patch

Review of attachment 680136 [details] [diff] [review]:
-----------------------------------------------------------------

When I talked to taras he seemed to like the idea of doing this at the end of startup: passing torch to him in case doing it earlier isn't agreeable to him.

bent may have an opinion too.
Attachment #680136 - Flags: review?(taras.mozilla)
Attachment #680136 - Flags: review?(jduell.mcbugs)
Attachment #680136 - Flags: feedback?(bent.mozilla)
I thought about this, this is not the most effective thing we can do here. A better way to fix this is to make our cookie storage more compact(but that's not going to happen soon enough). 
The way to speed up cookie startup overhead is to read in file with readahead to prime the cache. So before sqlite opens the file, open it and call fadvise(will_need), close it, then let sqlite do the job from fs-cache.
Otherwise we are reading 1-2MB of data in 32KB chunks, which results in terrible latency. This may result in up to  10x throughput improvement(depending on how shitty the that flash fs is)
Comment on attachment 680136 [details] [diff] [review]
patch

Without instrumenting code to provide cache-latency numbers(ie how long it takes to fetch a cookie the first time), there isn't enough data for me to say whether this is a good or a bad move.
Attachment #680136 - Flags: review?(taras.mozilla) → review-
Comment on attachment 680136 [details] [diff] [review]
patch

Review of attachment 680136 [details] [diff] [review]:
-----------------------------------------------------------------

I filed bug 810454 for the fadvise idea.

> this is not the most effective thing we can do here... terrible latency.

This bug really isn't about the latency issue so much as the app delete race.  See comment 0. Would you be willing to +r it based on that?  If you have no opinion on that I can have someone else review. (probably mconnor).
Attachment #680136 - Flags: review- → review?(taras.mozilla)
Comment on attachment 680136 [details] [diff] [review]
patch

(In reply to Jason Duell (:jduell) from comment #8)
> Comment on attachment 680136 [details] [diff] [review]
> patch
> 
> Review of attachment 680136 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I filed bug 810454 for the fadvise idea.
> 
> > this is not the most effective thing we can do here... terrible latency.
> 
> This bug really isn't about the latency issue so much as the app delete
> race.  See comment 0. Would you be willing to +r it based on that?  If you
> have no opinion on that I can have someone else review. (probably mconnor).

Sounds like mconnor the right person to review this then.
Attachment #680136 - Flags: review?(taras.mozilla) → review?(mconnor)
Attachment #680136 - Flags: review?(mconnor) → review+
https://hg.mozilla.org/mozilla-central/rev/4be2fde5e9be
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.