Last Comment Bug 876002 - (killSyncFormHistory) Breakdown: Remove nsIFormHistory2 so no synchronous form history code remains
(killSyncFormHistory)
: Breakdown: Remove nsIFormHistory2 so no synchronous form history code remains
Status: NEW
[snappy:p1] p=0 [qa-][good next bug][...
: addon-compat
Product: Toolkit
Classification: Components
Component: Form Manager (show other bugs)
: unspecified
: All All
: -- normal with 5 votes (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
:
:
Mentors: Mark Hammond [:markh]
Depends on: 878677 878690 878691 878692 879118
Blocks: StorageJank 987745
  Show dependency treegraph
 
Reported: 2013-05-24 15:54 PDT by (dormant account)
Modified: 2016-04-26 20:55 PDT (History)
13 users (show)
mmucci: firefox‑backlog+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
total number of sessions with fomrhistory slowsql (3.67 KB, text/plain)
2013-06-04 09:21 PDT, (dormant account)
no flags Details
number of slow sql queries(eg multiple queries per session) (3.67 KB, text/plain)
2013-06-04 09:22 PDT, (dormant account)
no flags Details
total number of sessions with formhistory slowsql (3.67 KB, text/plain)
2013-06-04 09:24 PDT, (dormant account)
no flags Details

Description (dormant account) 2013-05-24 15:54:37 PDT
i see removeEntry* and schema upgrade stuff using it in http://mxr.mozilla.org/mozilla-central/source/toolkit/components/satchel/nsFormHistory.js and schema upgrade stuff in .jsm

Marco/David, should code switching to the new async-only storage interface to prevent these kinds of omissions?
Comment 1 David Teller [:Yoric] (please use "needinfo") 2013-05-25 00:27:40 PDT
Async-only storage interface will make sure that you don't have sync code mixed with async (except for closing, see bug 874814), so I guess this would help. Mak knows that codebase better than I do, of course.
Comment 2 Marco Bonardo [::mak] 2013-05-25 05:58:20 PDT
There are various reasons that disallow us from doing schema upgrades asynchronously, the main one is that most of the services expose a raw connection, in many cases for testing but in other cases those are exposed to other components or add-ons (unfortunately). Another problem is that most of the code doing those schema migrations has very weak error checking that depends on being synchronous.
So some of the components must be carefully rewritten in better async fashion to avoid that.

Regarding formHistory, looks like mxr is again behind and not updating for whatever reason, the code is much different after bug 566746, the situation should be quite better than reported.
Comment 3 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-05-25 14:49:32 PDT
MXR looks to be up to date to me for those files.

nsFormHistory.js is clearly still be using main thread/synchronous SQLite. But AIUI that's the "old" back-end that desktop isn't be using anymore (in favor of FormHistory.jsm). The only other commitTransaction call is in the db migration in FormHistory.jsm, and that one really shouldn't be showing up frequently.

nsIFormHistory2 still has consumers in metro, android and sync, though, and I also see a user in SafariProfileMigrator.js and in one test (browser_sanitizeDialog_treeView.js). Seems to me we need to at least get bugs on file to remove all of these uses and actually get rid of nsIFormHistory2.

I wonder whether the sync consumer (services/sync/modules/engines/forms.js) is responsible for the remaining slowsql issues...

(Also, do we need a bug on file to have FormHistory.jsm use SQLite.jsm instead of rolling its own?)
Comment 4 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-05-25 15:03:54 PDT
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #3)
> The only other commitTransaction call is in the
> db migration in FormHistory.jsm, and that one really shouldn't be showing up
> frequently.

...but we should still get rid of it. As far as I can tell there isn't much reason why FormHistory.jsm's DB migration couldn't be async.
Comment 5 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-06-02 18:10:54 PDT
Mark, can you chase this down further (comment 3/comment 4)?

Maybe this is moot if we can just remove all consumers and get rid of the old component entirely, but as it is these two components (nsFormHistory.js and FormHistory.jsm) seem to operate on the same database (formhistory.sqlite), and I wonder if that's going to cause any issues. E.g. there seems to be "migration" code in both of them...
Comment 6 Mark Hammond [:markh] 2013-06-02 22:19:31 PDT
As best as Gavin and I can tell, the slowsql dashboard is showing the number of "commit transaction" instances declining - and looking at some of the other related SQL statements, we believe there are still some older nightlies out there which haven't updated since bug 566746 landed.  Once this stabilizes we should find the remaining cases are in other modules which continue to use nsIFormHistory2 - metro and sync.

So we'll treat this as a meta bug to remove nsIFormHistory2 and its implementation in nsFormHistory.js, which should kill those remaining cases.  In the meantime, we can continue to monitor that dashboard to verify our assertions above as more nightlies update themself - at which point we can re-evaluate if this bug should keep its [snappy:p1] status.
Comment 7 (dormant account) 2013-06-04 09:21:28 PDT
Created attachment 757982 [details]
total number of sessions with fomrhistory slowsql
Comment 8 (dormant account) 2013-06-04 09:22:08 PDT
Created attachment 757983 [details]
number of slow sql queries(eg multiple queries per session)
Comment 9 (dormant account) 2013-06-04 09:24:04 PDT
Created attachment 757984 [details]
total number of sessions with formhistory slowsql
Comment 10 (dormant account) 2013-06-04 09:28:22 PDT
jydoop script used to generate the data: https://github.com/mozilla/jydoop/blob/master/scripts/slowsql.py

ran it with time make ARGS="scripts/slowsql.py outputfile 20130512 20130603" hadoop
Comment 11 Marco Bonardo [::mak] 2013-06-27 12:09:49 PDT
The synchronous API should have no internal consumers in our codebase (apart dedicated tests) and warn deprecation to consumers at this point. If you can find anything else that is not a test, please file a dependency.

This bug will take care of finally removing the API and all remaining consumers, once we are comfortable with the removal, usually we wait at least a couple versions with the deprecation.
Comment 12 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-06-30 18:03:54 PDT
Data from bug 878677 comment 37 confirms that we've eliminated the majority of the form history main thread queries - good work everyone :)
Comment 13 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-08-05 12:17:55 PDT
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #12)
> Data from bug 878677 comment 37 confirms that we've eliminated the majority
> of the form history main thread queries - good work everyone :)

Note to myself since it's not obvious from bug metadata - this happened in the Firefox 24 cycle.
Comment 14 Marco Bonardo [::mak] 2014-03-28 01:50:35 PDT
this is actionable.
Comment 15 Marco Bonardo [::mak] 2015-04-09 02:40:49 PDT
Mark, are you actively working on this, or would you like to mentor the remaining work here?
Comment 16 Mark Hammond [:markh] 2015-04-09 02:46:02 PDT
Not working on it and very happy to mentor!
Comment 17 Marco Bonardo [::mak] 2015-04-09 03:20:15 PDT
Cool, when you have the time, please post a comment or dependent bugs with leftover work to do.
So far I just did a quick search
http://mxr.mozilla.org/mozilla-central/search?string=nsIFormHistory2

Note You need to log in before you can comment on or make changes to this bug.