690903 - nsFormHistory should lazily initializes SQLite DB

Status: RESOLVED FIXED

(Toolkit :: Form Manager, defect)

Description

[Mark Finkle (:mfinkle) (use needinfo?)]

Currently, the SQLite DB is opened (and created if needed) during the component initialization. If we could make this lazy, we could move the SQLite blocking file process out of the startup path.

This affects Firefox Mobile on phones with slow file systems quite a bit.

Comment 1

[Mounir Lamouri (:mounir)]

You would like to initialize the database the first time we try to access it instead of during startup, right? Is it better to push the task to a page load instead of app load? I really wonder what would be the worse in term of user experience.

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #1)
> You would like to initialize the database the first time we try to access it
> instead of during startup, right? Is it better to push the task to a page
> load instead of app load? I really wonder what would be the worse in term of
> user experience.

Page load would still be too early for Fennec. We manually initialize form history in a page load now. I was hoping we could delay until some call actually needed to use the DB. This is kinda how login manager works.

I have noticed that login manager and form history both use sync calls to SQLite during initialization, which hurts.

Comment 3

[Mounir Lamouri (:mounir)]

Doing the call asynchronously might require a lot of changes.
If I understood correctly the numbers in the bug Vivien put in the blocking list, the thing that really hurts is the first run that creates the database? In that case, I guess lazily initializing the database should be okay given that except when creating them, this will be seamless. At least, seems worthy enough for me to try to write a patch.

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

Yes, creating the DB is a _big_ hit on some phones (I'm looking at you Galaxy S), but even opening the DB connection every time we load is a noticeable hit. And opening the DB right as the home page loads means we have some blocking I/O right when the user wants to start using the app.

Comment 5

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

It seems to work but I didn't compute the speed gain. Though, on my dev phone (Galaxy S2), it's unlikely to be high.

Comment 6

[Mounir Lamouri (:mounir)]

To make sure this is clear, with this patch, nsFormHistory database will not be initialized on app load but the first time the user will select a text field in the content. On a fast phone it is barely visible. On a slow one, it will likely be visible. It's a trade-off to take or not...

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

Would it make any sense to convert all uses of:

this.dbConnection

to:

this.DBConnection

That might make it less painful to try to add a call to lazyDBInit()

Comment 8

[Mounir Lamouri (:mounir)]

Attachment: Patch v2

I'm ashamed... I should have think about that :(

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 564497 [details] [diff] [review]
Patch v2


>     dbMigrateToVersion3 : function () {
>+        this.lazyDbInit();
>+

Any reason you left this call here?

I might have changed this.dbConnection -> this._dbConnection, but it's your call on that.

Otherwise, looks good to me.

Comment 10

[Mounir Lamouri (:mounir)]

Attachment: Patch v2.1

f=mfinkle

Renaming dbConnection to _dbConnection. Removing the call I forgot to remove.

Thanks for those advices, I believe it's quite obvious I'm not a JS hacker :)

Comment 11

[Mounir Lamouri (:mounir)]

Dolske, I think this patch is pretty easy and would be a high value for the Mobile Startup Shrink effort. When do you think you can review it?

Comment 12

[Marco Bonardo [:mak]]

the storage usage in this component seems confusing, I hope bug 566746 will do a better work on it, I don't really see reasons why this is synchronous, are there valid use-cases requiring that?

Why do you need both dbConnection and DBConnection? couldn't you just have DBConnection and use getOwnPropertyDescriptor to check if it's a getter yet?

Comment 13

[Marco Bonardo [:mak]]

why do you invoke lazyDBInit() in dbMigrateToVersion3 if you immediately use DBConnection and the code uses DBConnection even before invoking dbMigrateToVersion3. I'd expect that to throw on migration

Comment 14

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #10)
> Created attachment 564529 [details] [diff] [review] [diff] [details] [review]
> Patch v2.1

Mounir - I think you attached the wrong patch

Comment 15

[Mounir Lamouri (:mounir)]

Attachment: Patch v2.1

Comment 16

[Marco Bonardo [:mak]]

Sorry, I'm annoying :) I still don't understand why you need _dbConnection, dbInit as well as lazyDBInit may just use a local var and return the created connection so that DBConnection just returns this.DBConnection = this.lazyDbInit();
dbInit may even be defined as an helper function inside lazyDBInit.

Comment 17

[Justin Dolske [:Dolske]]

Comment on attachment 564767 [details] [diff] [review]
Patch v2.1

>+    // The dabatase connection. You should use this.DBConnection instead,
>+    // except in |get DBConnection| and in |lazyDbInit()| and |dbInit()|.
>+    _dbConnection : null,

No one will ever read this comment. :)

It would be a lot simpler do just make |dbCommection| a self-memoizing getter and not rename it everywhere. That just needlessly inflates the diff.

 
But, really, zpao and I talked about then when rewriting it. There wasn't any point in doing this, because everything in nsIFormHistory requires the DB. In other words, why is some caller triggering the creation of the component, and is it just going to immediately call an API anyway?

Comment 18

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Justin Dolske [:Dolske] from comment #17)
 
> But, really, zpao and I talked about then when rewriting it. There wasn't
> any point in doing this, because everything in nsIFormHistory requires the
> DB. In other words, why is some caller triggering the creation of the
> component, and is it just going to immediately call an API anyway?

In Firefox Mobile (multi-process), we init the component so the child process scripts can get loaded - not because we want to use the component. But nsFormHistory.init() does both. If we don't load the child process scripts, the content will never get the form submit code activated.

Comment 19

[Mounir Lamouri (:mounir)]

Attachment: Patch v3

I will have to double-check that the initialization is actually done once. I'm quite not sure about this...

Comment 20

[Marco Bonardo [:mak]]

Object.getOwnPropertyDescriptor(this, "dbConnection").value === undefined can tell you if dbConnection is still a getter in dbInit(), if it's not then something is invoking it wrongly.

Comment 21

[Mounir Lamouri (:mounir)]

Attachment: Patch v3.1

This new patch should take into account all comments.

Comment 22

[Mounir Lamouri (:mounir)]

BTW, might be interesting to remove DBConnection getter, it's pretty useless. Though, it's a pain to check where it's currently used. Looking for ".DBConnection" returns 172 results in mxr... I'm too lazy to check which ones are related to FormHistory.

Comment 23

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #22)
> BTW, might be interesting to remove DBConnection getter, it's pretty
> useless. Though, it's a pain to check where it's currently used. Looking for
> ".DBConnection" returns 172 results in mxr... I'm too lazy to check which
> ones are related to FormHistory.

I'm pretty sure Sync uses it directly, at least until we have async APIs.

Comment 24

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Paul O'Shannessy [:zpao] from comment #23)
> (In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #22)
> > BTW, might be interesting to remove DBConnection getter, it's pretty
> > useless. Though, it's a pain to check where it's currently used. Looking for
> > ".DBConnection" returns 172 results in mxr... I'm too lazy to check which
> > ones are related to FormHistory.
> 
> I'm pretty sure Sync uses it directly, at least until we have async APIs.

Anything against using one form vs another? (|FormHistory.dbConnection| vs |formHistory.DBConnection|)?

I vote for |FormHistory.DBConnection| to not break everything in the outside world, and I agree with Mounir's that having two names that closed is confusing.

Comment 25

[Mounir Lamouri (:mounir)]

(In reply to Vivien Nicolas (:vingtetun) from comment #24)
> (In reply to Paul O'Shannessy [:zpao] from comment #23)
> > (In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #22)
> > > BTW, might be interesting to remove DBConnection getter, it's pretty
> > > useless. Though, it's a pain to check where it's currently used. Looking for
> > > ".DBConnection" returns 172 results in mxr... I'm too lazy to check which
> > > ones are related to FormHistory.
> > 
> > I'm pretty sure Sync uses it directly, at least until we have async APIs.
> 
> Anything against using one form vs another? (|FormHistory.dbConnection| vs
> |formHistory.DBConnection|)?
> 
> I vote for |FormHistory.DBConnection| to not break everything in the outside
> world, and I agree with Mounir's that having two names that closed is
> confusing.

.dbConnection is indeed only used internally. Once this patch is landed I will open a follow-up to rename dbConnection to DBConnection internally and remove dbConnection.

Comment 26

[Justin Dolske [:Dolske]]

Comment on attachment 566229 [details] [diff] [review]
Patch v3.1

I'd actually like to eventually move to some better way of having things hook up their listeners on startup... But I'm not sure what that would look like, exactly, so this is quite fine until then!

Comment 27

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/86b1bbf94216