575357 - nsNavHistory::VacuumDatabase should check if vacuum succeeded

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[(dormant account)]

PREF_LAST_VACUUM should get updated iff a vacuum succeeded. 

I found a old profile that has never been successfully vacuumed.

Comment 1

[Stebs]

My Minefield profile has: places.last_vacuum 1267395522 which corresponds to 28 Feb 2010 22:18:42 GMT
Does this mean it really was never vacuumed (despite enough runtime in idle), or is places.last_vacuum not updated/innacurate?
Could provide profile on request.

Comment 2

[(dormant account)]

(In reply to comment #1)
> My Minefield profile has: places.last_vacuum 1267395522 which corresponds to 28
> Feb 2010 22:18:42 GMT
> Does this mean it really was never vacuumed (despite enough runtime in idle),
> or is places.last_vacuum not updated/innacurate?
> Could provide profile on request.

I assume you are running a nightly. I'd be interested in some stats:

1) What is the filesize of your places.sqlite? Back it up(in case we ever need to reproduce this)
1.5) If you have an sqlite3 console program, would be curious to see what the result of "pragma page_size;" is.
2) Then do the vacuum as in http://mozillalinks.org/wp/2009/08/vacuum-firefox-databases-for-better-performance-now-with-no-restart/
3) What is the new size of places.sqlite?

Comment 3

[Stebs]

(In reply to comment #2)

> I assume you are running a nightly.
Ah, sorry yes (forgot only opening a Bug appends this automatically):
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:2.0b2pre) Gecko/20100629 Minefield/4.0b2pre
 
> Back it up(in case we ever need to reproduce this)
done

> 1) What is the filesize of your places.sqlite?
21.094.400 Bytes
Not that big but this profile is only for testing and I had to delete history + manual vacuum (probably in Feb) because clicking History on Menu Bar still freezed Browser for several seconds (this is on Netbook).

After vacuum:
18.862.080 Bytes

> result of "pragma page_size;"
4096 (and filesystem is ntfs)
BTW, my main Firefox profile has also pragma page_size of 4096, this is a bit astonishing (since theoretically parts of it date back to "Firebird" times), but I lost overview when/if I did simply:
let Firefox update the profile
create a new profile and copy relevant files over
(more recently) make a new profile, let weave fetch everything and grab those things that weave still misses.

Comment 4

[(dormant account)]

Stebs,
Looks like everything is working as expected. The vacuum triggers based on a combination of time and some sqlite stats. Fact that vacuum didn't run in a long time means that your db isn't dirty enough yet.

Comment 5

[Stebs]

Ok thanks, thought that vacuum is triggered (at least) every 2 months, no matter what.
places.last_vacuum is still 1267395522, so is not updated with manual vacuum script.

Comment 6

[(dormant account)]

This should block ff4

Comment 7

[Dave Townsend [:mossop]]

(In reply to comment #6)
> This should block ff4

Why?

Comment 8

[(dormant account)]

(In reply to comment #7)
> (In reply to comment #6)
> > This should block ff4
> 
> Why?

Since the code assumes the vacuum completed, the user is likely to go a long time without vacuuming their db. This causes very significant performance degradation. Since it's a trivial fix, we should fix it.

Comment 9

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Actually this ifx is easy, so if possible we should first spend time on bug 541373, and if that ends up not being done in time, we can do this in some hour.

Comment 10

[Shawn Wilsher :sdwilsh]

blocking final for correctness and so our performance win actually runs.

Comment 11

[[e-mail]]

Attachment: patch

Pretty simple. Not sure if you want to take this or just wait for the vacuum service bug, but in any case, this is a blocker, and here's a patch. ;)

Passes vacuum unit tests.

Comment 12

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

yeah I'm not sure if we wish to take this and then remove the code again, but probably won't hurt anyone since the patch is done. I'm currently working on the  vacuum service bug and have a wip patch.
Btw this will hardly make b5, so I'll review on monday.

Comment 13

[Shawn Wilsher :sdwilsh]

Comment on attachment 470051 [details] [diff] [review]
patch

(In reply to comment #12)
> Btw this will hardly make b5, so I'll review on monday.
Depends on how much awesome is in the air.

>+static VacuumDBListener sVacuumDBListener;
Pretty sure taras hates it when we do this.  Make it a non-static member of nsNavHistory please?

>+  if (mVacuumInProgress)
>+    return NS_OK;
nit: places style is to brace one line ifs

>+  void SetVacuumInProgress(bool aValue)
>+  {
nit: NS_PRECONDITION(NS_IsMainThread(), ...) please

r=sdwilsh

Comment 14

[[e-mail]]

Attachment: v2

Done. Did you mean ASSERTION instead of PRECONDITION though? All the other code in navhist just asserts.

Comment 15

[Shawn Wilsher :sdwilsh]

(In reply to comment #14)
> Done. Did you mean ASSERTION instead of PRECONDITION though? All the other code
> in navhist just asserts.
I meant PRECONDITION.  It's a precondition of the method that you are on the main thread.

Comment 16

[Brendan Eich [:brendan]]

DBC is cool and all, but we've long ago stopped using NS_PRECONDITION and the other renamings of NS_ASSERTION. Why a late come-back attempt? I claim without automation there's no benefit.

But maybe someone has a static analysis in mind? It would seem to have to treat 
assertions on "initial flows" in functions and methods as preconditions.

FWIW, Rust does not distinguish pre-, post-, in-, and other positions of checks.

/be

Comment 17

[Shawn Wilsher :sdwilsh]

(In reply to comment #16)
> DBC is cool and all, but we've long ago stopped using NS_PRECONDITION and the
> other renamings of NS_ASSERTION. Why a late come-back attempt? I claim without
> automation there's no benefit.
Didn't know we prefered not to use it.  Is that documented somewhere?

> But maybe someone has a static analysis in mind? It would seem to have to treat 
> assertions on "initial flows" in functions and methods as preconditions.
Largely hoping that we can make NS_PRECONDITION actually fatal for debug builds (I don't ever get false positives on those AFAIK), but if we aren't supposed to use it, I'm fine with not doing so.

Comment 18

[Brendan Eich [:brendan]]

dbaron and Ted said on IRC that we are making progress on making assertions fatal by drawing lines in the sand: no assertions above this count for this tinderbox. The assertions that botch in my experience can be pre-conditions in all but name just as easily as not, so slicing that way may not help (it may -- I don't know).

/be

Comment 19

[[e-mail]]

Attachment: v3

For some reason, this passed locally, failed on tinderbox, and then started failing locally. :(

There were three problems: 1) 'PRAGMA journal_mode = ...' actually returns a result (the resulting journal mode), which I was asserting wouldn't happen; 2) I was skipping vacuum if an async vacuum was already in flight, which the tests don't expect; 3) the 'VACUUM' fails in the tests.

2) is easy to fix; I added the guard in to prevent rapidly sequential vacuums from thrashing the db, but that'll never happen with idle-daily. So we don't need it.

3) is strange. Nothing I've done would cause it to fail, so it's always been failing. It's probably harmless, since the tests just look for the notification to fire. The error returned is code 1, 'General SQL error or missing database'.

What do you think?

Comment 20

[[e-mail]]

I'd recommend landing this, since it makes things strictly better, and file a followup on the error condition if warranted.

Comment 21

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 470161 [details] [diff] [review]
v3

>diff --git a/toolkit/components/places/src/nsNavHistory.cpp b/toolkit/components/places/src/nsNavHistory.cpp
>--- a/toolkit/components/places/src/nsNavHistory.cpp
>+++ b/toolkit/components/places/src/nsNavHistory.cpp
>@@ -217,16 +217,54 @@ NS_IMPL_CI_INTERFACE_GETTER5(
> namespace {
> 
> static PRInt64 GetSimpleBookmarksQueryFolder(
>     const nsCOMArray<nsNavHistoryQuery>& aQueries,
>     nsNavHistoryQueryOptions* aOptions);
> static void ParseSearchTermsFromQueries(const nsCOMArray<nsNavHistoryQuery>& aQueries,
>                                         nsTArray<nsTArray<nsString>*>* aTerms);
> 
>+class VacuumDBListener : public mozIStorageStatementCallback

you should derive your class from AsyncStatementCallback, thsu you'll get handlerError for free.

>+{
>+public:
>+  VacuumDBListener(nsIPrefBranch* aBranch) : mPrefBranch(aBranch)

initializations on constructor should be on a new line.
We should probably add pref service to services.cpp as observer service and avoid this field, it is a pretty used service, btw it's not directly related to this bug.

>+
>+  NS_DECL_ISUPPORTS

I'd prefer DECL as first entry after public: for code coherence

>+  NS_IMETHOD HandleError(mozIStorageError* aError)
>+  {
>+    // Nothing to do here. We'll warn in HandleCompletion().
>+    return NS_OK;
>+  }

useless with AsyncStatementCallback

>+  NS_IMETHOD HandleCompletion(PRUint16 aReason)
>+  {
>+    if (aReason == REASON_FINISHED) {
>+      if (mPrefBranch) {
>+        (void)mPrefBranch->SetIntPref(PREF_LAST_VACUUM,
>+                                      (PRInt32)(PR_Now() / PR_USEC_PER_SEC));
>+      }
>+    } else {
>+      NS_WARNING("Database vacuum failed");

else is useless with AsyncStatementCallback since handlerError will warn

>@@ -455,16 +493,18 @@ nsNavHistory::Init()
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   // recent events hash tables
>   NS_ENSURE_TRUE(mRecentTyped.Init(128), NS_ERROR_OUT_OF_MEMORY);
>   NS_ENSURE_TRUE(mRecentLink.Init(128), NS_ERROR_OUT_OF_MEMORY);
>   NS_ENSURE_TRUE(mRecentBookmark.Init(128), NS_ERROR_OUT_OF_MEMORY);
>   NS_ENSURE_TRUE(mRecentRedirects.Init(128), NS_ERROR_OUT_OF_MEMORY);
> 
>+  mVacuumDBListener = new VacuumDBListener(mPrefBranch);


what's exactly the reason to create this at startup? we could even not run vacuum in this session.
Should be created only if needed in the vacuum execution code, and why is it stored in the history object? I'd guess passing it to the async execute should already addref it and keep it alive?

>+
>+  // Used to keep track of db vacuum operations

end comments with period please.

Comment 22

[Shawn Wilsher :sdwilsh]

Comment on attachment 470161 [details] [diff] [review]
v3

clearing request; please address mak's feedback comments

Comment 23

[[e-mail]]

Attachment: v4

Comment 24

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 470947 [details] [diff] [review]
v4

>diff --git a/toolkit/components/places/src/nsNavHistory.cpp b/toolkit/components/places/src/nsNavHistory.cpp

>+  NS_IMETHOD HandleResult(mozIStorageResultSet*)
>+  {
>+    // 'PRAGMA journal_mode' statements always return a result. Ignore it.

nit: double space after periods in comments.

code changes are ok, have you tried running test_vacuum.js, test_vacuum2.js and test_vacuum3.js? could you run them locally in case? should take a couple minutes. They are not particularly wise so should not be affected by these changes (And I'm probably going to rewrite them in Storage) but better avoiding unexpected orange if we can prevent it.

Comment 25

[[e-mail]]

Thanks. Try's chewing on it now, but yeah, all the places xpcshell tests passed locally.

I'll land this first on e10s, since I've got other stuff to land there, and merge it to m-c in one fell swoop.

Comment 26

[[e-mail]]

http://hg.mozilla.org/projects/electrolysis/rev/2d00495994f6

Comment 27

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Dan, this was merged to trunk in the last days IIRC, not sure what is the procedure to close merged bugs.

Comment 28

[[e-mail]]

Indeed -- we just reso/fix them once they get merged.

http://hg.mozilla.org/mozilla-central/rev/2d00495994f6