Created attachment 567921 [details] [diff] [review] patch v1.0 This has a lot of good reasons: - single point of access and management of the database - shared StatementCaches - code reuse - smaller History - services dedicated to their scope, not to maintaining database data - easier to refactor single services It also has a drawback, that is the patch size, since this touches all of Places cpp code, and related to this, possibility of regressions, but luckily we have more than 200 tests looking at us. This is an almost ready version of the patch, now pushing it to try to catch eventually missed failures and measure Talos impact. I'll also do a self-review pass considered the size.
Created attachment 568208 [details] [diff] [review] patch v1.1 A brief explanation. I created a new Database class that handles database initialization, shutdown and keeps the connection handle, plus separated statements caches for synchronous, asynchronous and off-mainthread synchronous statements. All services now keep ownership on this object, rather than relying on nsNavHistory, nsNavHistory itself has been epurated from database management stuff (next step is to remove all deprecated stuff and split out the query builder), so that now it's almost just the history service. More work can be done there, but that will come later. While there, I fixed a couple minor dangerous things that came out while rebuilding and testing everything (like re-entrance due to pref observers, ghost instance in case of service init), minor things though. The patch is huge since this touches all cpp services and all query strings. I avoided changing the order of notifications or queries for now, those can happen in future refactorings. Last try push shown green tests, the only known perf regression was Ts_shutdown due to bug 692120, since I built this patch on top of that, I'm now testing on try a decoupled patch, it's just matter of changing a few (<10) rows luckily.
Created attachment 568339 [details] [diff] [review] additional changes here I'm collecting additional changes due to try results, so the patch is not a moving target. Just a couple minor things that should be merged with the patch.
Comment on attachment 568208 [details] [diff] [review] patch v1.1 Review of attachment 568208 [details] [diff] [review]: ----------------------------------------------------------------- overall approach looks great. difficult to follow all of the moving-around of code, but i didn't see anything that screamed out as missing. yes, the tests have our back here, let's get it landed and in the hands of some testers stat. ::: toolkit/components/places/Database.cpp @@ +362,5 @@ > + > + // Initialize all the items that are not part of the on-disk database, like > + // views, temp tables and functions. The database should not be considered > + // corrupt if these fail. > + rv = InitTempTriggers(); nit: if this does other non-trigger things, should rename for clarity? ::: toolkit/components/places/History.h @@ +145,5 @@ > */ > mozIStorageConnection* GetDBConn(); > > /** > + * The database handle. Always invoke GetDBConn() before using this. who should always invoke GetDBConn()? how is it possible for that not to happen? ::: toolkit/components/places/nsPlacesImportExportService.cpp @@ -144,5 @@ > #define RESTORE_NSIOBSERVER_DATA NS_LITERAL_STRING("html") > #define RESTORE_INITIAL_NSIOBSERVER_DATA NS_LITERAL_STRING("html-initial") > > -// Maximum number of backups to retain. > -#define BROWSER_BOOKMARKS_MAX_BACKUPS_PREF "browser.bookmarks.max_backups" nice cleanup catch in this file, thanks. ::: toolkit/components/places/nsPlacesMacros.h @@ +66,2 @@ > NS_RELEASE(_sInstance); \ > + _sInstance = nsnull; \ why is this necessary?
> ::: toolkit/components/places/Database.cpp > @@ +362,5 @@ > > + > > + // Initialize all the items that are not part of the on-disk database, like > > + // views, temp tables and functions. The database should not be considered > > + // corrupt if these fail. > > + rv = InitTempTriggers(); > > nit: if this does other non-trigger things, should rename for clarity? Looks like I forgot to fix the comment after fixing the function name... > ::: toolkit/components/places/History.h > @@ +145,5 @@ > > */ > > mozIStorageConnection* GetDBConn(); > > > > /** > > + * The database handle. Always invoke GetDBConn() before using this. > > who should always invoke GetDBConn()? how is it possible for that not to > happen? this is an internal helper, so the object itself. Since mDBConn is inited lazily one should always use the getter and never the direct reference. the comment sucks though, will fix it. > ::: toolkit/components/places/nsPlacesMacros.h > @@ +66,2 @@ > > NS_RELEASE(_sInstance); \ > > + _sInstance = nsnull; \ > > why is this necessary? Actually, I discovered at my time expense, that if the instance initialization fails (ideally never, but it happened while I was coding due to my faults) we found ourselves with a dangling pointer and we badly assert on shutdown thinking that we are shutting down someone else's instance. By also nulling it out shutdown notices there isn't a global instance and just proceeds correctly without wrongly asserting.
Created attachment 569046 [details] [diff] [review] patch v1.2 Addresses comments. I'm going to attach a part 2 patch on top of this, to be reviewed apart, since the issue I pointed out before is true already. Practically favicons service does not need anymore to init history service, thus it does not. Due to this, nothing is listening to shutdown and closing the connection correctly. Due to this some tests involving only icons are failing to remove locks from places.sqlite files and the harness bails out on shutdown. I'm thus moving the global init and shutdown stuff to the shared Database object and history is just going to be a simple observer, as any other. This was planned already, so I'm just going to do it now.
Created attachment 569052 [details] [diff] [review] part 2: init and shutdown paths move init and shutdown to Database and use weak observers references where possible (avoid removals in the shutdown path). On Try now. PS: The previous run has the usual Ts_shutdown regression even without the work in bug 692120, looks like touching anything in Places causes it, we may be on a boundary. I'm checking again, but it's likely I can't do anything about it.
Comment on attachment 569052 [details] [diff] [review] part 2: init and shutdown paths Review of attachment 569052 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/places/Database.h @@ +54,5 @@ > +// Fired when initialization fails due to a locked database. > +#define TOPIC_DATABASE_LOCKED "places-database-locked" > +// Profile topics. > +#define TOPIC_PROFILE_CHANGE_TEARDOWN "profile-change-teardown" > +#define TOPIC_PROFILE_BEFORE_CHANGE "profile-before-change" For clarity, please add comment about where in the application life-cycle the profile topics are received, and how/why Places uses them.
Fwiw, this is the last try run https://tbpl.mozilla.org/?tree=Try&rev=0aa402f95040 that includes the patches in this bug, Bug 692120 and Bug 696159 (coalescing to save some resources). It's petty green and compare-talos shows only the fancy Ts shutdown regression. I'm now working on removing some more shutdown work, in a separate bug (will file it in minutes) and at that point on a clean shutdown we won't do _any_ database work, apart finalizing some statements. If it still regresses badly, I have definite proof it is a botched test and will proceed with landing this bunch of patches.
Created attachment 569219 [details] [diff] [review] patch v2.0 qfolded patch, with addressed comments.