541373 - Provide a global VACUUM component

Status: RESOLVED FIXED

(Core :: SQLite and Embedded Database Bindings, defect)

Description

[Marco Bonardo [:mak]]

This comes from my comment here: https://bugzilla.mozilla.org/show_bug.cgi?id=383031#c18

the idea is a cpp component that tracks all services in need of vacuuming, maybe trough a nsICategoryCache.
Each service should implement a common mozPIDatabase interface, the exposes the database connection.
each service should clearly register in the "wants-vacuum" category.

The vacuum component tracks which have already been vacuumed and when, being sure to vacuum a max of 1 db per day, and not more than once a month. This could happen during daily idle notification.

on vacuum start the component notifies the service that is about to be vacuumed, so that it knows that it won't be able to write to the db till vacuum has finished.
on vacuum end the component notifies so that the service knows everything is back working.

vacuum call happens on async storage.

Any ideas about this?

Comment 1

[Jo Hermans]

The vacuum of places is done every 1 or 2 months. The code tries to be clever, it only wants to do it if enough room can be made available. In my experience this never happens (because I browse daily, and about the same number of URLS are added and removed daily ???), and the vacuum is actually triggered after 2 months. Personally, I would prefer that it didn't do this thing, and just do the vacuum once per month.

Comment 2

[Marco Bonardo [:mak]]

sure, we could simplify and just run once per month, Places impl was experimental exactly to test these kind of things. btw we are not sure that vacuuming once a month gives back advantages against vacuuming once every 2 months. It really depends on the user-type, if your way to use the browser is one hour per month, then we could even vacuum once a year, if you navigate everyday 12 hours a day, we could do once a week.
It also depends on component and database complexity. that's hard to guess for a generic component. So we could just go pessimistic and vacuum monthly as a first step, reduce if we see users complaining.

Comment 3

[Dietrich Ayala (:dietrich)]

This should also support vacuuming during or post- application update.

Comment 4

[trogdor]

Just a couple of thoughts in regards to the vacuum and auto-vacuum process.

1) "The vacuum of places is done every 1 or 2 months. The code tries to be clever,
it only wants to do it if enough room can be made available. In my experience
this never happens ....< snip>....Personally, I would prefer that it didn't do this thing, and just do"

2) "It also depends on component and database complexity. that's hard to guess for
a generic component. So we could just go pessimistic and vacuum monthly as a
first step, reduce if we see users complaining."

Just .02 cents worth:
1) How about making it part of the UI; part of  Preferences / Options (or about:config).
2) How often to compact (ie: time frame)
3) By size ( how much has the db grown )
4) By space  savings (IE: start vacuum when I can save 1 meg).
5) Compact by user on request.
6) Disable compacting because I don't care about the database size (except for Firefox version upgrades IE: point releases).

My reference point is something like the design of Thunderbird.  You have various options to compact the DB:
1) On request
2) Auto-magically when it can save a certain amount of space.

Related note; having things done auto-magically may or may not be a good idea;
Users might find that the browser will slow down or their machine may slow down when compacting.  Giving the users the option so the know when or what to expect.

At present I have seen numerous suggestions about the DB; some suggestions I find somewhat disconcerting.  Suggestions like:
1) Disable the phishing protection.
2) Delete the data base  file manually and let it re-download.
3) Let tools like CCleaner delete (if i remember correctly).

Comment 5

[Marco Bonardo [:mak]]

(In reply to comment #4)
> 1) How about making it part of the UI; part of  Preferences / Options (or
> about:config).

this is internal management, and no user is expected to deal with them, this is just something that is not reasonable to put in face of users. Sure we could provide an hidden pref still.

(In reply to comment #3)
> This should also support vacuuming during or post- application update.

I'm not sure that would help more than vacuuming once a month, what's the benefit apart making update process longer?

Comment 6

[trogdor]

"this is internal management, and no user is expected to deal with them, this is
just something that is not reasonable to put in face of users. Sure we could
provide an hidden pref still."

That will work in my humble opinion.  

Take this with a grain of salt; cookie management; deleting specific cookies & blocking cookies from non-originating web sites as well as specific sites, clearing user data, caching off line data (privacy and performance), enabling private browsing so the data doesn't gets stored, clearing cache on a time interval, selectively allowing certificates (w/out 3rd party authentication), has been exposed to the users.  T-bird has exposed the compacting of DBs as well (different project; so the analogy is not as relevant)  

Yes, I do respect that this may be more complicated for the average user.

"> This should also support vacuuming during or post- application update."

"I'm not sure that would help more than vacuuming once a month, what's the
benefit apart making update process longer?"

The basis for the statement; was in regards to performance and PROTECTION.  As the DB grows in size and potentially fragments (depends on type of vacuum used vs not used) how will the affect browser speed when the DB gets queried.  The larger the DB the longer queries time; that can potentially take longer for results to be returned.  If a larger DB negatively impacts performance then upgrades and a vacuum would restore the performance of FF in daily use.  Also, compacting the DB so that users don't do their own (potentially dangerous) workarounds; like: 

1) deleting the database waiting for a new copy to download.  That leaves end users without forgery protection for a period of time.  So it temporarily defeats the purpose of forgery protection.
2) Disabling the feature of web site forgery.  Disabling it because some users track file size opens them up to web site forgery.  Which negates the security feature that was built into FF.

Firefox is suppose to be the safest or one of the safest browsers on the web.  Users doing workarounds because of DB size or potential performance issues makes it a little less safe.  Your opinion may vary.

My sincerest apologies;  I haven't seen any sort of testing on the specific issue.  With respect to making updates take longer; it was a potential trade off on performance, security vs upgrade time.  It was a suggestion based without me doing any testing and analysis "with your specific metrics".  My testing data will not be posted and hence forth deleted because the metrics are potentially different.

If I knew how the FF teams performs a regression test with DB's of various sizes I would have done so and posted the results.  Actually I should have asked about it when my DB was at 50-60 megs.  I am not aware of the metrics that are used; sorry.

At this point I will bow out and post no further suggestions neither conduct any further testing nor provide safer workarounds (IE: using sqliteX to manually compact files).  I will leave it up to the developers and testers to give the feed back and perform the testing and regression analysis on:
1) DB size
2) Query time
3) Effects of 1 & 2 in regards to various browser performance metrics.
4) Upgrade time while balancing options 1,2 and 3 providing that any of the above is relevant.
5) Workarounds for bug reports.
 
Once again my apologies that I interfered with your process; I can assure you it won't happen again.  All the End Users probably already understand the risks of: disabling, deleting their forgery DB.  Those suggestion already liter the internet.

PS: Hopefully I  can remove my CC'ng of emails on this issues.  If not please feel  free to remove my email address from this report.

Best regards

Comment 7

[Marco Bonardo [:mak]]

(In reply to comment #6)
> Yes, I do respect that this may be more complicated for the average user.

Usually exposed options bring to a visible change, or a measurable win. this one is not measurable from the user so would be hard to setup.

> "> This should also support vacuuming during or post- application update."
> 
> "I'm not sure that would help more than vacuuming once a month, what's the
> benefit apart making update process longer?"
> 
> The basis for the statement; was in regards to performance and PROTECTION.  As
> the DB grows in size and potentially fragments (depends on type of vacuum used
> vs not used) how will the affect browser speed when the DB gets queried.

We should not forget the also VACUUMing too often hurts performances, when it comes to insert and updates. Those can indeed reuse free space that is cheaper than allocating new space.
Provided that major updates happen every 6 months, and that we would VACUUM once a month, an additional VACUUM at upgrade means just longer upgrade times to me. I thought we had alreay discarded the upgrade path due to complains about "longer updates cause users will to delay updates".

Comment 8

[(dormant account)]

The discussion here seems to be combining 2 distinct concepts. Every file-system has some block-size setting, optimizing for that yields the most compact layout on disk. On the other hand, one should never-ever read in those block sizes. They are designed for efficiently packing files only. One should always read some (large) multiple of that.

For example I've spent a lot of time investigating how binaries get paged in from disk which is equivalent to doing random io queries in sqlite. There windows is hardcoded to 8K(for really tiny data mmap areas) and 32K(for huge code areas). Linux always reads in 128K chunks and that will be bumped up soon hopefully.

For mmap. Linux and other sane Unices will bump the read size to something respectable (like 2MB) chunks when the userspace program indicates it is interested in a particular file chunk via madvise. If sqlite knows the io ranges it is about to read it should really consider doing fadvise on unix-like platforms by default.

Storage likes bulky IO, so at least for Mozilla I'd suggest 32K queries as a minium and maybe 512K chunks for databases that do  SELECT * and bulky writes.

Comment 9

[Jo Hermans]

Tark, this bug is not mentioning block sizes (or paging) at all.

Comment 10

[Shawn Wilsher :sdwilsh]

(In reply to comment #9)
> Tark, this bug is not mentioning block sizes (or paging) at all.
No, but in order for those things to apply, you need to vacuum the db.

Comment 11

[Jo Hermans]

That's Taras, not Tark. Sorry, got you confused with a co-worker.

Comment 12

[(dormant account)]

(In reply to comment #9)
> Tark, this bug is not mentioning block sizes (or paging) at all.

You are right, commented on wrong bug, sorry for the spam. Meant to go in bug  	 416330

Comment 13

[Tony Mechelynck [:tonymec]]

Another idea: Don't (or have a pref not to) vacuum at startup. I have a lot of tabs, and when loading them all at startup I need all the CPU and I/O power I can get. Actually, it has already gone so far that I get timeouts from pages read by HTTPS (from the bugzilla.mozilla.org, addons.mozilla.org, spreadsheets.google.com and mail.google.com sites) so that's a time when anything that can wait, should.

Comment 14

[Jo Hermans]

Vacuuming is done when the browser is idle, not at startup. This is just FUD.

Comment 15

[(dormant account)]

This should be a ff4 blocker

Comment 16

[Marco Bonardo [:mak]]

Attachment: patch v1.0

Let's start the feedback hell :)
This implements a vacuum manager in Storage, it is running the usual VACUUM command. It is starting on profile-after-change and listening to idle-daily. It is using a nsCategoryCache to gather partecipants that can subscribe through their manifest (the system is opt-in). There is a new interface that must be implemented by any partecipant.

Talking with Taras, we think that we should do more, in particular SQLite VACUUM does replace the database in place, but this means it won't be effective at all against system fragmentation.
So, if a small database starts getting fragmented due to its size growing (and now we can do a better job thanks to bug 581606), it won't ever be repositioned on the filesystem, resulting in a bunch of fragments even if VACUUM it (it won't be "internally" fragmented, but will be system fragmented).

Exactly VACUUM does:
- create a new temp database in tmp folder/device
- copy the old database to it
- open a transaction
- truncate the old database
- copy back the new temp database page by page to the old one
- commit the transaction

What we want is to ask the filesystem to try to find a better (less fragmented) space to reposition the database. Options to do so:
- on shutdown or startup after a VACUUM, copy the database to a new position and replace the old one.
- ask upstream to create a new file handle for the VACUUMed database.

Taras, is this correct based on our discussion?

Comment 17

[Marco Bonardo [:mak]]

Will need some simple test to ensure the system does not break.

Comment 18

[(dormant account)]

Macro, thanks for summarizing the problem/solution space. Filling in a few details below:

> Exactly VACUUM does:
> - create a new temp database in tmp folder/device
> - copy the old database to it
> - open a transaction
> - truncate the old database
> - copy back the new temp database page by page to the old one
> - commit the transaction

Nit: it copies data first, then truncates.


> 
> What we want is to ask the filesystem to try to find a better (less fragmented)
> space to reposition the database. Options to do so:
> - on shutdown or startup after a VACUUM, copy the database to a new position
> and replace the old one.
> - ask upstream to create a new file handle for the VACUUMed database.

The third option is to copy all of the databases in the profile dir on version change. 

Forth option is to has an administrator-level defrag component to firefox, but that is complex and not possible on OSX.

Comment 19

[Marco Bonardo [:mak]]

(In reply to comment #18)
> The third option is to copy all of the databases in the profile dir on version
> change. 

while this is a one shot option. I think that just copying a single db at a time (the vacuumed one) should be lighter for the user.

> Forth option is to has an administrator-level defrag component to firefox, but
> that is complex and not possible on OSX.

agree.

Comment 20

[Marco Bonardo [:mak]]

btw, looking at the options  we brought, looks like we can really split the problem in the vacuum and the relocate parts, and the relocate part does not break this one. So, since the freeze is not so far, if we want this in FX4, we should take the interfaces and vacuum, any relocation would be internal code not really affected by the API freeze.

Comment 21

[(dormant account)]

I agree with both of the above comments.

Comment 22

[Marco Bonardo [:mak]]

hm, I can remove the databaseName part from the idl, since I can gather it from conn.databaseFile
Also need to add some assertion and additional check.
Will also file a bug for the relocation problem, dependent on this one.

Comment 23

[Marco Bonardo [:mak]]

Attachment: patch v1.1

Now includes a test and should be clean enough to start reviewing. Refactored some of the code to use an internal helper class. Simplified the interface.
Vacuum is implemented for Places here.

Will push to try to check talos numbers are not affected.

Comment 24

[Andrew Sutherland [:asuth] (he/him)]

s/partecipant/participant/

Especially now that we have jsctypes, is it possible to make sure we do not vacuum when on battery?  sdwilsh has an enhancement bug on a service providing that very useful bit of information.

The courting period of the vacuumer and the participant is somewhat abrupt.  Any chance for letting the the participant introduce some asynchronous slack in there so it can finish out anything interesting it has going on?

It might be useful for the vacuum service to notify observers that some part of the application has decided to do something with an incredibly high I/O burden so that they can quiesce so they're not fighting with the vacuum.  Maybe the topic could be generic like "io-firestorm-starting" with a subject of "vacuum", followed by a "io-firestorm-done" with a subject of "vacuum".  (Those are not real suggested names, but the idea is serious.)

What is the expected/desired behaviour for "user comes back, tries to quit firefox, has really really really big database that is taking a while to vacuum"?  Given how storage currently operates and with this patch I think the answer is "during XPCOM thread shutdown the vacuum runs to completion but the user won't know whta is going on".  Should cancellation be considered?

Comment 25

[(dormant account)]

(In reply to comment #24)
> Especially now that we have jsctypes, is it possible to make sure we do not
> vacuum when on battery?  sdwilsh has an enhancement bug on a service providing
> that very useful bit of information.

Sounds like something that should be done on a higher level, ie modifying idle-daily to not run on battery.

> 
> What is the expected/desired behaviour for "user comes back, tries to quit
> firefox, has really really really big database that is taking a while to
> vacuum"?  Given how storage currently operates and with this patch I think the
> answer is "during XPCOM thread shutdown the vacuum runs to completion but the
> user won't know whta is going on".  Should cancellation be considered?

Cancellation sounds reasonable for a followup bug. I think a properly vacuumed(ie via hot copy) db will overall be fast enough to not introduce noticeable slowdown for firefox-sized databases. Thunderbird may be a different matter. This component is opt-in, so any unwieldy databases can always special-case their own vacuum.

Comment 26

[Shawn Wilsher :sdwilsh]

(In reply to comment #25)
> Cancellation sounds reasonable for a followup bug. I think a properly
> vacuumed(ie via hot copy) db will overall be fast enough to not introduce
> noticeable slowdown for firefox-sized databases. Thunderbird may be a different
> matter. This component is opt-in, so any unwieldy databases can always
> special-case their own vacuum.
I don't want to create a footgun here.  Can we get some numbers to back up our hunches please?

Comment 27

[Marco Bonardo [:mak]]

Vacuum of a database like places takes just some seconds, and happens once a month, do we really care so much of the possibility to cause a longer shutdown once a month?

(In reply to comment #24)
> s/partecipant/participant/

oops, italian fooled me

> Especially now that we have jsctypes, is it possible to make sure we do not
> vacuum when on battery?  sdwilsh has an enhancement bug on a service providing
> that very useful bit of information.

iirc that bug is sleeping. Taras suggestion seems sensible, idle-daily should not run on battery, but then we probably would never run it on portable/mobile devices? Due to the 1-month frequency probably this is not a blocking issue, something to investigate as follow-up imo.

> The courting period of the vacuumer and the participant is somewhat abrupt. 
> Any chance for letting the the participant introduce some asynchronous slack in
> there so it can finish out anything interesting it has going on?

Hm, any idea what we could do?
We could make so that if (NS_FAILED(participant->onBeginVacuum()) we bail out and move to the next database. We will retry later. It's easy and effective.

My thought was that since vacuum runs async, any sync stuff must have finished, and any async stuff is already in the pool before vacuum. But we could do what I said before.

> It might be useful for the vacuum service to notify observers that some part of
> the application has decided to do something with an incredibly high I/O burden
> so that they can quiesce so they're not fighting with the vacuum.

Yes, I was thinking about a global notification, "pandoras-box-open/closed" :p Well I'm open to suggestions, or a plain simple "heavy-io-begin/end"

Comment 28

[Marco Bonardo [:mak]]

(In reply to comment #27)
> Vacuum of a database like places takes just some seconds, and happens once a
> month, do we really care so much of the possibility to cause a longer shutdown
> once a month?

ps: this is also mitigated by the fact the shutdown must happen exactly at idle-daily

Comment 29

[Marco Bonardo [:mak]]

Also notice, we first implemented this in Places exactly to evaluate if users would have noticed it. I've not seen a single bug complaining.

Comment 30

[Shawn Wilsher :sdwilsh]

(In reply to comment #27)
> Vacuum of a database like places takes just some seconds, and happens once a
> month, do we really care so much of the possibility to cause a longer shutdown
> once a month?
Yes, this is a specific consumer.  We are making a generic API.  You are also basing "seconds" on your dev environment I presume?  What about timings for a mobile device?

Comment 31

[Marco Bonardo [:mak]]

(In reply to comment #30)
> Yes, this is a specific consumer.

One of the "big ones", so it's mostly the reference measure (along with urlclassifier3.sqlite). the API is opt-in, sure you could argue extension developers could add thousands of databases, but they can also kill our startup, shutdown and do a bunch of other weird stuff (along the awesomeness).
My system is not so fast, sure it's fast enough for me but quite old (2 years old I think). Plus we now have 32K page size that makes it quite much faster.

> basing "seconds" on your dev environment I presume?  What about timings for a
> mobile device?

Places adapts history size to the memory available on the device, for example, and most of other services do the same setting smaller limits (less cookies and so on). Sure vacuum could be slower on those devices, but I doubt there is a better place to do it than idle once a month (that it's also hard to hit on a mobile device, on the other side). We are already doing this after all, we'll just do this a bit more often since will be 1 database per day (interesting question, what happens if we have more than 30 databases registered? Some could be left out of the game).

We already discussed doing this at upgrade times, and people complained that longer upgrade times make people delay upgrades, and that's worse.

Comment 32

[(dormant account)]

(In reply to comment #30)
> Yes, this is a specific consumer.  We are making a generic API.  You are also
> basing "seconds" on your dev environment I presume?  What about timings for a
> mobile device?

Lets focus on desktop Firefox in this bug. One can always find horrible corner cases on mobile, especially in our sqlite usage. We can do per-device tweaks in followup bugs.

Comment 33

[Marco Bonardo [:mak]]

as a side note, Tryserver gave green, and talos does not show any interesting change

Comment 34

[Andrew Sutherland [:asuth] (he/him)]

(In reply to comment #27)
> Vacuum of a database like places takes just some seconds, and happens once a
> month, do we really care so much of the possibility to cause a longer shutdown
> once a month?

To clarify, my concern is not so much about the shutdown behaviour (which I believe to be reasonable, especially for initial development) as making sure:
1) we're all on the same page as to what it will be.
2) document/specify it somewhere so that people do not need to deduce the emergent properties of the system.

(And as an aside, I'm not trying to grow the requirements to support Thunderbird's current potentially absurdly large global database file; it needs stronger medicine applied to a new body. :)

Comment 35

[Marco Bonardo [:mak]]

Attachment: patch v1.2

- Replace bogus "partecipant" string with "participant"
- Make onVacuumBegin return a boolean, if true vacuum can proceed, otherwise it will skip to next participant. This allows participant to temporary opt-out if they are running some heavy task that can't be interrupted.
- Store last reached index in a pref, so that if we have more than 30 participants all of them have a chance to participate.
- Fire global "heavy-io-task" notifications with data "vacuum-begin" or "vacuum-end". Fixed test to check these.

Comment 36

[Marco Bonardo [:mak]]

If we want to make this a no-op for mobile it's fine for me, but I don't think there is a global switch for that, we should #ifdef each mobile platform in mozStorageModule.cpp, making the component not start should be enough.

Comment 37

[Marco Bonardo [:mak]]

Attachment: patch v1.3

unbitrot

Comment 38

[Shawn Wilsher :sdwilsh]

Comment on attachment 472814 [details] [diff] [review]
patch v1.3

For review comments with expandable context, please see http://reviews.visophyte.org/r/471474/

on file: storage/public/mozIStorageVacuumParticipant.idl line 15
>  * The Original Code is Oracle Corporation code.
>  *
>  * The Initial Developer of the Original Code is
>  *  Oracle Corporation
>  * Portions created by the Initial Developer are Copyright (C) 2004
>  * the Initial Developer. All Rights Reserved.

Please don't copy from another file.  Please use
http://www.mozilla.org/MPL/boilerplate-1.1/


on file: storage/public/mozIStorageVacuumParticipant.idl line 53
>    * @note If the database is using WAL journal mode and the current page size
>    *       is not the expected one, journal mode will be changed to TRUNCATE.

Why?


on file: storage/public/mozIStorageVacuumParticipant.idl line 55
>    *       The VACUUM participant will have to make sure WAL is setup again on
>    *       next application start.

Why couldn't this do that for them?


on file: storage/public/mozIStorageVacuumParticipant.idl line 58
>    *       Passing an invalid value will use default Storage page size.

We should define that value in the IDL actually, so it's clear what the
default is.


on file: storage/src/mozStorageConnection.h line 59
> #define DEFAULT_PAGE_SIZE 32768

this should be kept in sync with the SQLite one, right?  Shouldn't we add
comments in both places then?


on file: storage/src/mozStorageVacuumManager.h line 18
>  * The Initial Developer of the Original Code is the Mozilla Foundation.

There should be a newline here per http://www.mozilla.org/MPL/boilerplate-1.1/


on file: storage/src/mozStorageVacuumManager.h line 52
> class mozStorageVacuumManager : public nsIObserver

just "VacuumManager".  It's already in the mozilla::storage namespace.


on file: storage/src/mozStorageVacuumManager.cpp line 55
> // Used to notigy begin and end of a heavy io task.

typo


on file: storage/src/mozStorageVacuumManager.cpp line 82
>   bool Execute();

nit: lowercase


on file: storage/src/mozStorageVacuumManager.cpp line 180
>     // Check journal mode.  WAL journaling does not allow VACUUM to change page
>     // size, thus we have to temporary switch journal mode to TRUNCATE.

But we don't do it temporarily.  We should though


on file: storage/src/mozStorageVacuumManager.cpp line 224
>   mozIStorageBaseStatement** stmts = static_cast<mozIStorageBaseStatement**>
>     (NS_Alloc(statements.Count() * sizeof(mozIStorageBaseStatement*)));

you should just use nsTArray<mozIStorageBaseStatement*> here, and copy it
over.  No need to manage memory ourselves (also, I think you leak that array).


on file: storage/src/mozStorageVacuumManager.cpp line 245
>   NS_WARNING(NS_LITERAL_CSTRING("Unable to VACUUM database: ").get());
>   NS_WARNING(mDBFilename.get());

Dumping the error here would be very very useful.  Also might be a good idea
to PR_LOG it


on file: storage/src/mozStorageVacuumManager.cpp line 254
>   // 'PRAGMA journal_mode' statements always return a result.  Ignore it.

you should be more clear that it's a PRAGMA journal_mode = XYZ statement.  Of
course the getting will return a result.


on file: storage/src/mozStorageVacuumManager.cpp line 291
> mozStorageVacuumManager*
> mozStorageVacuumManager::GetSingleton()
> {
>   if (gMozStorageVacuumManager) {
>     NS_ADDREF(gMozStorageVacuumManager);
>     return gMozStorageVacuumManager;
>   }
>   gMozStorageVacuumManager = new mozStorageVacuumManager();
>   if (gMozStorageVacuumManager) {
>     NS_ADDREF(gMozStorageVacuumManager);
>     if (NS_FAILED(gMozStorageVacuumManager->Init())) {
>       NS_RELEASE(gMozStorageVacuumManager);
>     }
>   }
>   return gMozStorageVacuumManager;
> }                                                                            

There should be a macro for xpcom that means we shouldn't need to implement
this ourselves.


on file: storage/src/mozStorageVacuumManager.cpp line 383
>   } else if (strcmp(aTopic, OBSERVER_TOPIC_XPCOM_SHUTDOWN) == 0) {

nit: storage style is like places; }\nelse


on file: storage/test/unit/test_vacuum.js line 30
>     } else {

nit: }\nelse


on file: storage/test/unit/test_vacuum.js line 40
> function run_test()

more comments in general would be useful here.

This test also seems to be lacking in most of the cases we bail on.  Can we please get some more test coverage on that?


on file: storage/test/unit/vacuumParticipant.js line 53
comment in this file saying what test file it is used for would be very
helpful


Lastly, can you pull the places stuff out into a different bug so we can move faster on this please?

Comment 39

[Marco Bonardo [:mak]]

(In reply to comment #38)

> on file: storage/public/mozIStorageVacuumParticipant.idl line 58
> >    *       Passing an invalid value will use default Storage page size.
> 
> We should define that value in the IDL actually, so it's clear what the
> default is.

By "that value" you mean hardcoding in the idl the Storage default value (32768) or having a constant like USE_DEFAULT_PAGE_SIZE = 0 that participants can return as expectedDatabasePageSize?
I did not want to hardcode the default page size here as well because then we would have to maintain it sync in 3 files, 2 files is already bad enough.

Comment 40

[Shawn Wilsher :sdwilsh]

(In reply to comment #39)
> By "that value" you mean hardcoding in the idl the Storage default value
> (32768) or having a constant like USE_DEFAULT_PAGE_SIZE = 0 that participants
> can return as expectedDatabasePageSize?
> I did not want to hardcode the default page size here as well because then we
> would have to maintain it sync in 3 files, 2 files is already bad enough.
Why would we need to store it in three?  If we store it in the IDL, in all the storage/ source files, we can just use the IDL constant.

Comment 41

[Marco Bonardo [:mak]]

(In reply to comment #40)
> Why would we need to store it in three?  If we store it in the IDL, in all the
> storage/ source files, we can just use the IDL constant.

well it's a minor detail, but they will have to include mozIStorageParticipant.h even if they don't need it.

Comment 42

[Marco Bonardo [:mak]]

ok the page size problem is solved.

(In reply to comment #38)
> on file: storage/src/mozStorageVacuumManager.cpp line 291
> > mozStorageVacuumManager*
> > mozStorageVacuumManager::GetSingleton()
> > {
> >   if (gMozStorageVacuumManager) {
> >     NS_ADDREF(gMozStorageVacuumManager);
> >     return gMozStorageVacuumManager;
> >   }
> >   gMozStorageVacuumManager = new mozStorageVacuumManager();
> >   if (gMozStorageVacuumManager) {
> >     NS_ADDREF(gMozStorageVacuumManager);
> >     if (NS_FAILED(gMozStorageVacuumManager->Init())) {
> >       NS_RELEASE(gMozStorageVacuumManager);
> >     }
> >   }
> >   return gMozStorageVacuumManager;
> > }                                                                            
> 
> There should be a macro for xpcom that means we shouldn't need to implement
> this ourselves.

I'm already using NS_GENERIC_FACTORY_SINGLETON_CONSTRUCTOR but it needs GetSingleton implementation and I want to have a separate Init() method, while NS_GENERIC_FACTORY_CONSTRUCTOR_INIT does not make a singleton constructor. So I'd need something like a NS_GENERIC_FACTORY_SINGLETON_CONSTRUCTOR_INIT that does not exist, unless I don't see it.

Comment 43

[Shawn Wilsher :sdwilsh]

(In reply to comment #42)
> I'm already using NS_GENERIC_FACTORY_SINGLETON_CONSTRUCTOR but it needs
> GetSingleton implementation and I want to have a separate Init() method, while
> NS_GENERIC_FACTORY_CONSTRUCTOR_INIT does not make a singleton constructor. So
> I'd need something like a NS_GENERIC_FACTORY_SINGLETON_CONSTRUCTOR_INIT that
> does not exist, unless I don't see it.
Oh right, I forgot about that...

Comment 44

[Marco Bonardo [:mak]]

Attachment: patch v1.4

Fixed review comments and added a bunch of tests, removed Places changes (will file a follow-up).

Notice that to support wal unsetting and page size changes I had to run those pragmas synchronously. Trying to run them in an async batch was failing with SQLITE LOGIC ERROR 1, I guess because async statements were live on the db? So this does not have the statements array (btw, nsTArray<mozIstorageBaseStatement*> was working fine without having to alloc anything, just fyi).

Comment 45

[Shawn Wilsher :sdwilsh]

(In reply to comment #44)
> Notice that to support wal unsetting and page size changes I had to run those
> pragmas synchronously. Trying to run them in an async batch was failing with
> SQLITE LOGIC ERROR 1, I guess because async statements were live on the db?
Likely because they were inside a transaction.  You'd want to do more than one batch, I think, but I'll take a look at the code before we know for sure.

Comment 46

[Marco Bonardo [:mak]]

those statements are run only in the edge case that page size needs to be adjusted, that ideally happens just once per db in the worst case. So I'd not mind transactions too much for those.

Comment 47

[Shawn Wilsher :sdwilsh]

Comment on attachment 474015 [details] [diff] [review]
patch v1.4

This is really close to being this.  This is actually looking pretty great (way better than I ever expected us to be able to do), so thanks a ton for tackling this.  For review comments with expandable context, please see http://reviews.visophyte.org/r/474015/.

on file: storage/public/mozIStorageVacuumParticipant.idl line 50
>   // This value must stay in sync with the SQLITE_DEFAULT_PAGE_SIZE define in
>   // /db/sqlite3/src/Makefile.in

would prefer to see a javadoc style comment here.  It should also state what
the constant is and what it is used for.


on file: storage/public/mozIStorageVacuumParticipant.idl line 55
>    * The expected page size for the database.  VACUUM will try to correct the
>    * page size based on this value.

should mention that this is bytes somewhere


on file: storage/public/mozIStorageVacuumParticipant.idl line 61
>    *       The VACUUM MAnager will try to restore WAL mode, but for this to

nit: typo: MAnager

Also, please put @note before each different note.  Right now it looks like
there is only one long note here.


on file: storage/src/VacuumManager.cpp line 117
> Vacuumer::execute()

Assert that this is only on the main thread please.


on file: storage/src/VacuumManager.cpp line 129
>   nsCOMPtr<nsIFile> databaseFile;
>   mDBConn->GetDatabaseFile(getter_AddRefs(databaseFile));

databaseFile could be NULL if we have an in-memory database.  We should 1)
assert about this, and 2) return so we don't crash.

Also, please add a test for this case.


on file: storage/src/VacuumManager.cpp line 161
>   // Compare current page size with the expected one.  VACUUM can change the
>   // page size value if needed.

I think we actually want to do this check before we decide to not vacuum.  In
the rare case where the page size is non-optimal but we have recently
vacuumed, I think we'd want to try to vacuum to correct the page size.


on file: storage/src/VacuumManager.cpp line 166
>     NS_WARNING("Invalid page size requested for database, will use default ");

PR_LOG this please too.

nit: "Invalid page size requested for database, will use default for '%s'",
and this will have to be made into a string first.  As it's written, it will
be printed on two different lines.


on file: storage/src/VacuumManager.cpp line 190
>       {
>         nsCOMPtr<mozIStorageStatement> stmt;
>         rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING(
>           "PRAGMA journal_mode"
>         ), getter_AddRefs(stmt));
>         NS_ENSURE_SUCCESS(rv, false);
>         PRBool hasResult;
>         rv = stmt->ExecuteStep(&hasResult);
>         NS_ENSURE_TRUE(NS_SUCCEEDED(rv) && hasResult, false);
> 
>         rv = stmt->GetUTF8String(0, mOriginalJournalMode);
>         NS_ENSURE_SUCCESS(rv, false);
>       }

Why is this scoped?  I suspect it is because you need to at least call Reset,
but I think it'd be more clear to not scope it and explicitly call Reset.


on file: storage/src/VacuumManager.cpp line 204
>       if (mOriginalJournalMode.EqualsLiteral("wal")) {
>         // Set journal to a backwards compatible one.
>         rv = mDBConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
>           "PRAGMA journal_mode = TRUNCATE"));
>         NS_ENSURE_SUCCESS(rv, false);

This statement is one I don't really want us to do on the main thread.  Can we
do a one-off async one here?


on file: storage/src/VacuumManager.cpp line 213
>     rv = mDBConn->ExecuteSimpleSQL(
>       nsPrintfCString("PRAGMA page_size = %ld", expectedPageSize)
>     );

While this one is fine to do asynchronously, I think it's best to preserver
ordering and dump it off to the background thread too.

If push comes to shove, we can create a simple runnable to do all this for us
and just run this whole code block like this.  We can get the background
thread by casing the mozIStorageConnection to mozilla::storage::Connection and
then call getAsyncExecutionTarget on it.  In fact, that is probably the way we
want to go for this.


on file: storage/src/VacuumManager.cpp line 286
> NS_IMETHODIMP
> Vacuumer::HandleResult(mozIStorageResultSet*)
> {
>   return NS_OK;
> }

I think we want an NS_NOTREACHED here, right?


on file: storage/src/VacuumManager.cpp line 306
>     // If mOriginalJournalMode is set to WAL, it was temporarily changed and it
>     // should be restored.  Note we don't set this string if the page size does
>     // not need to be changed.

I think we should use a boolean to track if we need to update it or not. 
Having this have two meanings can lead to bugs down the line that I'd like to
avoid.


on file: storage/src/VacuumManager.cpp line 309
>     if (mOriginalJournalMode.EqualsLiteral("wal")) {
>       nsresult rv = mDBConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
>         "PRAGMA journal_mode = WAL"));
>       NS_WARN_IF_FALSE(NS_SUCCEEDED(rv), "Restoring WAL journal mode failed.");

Now we know that this is expensive to do based on your work in bug 573492. 
This needs to be done async, and we should probably not say we are done yet
either via the observer service).

Comment 48

[Marco Bonardo [:mak]]

Attachment: patch v1.5

made statements async, added memory database test (I'm not asserting but warning in this case because otherwise I could not test, it is an edge case though so that looks enough), fixed other comments.

Comment 49

[Shawn Wilsher :sdwilsh]

Comment on attachment 475359 [details] [diff] [review]
patch v1.5

This ended up being a bit more thourough than my normal reviews.  I guess not getting interrupted because I'm on a plane means I can think more about this stuff.

> NS_GENERIC_FACTORY_SINGLETON_CONSTRUCTOR(Service,
>                                          Service::getSingleton)
> NS_GENERIC_FACTORY_CONSTRUCTOR(StatementWrapper)
>-
>+NS_GENERIC_FACTORY_SINGLETON_CONSTRUCTOR(VacuumManager,
>+                                         VacuumManager::GetSingleton)
nit: getSingleton like Service please.

>+/**
>+ * mozIStorageConnectionInfo allows to share components connection and related
>+ * information on it.
>+ */
nit: wording is awkward.  Let's try this:
/**
 * This interface contains the information that the Storage service needs to
 * vacuum a database.  This interface is created as a service through the
 * category manager with the category "vacuum-participant".  Please see
 * [link to devmo wiki page] for more information.
 */
You can just create a stub page for this or fill it out and have sheppy look it over.  Either way, we should totally have a link here because we want people to use this. :)

>+  /**
>+   * [snip]  VACUUM will try to
>+   * correct the page size based on this value.
nit: "The vacuum manager will try to correct the page size during idle based on this value."

>+   * @note If the database is using WAL journal mode and the current page size
nit: "...using the WAL journal mode..."

>+   *       is not the expected one, journal mode will be changed to TRUNCATE
nit: "...one, the journal mode..."

>+   *       because WAL does not allow page size changes.
>+   *       The VACUUM Manager will try to restore WAL mode, but for this to
nit: vacuum doesn't need to be all uppercase since we aren't writing actual SQL here

>+   *       work reliably the participant must ensure to always reset statements.
probably a good idea to also say that the journal mode will remain as TRUNCATE if statements are not properly reset.

>+   * @note When a VACUUM operation starts or ends it will also be globally
>+   *       notified a "heavy-io-task" notification through the observer service
>+   *       with data "vacuum-begin" or "vacuum-end".
nit: '...it will also dispatch a global "heavy-io-task" notification through the observer service with the data argument being either "vacuum-begin" or "vacuum-end".'

Should we add an error/warning callback too so consumers can know easier?  I'm do not have strong opinions on this.

>+++ b/storage/src/VacuumManager.cpp
>+#include "mozIStorageConnection.h"
>+#include "mozStorageConnection.h"
only need mozStorageConnection here

>+#define PREF_VACUUM_BRANCH "storage.vacuum.last."
please add a comment here explaining what this is used for

>+// Time between subsequent VACUUM calls for a certain database.
>+#define VACUUM_INTERVAL_SECONDS 30 * 86400
also explain what this time is in a more human readable form.  I can't really do that math in my head so quickly, and I suspect others can't either.

>+#include "prlog.h"
move this up with the other #includes please

>+  nsresult NotifyCompletion(bool aSucceeded);
nit: storage style guide says this should start with a lowercase letter since it's not a method on an idl.

>+class BaseCallback : public mozIStorageStatementCallback
So, I think you want to implement nsISupport stuff on this, and have all classes, including Vacuumer, to inherit from it.  They all need the exact same QI, AddRef, and Release implementation, so there is no point in not having them all share the same implementation.  I know this can get hairy to do with xpcom sometimes, but pretty sure nsRunnable has code you can look at for reference.

>+
>+
>+NS_IMETHODIMP
>+BaseCallback::HandleError(mozIStorageError* aError)
nit: you seem to have two lines of space between a bunch of methods which is weird.  I don't think we do that elsewhere

>+////////////////////////////////////////////////////////////////////////////////
>+//// SilentCallback
>+
>+class SilentCallback : public BaseCallback
can you add some comments about what silent means here and what this is used for please?

>+  // We succeeded if both vacuum are WAL restore succeeded.
nit: "...vacuum and WAL restoration succeeded."

>+  // Get connection and check its validity.
nit: "Get the connection..."

>+  // Get database filename.
please elaborate why we need the filename here too please

>+  NS_ConvertUTF16toUTF8 mDBFilename(databaseFilename);
This does not do what you think it does here.  This doesn't assign to this->mDBFilename, but rather it makes a new local variable that shadows this->mDBFilename.  I think you want this instead:
mDBFilename = NS_ConvertUTF16toUTF8(databaseFilename);
You might need to use Assign though, so check the string guide (or ask someone) please.

>+  NS_ASSERTION(!mDBFilename.IsEmpty(), "Database filename cannot be empty");
move this assertion to the locations where mDBFilename is used please so we can catch bugs like above :)

>+  bool canOptimizePageSize = false;
do not need to assign here since we will return on error, or set a value.

>+    rv = stmt->ExecuteStep(&hasResult);
>+    NS_ENSURE_TRUE(NS_SUCCEEDED(rv) && hasResult, false);
Actually, split these into two different checks please.  You cannot tell which one failed in the console.

>+  // Notify we are about to start VACUUMing.  The participant can opt-out if it
nit: "Notify that we are..."

>+  // cannot handle a VACUUM at this time, then we'll move to the next one.
nit "...time, and then..."

>+  if (canOptimizePageSize) {
>+    // Check journal mode.  WAL journaling does not allow VACUUM to change page
>+    // size, thus we have to temporary switch journal mode to TRUNCATE.
nit: "...to temporarily switch the journal..."

>+    if (journalMode.EqualsLiteral("wal")) {
>+      mRestoreWAL = true;
>+      // Set journal to a backwards compatible one.
nit: journal mode

>+      nsCOMPtr<mozIStoragePendingStatement> ps;
>+      stmt->ExecuteAsync(callback, getter_AddRefs(ps));
nit: (void) result you don't check

>+    nsCOMPtr<mozIStorageAsyncStatement> pageSizeStmt;
>+    rv = mDBConn->CreateAsyncStatement(nsPrintfCString(
>+      "PRAGMA page_size = %ld", expectedPageSize
>+    ), getter_AddRefs(pageSizeStmt));
I imagine you make this an async statement because you want to make sure it runs after the possible conversion of the journal mode, correct?  If so, we should add a comment as a big warning saying that we should not run any more synchronous statements after we check for the journal mode.

>+    pageSizeStmt->ExecuteAsync(callback, getter_AddRefs(ps));
nit: (void) result you don't check

>+#ifdef PR_LOGGING
>+  {
>+    PRInt32 result;
>+    nsresult rv = aError->GetResult(&result);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+    nsCAutoString message;
>+    rv = aError->GetMessage(message);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+    PR_LOG(gStorageLog, PR_LOG_ERROR,
>+           ("VACUUM failed with error: %d '%s'", result, message.get()));
>+    PR_LOG(gStorageLog, PR_LOG_ERROR,
>+           ("Database was: '%s'", mDBFilename.get()));
I think this ends up being two different lines in the log file, which isn't ideal since you could have different threads logging possibly.

>+    // Handle errors now, it's important to notify participant before throwing.
>+    if (!stmt || !callback) {
>+      NotifyCompletion(false);
>+      return NS_ERROR_UNEXPECTED;
>+    }
note that throwing doesn't actually accomplish anything here.

>+VacuumManager*
>+VacuumManager::GetSingleton()
>+{
[snip]
>+}                                                                            
egads trailing whitespace!

>+  // Observe idle-daily to run a single vacuum per day.
>+  nsCOMPtr<nsIObserverService> os = mozilla::services::GetObserverService();
>+  NS_ENSURE_STATE(os);
>+  nsresult rv = os->AddObserver(this, OBSERVER_TOPIC_IDLE_DAILY, PR_FALSE);
Can't we actually register with the category manager for this topic instead of doing work at profile-after-change?  Seems like the better approach here.

>+  NS_ENSURE_SUCCESS(rv, rv);
>+  rv = os->AddObserver(this, OBSERVER_TOPIC_XPCOM_SHUTDOWN, PR_FALSE);
>+  NS_ENSURE_SUCCESS(rv, rv);
...and then we wouldn't have to unregister ourselves on shutdown

>+NS_IMETHODIMP
>+VacuumManager::Observe(nsISupports *aSubject,
>+                       const char *aTopic,
>+                       const PRUnichar *aData)
I think this is actually the only place where you have the pointer style correct in this patch.  We'll have a flag day after Firefox 4 ships to update the pointer style to that of the code-base style, but let's keep the module consistent for now please.

>+++ b/storage/src/VacuumManager.h
>+#ifndef VacuumManager_h_
>+#define VacuumManager_h_
I think we are supposed to do mozilla_storage_VacuumManager_h__ here too.  Same with other header files you add.

>+  /**
>+   * Obtains the VacuumManager object.
>+   */
>+  static VacuumManager* GetSingleton();
>+
>+  /**
>+   * Initializes the VacuumManager object.  Must be called just once.
>+   */
>+  nsresult Init();
nit: both of these should follow mozilla::storage::Service style (getSingleton, initialize)

>+++ b/storage/src/mozStorageConnection.h
>@@ -47,16 +47,19 @@
> 
> #include "nsString.h"
> #include "nsDataHashtable.h"
> #include "mozIStorageProgressHandler.h"
> #include "SQLiteMutex.h"
> #include "mozIStorageConnection.h"
> #include "mozStorageService.h"
> 
>+#include "mozIStorageVacuumParticipant.h"
>+#define DEFAULT_PAGE_SIZE mozIStorageVacuumParticipant::DEFAULT_PAGE_SIZE
Hmm, actually, this constant should probably be placed on mozIStorageConnection since we set it on open to this.

>+++ b/storage/test/unit/test_vacuum.js
>+/**
>+ * Returns a new nsIFile reference for a profile database.
>+ */
>+function new_file(name)
>+{
>+  let file = Services.dirsvc.get("ProfD", Ci.nsIFile);
>+  file.append(name + ".sqlite");
>+  return file;
>+}
I think, given what this does, it'd be more clear to be called new_db_file.

>+  // Change initial page size.
It is unclear why we do that for the test here, so please elaborate.

>+  else {
>+    // Set last VACUUM to a date in the past.
>+    Services.prefs.setIntPref("storage.vacuum.last.testVacuum.sqlite",
>+                              parseInt(Date.now() / 1000 - 31 * 86400));
>+    (TESTS.shift())();
Would prefer to see this in a do_execute_soon so failure stacks are easier to follow and understand (and shorter).

>+const TESTS = [
>+
>+function test1()
would really prefer you to stick with the format of most other storage tests that have descriptive names of the test as the method name.  See things like test_storage_connection.js for an example.

>+++ b/storage/test/unit/vacuumParticipant.js
>+function new_file(name)
ditto about the name.  Also, missing the javadoc comments.

>+function getDatabase(aFile)
javadoc comments please

>+    else if (aData == "wal") {
>+      try {
>+        this._dbConn.close();
>+      } catch (e) {}
nit: newline before catch (you did this elsewhere, just not in this method).  Also, catch(e) I think.

r=sdwilsh with those changes.  This will kick ass.

Comment 50

[Marco Bonardo [:mak]]

(In reply to comment #49)
> Comment on attachment 475359 [details] [diff] [review]
> >+   * @note When a VACUUM operation starts or ends it will also be globally
> >+   *       notified a "heavy-io-task" notification through the observer service
> >+   *       with data "vacuum-begin" or "vacuum-end".
> nit: '...it will also dispatch a global "heavy-io-task" notification through
> the observer service with the data argument being either "vacuum-begin" or
> "vacuum-end".'
> 
> Should we add an error/warning callback too so consumers can know easier?  I'm
> do not have strong opinions on this.

onEndVacuum passes a succeeded bool to the participant

Comment 51

[Marco Bonardo [:mak]]

(In reply to comment #49)
> >+  // Observe idle-daily to run a single vacuum per day.
> >+  nsCOMPtr<nsIObserverService> os = mozilla::services::GetObserverService();
> >+  NS_ENSURE_STATE(os);
> >+  nsresult rv = os->AddObserver(this, OBSERVER_TOPIC_IDLE_DAILY, PR_FALSE);
> Can't we actually register with the category manager for this topic instead of
> doing work at profile-after-change?  Seems like the better approach here.
> 
> >+  NS_ENSURE_SUCCESS(rv, rv);
> >+  rv = os->AddObserver(this, OBSERVER_TOPIC_XPCOM_SHUTDOWN, PR_FALSE);
> >+  NS_ENSURE_SUCCESS(rv, rv);
> ...and then we wouldn't have to unregister ourselves on shutdown

I thought about that, but I don't think that works out of the box. For it to work, idle service should have a category cache and should init the participants.
It would make MUCH sense for a lot of other services too, but has to be implemented in a separate bug.

Comment 52

[Marco Bonardo [:mak]]

filed bug 598966 to add a magic idle-daily category.

Comment 53

[Shawn Wilsher :sdwilsh]

(In reply to comment #50)
> onEndVacuum passes a succeeded bool to the participant
Oh right!  Couldn't we write test cases for a few more situations where we bail out then?

Comment 54

[Marco Bonardo [:mak]]

(In reply to comment #53)
> (In reply to comment #50)
> > onEndVacuum passes a succeeded bool to the participant
> Oh right!  Couldn't we write test cases for a few more situations where we bail
> out then?

it's tricky, I can't cause a vacuum failure on my will, I think. Will think about that some more.

So far I've addressed everything but:
- NSISUPPORTS: need to check inheritance with more attention because if we end up using the underlying class implementation of addRef we will leak. My first implementation was using a single impl and NS_ISUPPORTS_INHERITED but everything was leaking.
- adding a test for succeeded (as I said could be tricky, will let you know)
- idle-daily category (will need bug 598966)

Comment 55

[Marco Bonardo [:mak]]

Attachment: patch v1.6

checkpoint to save my work and eventually for further comments.
idl should be finalized here for SR (moved the const to mozIStorageConnection).
See comment above for what is left to do.

Comment 56

[Shawn Wilsher :sdwilsh]

(In reply to comment #54)
> - NSISUPPORTS: need to check inheritance with more attention because if we end
> up using the underlying class implementation of addRef we will leak. My first
> implementation was using a single impl and NS_ISUPPORTS_INHERITED but
> everything was leaking.
You shouldn't have to use that at all since they all QI to the same thing.  Just don't implement nsISupports on the child interfaces, and you'll be OK.  Example:
nsRunnable implements it's QI and AddRef/Release here:
http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsThreadUtils.cpp#55
And then we inherit from it in storage here:
http://mxr.mozilla.org/mozilla-central/source/storage/src/mozStorageAsyncStatementExecution.cpp#145
...and we never declare anything about nsISupports because the inheritance takes care of it for us.

Comment 57

[Marco Bonardo [:mak]]

Attachment: patch v1.7

Addressed all comments so far, in this version:
- nsISupports inheritance is fixed, and I was able to kill SilentCallback class in favor of BaseCallback (thanks for pointing me to nsRunnable)
- added a test for vacuum failure, unfortunately it is commented out due to bug 599098, but we can enable it later, once that bug is fixed.
- cleanup DEFAULT_PAGE_SIZE, some location was still using the old position in idls

Todo:
- bug 598966 - Add idle-daily category (Could be a followup ideally, but if I can fix that before, I'll be happy)

Comment 58

[Marco Bonardo [:mak]]

bug 598966 has a patch (waiting for review) so i'll attach a new part to use it.

Comment 59

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 478058 [details] [diff] [review]
patch v1.7

Looks ok, sorry for the long review -- also please don't mark the r+'d versions of patches as obsolete :-)

Comment 60

[Marco Bonardo [:mak]]

> Looks ok, sorry for the long review -- also please don't mark the r+'d versions
> of patches as obsolete :-)

sorry, unfortunately we don't have anymore the awesome jetpack from ehsan that was showing reviewed patches even if obsolete (I actually requested him to add that and it was awesome), I agree that default bugzilla behavior is not so nice.
Thanks for the sr.

Comment 61

[Marco Bonardo [:mak]]

Attachment: Part 2: use idle-daily category for startup

landing this will require bug 602871, to avoid running vacuum for each xpcshell test.

Comment 62

[Shawn Wilsher :sdwilsh]

Comment on attachment 482521 [details] [diff] [review]
Part 2: use idle-daily category for startup

> static const mozilla::Module::CategoryEntry kStorageCategories[] = {
>-  { "profile-after-change", "MozStorage Vacuum Manager", VACUUMMANAGER_CONTRACTID },
>+  { "idle-daily", "MozStorage Vacuum Manager", VACUUMMANAGER_CONTRACTID },
>   { NULL }
> };
Can you please format this more like kStorageModule below it.  Missed that before...

r=sdwilsh

Comment 63

[Marco Bonardo [:mak]]

part 1: http://hg.mozilla.org/mozilla-central/rev/7d13edc29272
part 2: http://hg.mozilla.org/mozilla-central/rev/0e4c3af3ac59

Comment 64

[Dietrich Ayala (:dietrich)]

Marco, to ease the documentor's job, can you please summarize exactly what was added/changed/removed, any configurable bits, and any behavior changes?

Comment 65

[Marco Bonardo [:mak]]

it's mostly a newly added idl called mozIStorageVacuumParticipant.idl (each method is well documented in the interface, as well as it has a brief intro), components can implement this interface and register through their manifest to the "vacuum-participant" category.

Comment 66

[Eric Shepherd [:sheppy]]

Documentation updated:

https://developer.mozilla.org/en/mozIStorageConnection#SQLite_database_page_size_constant

The article about the new interface is here:

https://developer.mozilla.org/en/XPCOM_Interface_Reference/mozIStorageVacuumParticipant

The URL specified in the IDL works; it redirects to the actual location of the document.

The article needs details about how to use it, though. I'll be working on that eventually, although it would be helpful if someone else could put something in there, as it looks like our documentation for the Category Manager and how to use manifests for stuff like this is not very good (if it's there at all; I sure couldn't find it).

Comment 67

[Marco Bonardo [:mak]]

Thanks for ther awesome work!
Do you have suggestions on where to put that additional documentation?

Comment 68

[Eric Shepherd [:sheppy]]

I'd just put it, for now, at the top of the page here:

https://developer.mozilla.org/en/XPCOM_Interface_Reference/mozIStorageVacuumParticipant

Where there's text that says "Add more details here." If it's substantial enough, we'll move it to a separate how-to page.