449443 - Upgrade to SQLite 3.6.4

Status: VERIFIED FIXED

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

Description

[Shawn Wilsher :sdwilsh]

Comment 1

[Shawn Wilsher :sdwilsh]

I'm only seeing a 12ms increase in startup time locally :(

Comment 2

[Shawn Wilsher :sdwilsh]

try server Ts numbers are not giving me consistent results at all.  Latest shows a very small Ts regression on windows and linux - nothing like we saw before.  Previous build (should have been no changes) on mac regressed Ts badly, but went back down with the second build.  Weird.

Comment 3

[Shawn Wilsher :sdwilsh]

So, the try server is only showing a regression on mac now...
http://graphs-stage.mozilla.org/graph.html#show=184032,117694,139323

Mac has been real noisy, but it wasn't a problem in the past.  I think we should go ahead and try this on central again.

Comment 4

[Shawn Wilsher :sdwilsh]

Attachment: v1.0

This contains only the changes to mozilla code.  The sqlite patch can be seen here:
http://hg.mozilla.org/users/sdwilsh_shawnwilsher.com/storage-async/file/3f8b1e50519e/sqlite.upgrade

Comment 5

[Asaf Romano (gone)]

Comment on attachment 336952 [details] [diff] [review]
v1.0

r=mano

Comment 6

[Shawn Wilsher :sdwilsh]

Going to talk to drivers soon about landing this

Comment 7

[Shawn Wilsher :sdwilsh]

I pushed this to mozilla-central since the try server showed no performace regressions (maybe a small one on Ts for Linux, but the numbers are a bit noisy):
http://hg.mozilla.org/mozilla-central/rev/572549fd6be3
http://hg.mozilla.org/mozilla-central/rev/789b491cc8d4

Comment 8

[Shawn Wilsher :sdwilsh]

This might get backed out due to a Linux Ts regression.  Still in talks with drivers.  It's pretty bad (1650ms - 2500ms), but we aren't sure if it's infrastructure related yet since the try server didn't show this.

Comment 9

[Shawn Wilsher :sdwilsh]

This got backed out.

Comment 10

[[e-mail]]

VC7.1 doesn't like this for two reasons:
1. sqlite.c has more than 65535 lines (VC7.1 debug information limit warning)
2. VC7.1 doesn't understand __declspec(deprecated("was declared experimental"))
   at all. Please can you change the SQLITE_EXPERIMENTAL guard condition to:
#elif defined(_MSC_VER) && _MSC_VER >= 1400

Comment 11

[Peter Weilbacher]

Hehe, my name will stay in README.MOZILLA forever. :-)

Shawn, did you find out why there was no such Ts regression visible on the try servers? Any idea why Windows was not affected or not as much as last time?

Comment 12

[Shawn Wilsher :sdwilsh]

(In reply to comment #10)
> VC7.1 doesn't like this for two reasons:
> 1. sqlite.c has more than 65535 lines (VC7.1 debug information limit warning)
> 2. VC7.1 doesn't understand __declspec(deprecated("was declared experimental"))
>    at all. Please can you change the SQLITE_EXPERIMENTAL guard condition to:
> #elif defined(_MSC_VER) && _MSC_VER >= 1400
You should file a bug with the SQLite folks on that.  We've got a pretty strict policy of not taking changes to sqlite3.c or sqlite3.h

(In reply to comment #11)
> Shawn, did you find out why there was no such Ts regression visible on the try
> servers? Any idea why Windows was not affected or not as much as last time?
We don't know to all of the above.  We have, however, found a linux box that shows the same behavior mozilla-central's tinderbox shows, so I'm going to try and play with that today.  Not sure what tools I can use short of strace at the moment, but it's something to try...

Comment 13

[Shawn Wilsher :sdwilsh]

See http://shawnwilsher.com/archives/175 for details on what's going on with this regression.

Comment 14

[Shawn Wilsher :sdwilsh]

Filed bug 456910 which is the likely cause of this regression.  Also, 3.6.3 is out...

Comment 15

[Shawn Wilsher :sdwilsh]

Trying again:
http://hg.mozilla.org/mozilla-central/rev/7e8ed11500ae
http://hg.mozilla.org/mozilla-central/rev/cac36f3599e0

Comment 16

[Shawn Wilsher :sdwilsh]

Alright - I'm going to go ahead and mark this as fixed.  Hurray!

The leak issue that has cropped up will be tracked in bug 457158 (it has an easy fix if the cause isn't obvious)

Also, we very likely want to take this (and bug 456910) on branch to fix a number of crashers, so nominating (although this will need a new branch patch)

Comment 17

[Shawn Wilsher :sdwilsh]

We have to backout bug 456910 because of a Tp regression, which means we need to back this out :(

Comment 18

[Shawn Wilsher :sdwilsh]

SQLite is going to provide a way to use the old journal mode (truncate) for 3.6.4.

Let's try that...

Comment 19

[Daniel Veditz [:dveditz]]

I dubious we would even _approve_ this for 1.9.0, but we're certainly not going to block on it. Especially since it had to be backed out of trunk and we're in a "let's try that" mode.

Comment 20

[Shawn Wilsher :sdwilsh]

(In reply to comment #19)
> I dubious we would even _approve_ this for 1.9.0, but we're certainly not going
> to block on it. Especially since it had to be backed out of trunk and we're in
> a "let's try that" mode.
Please re-read the bug and take a look at the list of bugs this bug blocks.  Upgrading SQLite fixes a number of crashes on branch.  Additionally, with 3.6.4 (which we haven't landed in the tree), we should be able to regain the perf hit that we took when we upgraded to 3.5.9 for 1.9.0.

Comment 21

[Samuel Sidler (old account; do not CC)]

Er, looking at the bugs that block this...

  * bug 417037 is already fixed on the branch
  * bug 443577 is a crash (at best, #40 overall)
  * bug 444446 is a crash (part of #15 overall)

The remaining bugs are not crashes. Because of where bug 444446 crashes, it's likely not the *only* crash with that stack signature, which makes it not a topcrash overall. The perf hit, while unfortunate, is not major enough for this to become a blocker.

Because upgrading sqlite doesn't fix a topcrash or a security bug (afaict), this bug is not a blocker, though we will consider it after it successfully lands on the trunk, bakes, and has no open regressions. Please request approval on an appropriate patch (or patch placeholder) when that is the case.

Note also that upgrading sqlite likely needs some bake time on the branch as well and we're unlikely to even consider approval at the last minute.

Comment 22

[Daniel Veditz [:dveditz]]

(In reply to comment #20)
> Please re-read the bug [...]

I did, and got stuck on comment 9 and comment 17 again. We've had a string of bumpy "stable" releases and aren't taking 'unbaked' fixes, let alone those which got backed out not once but twice.

In any case this change falls outside the newly narrowed criteria that we're using for branch so you'll need to request special dispensation from mconnor even after a successful trunk landing.

Comment 23

[Shawn Wilsher :sdwilsh]

Attachment: v1.1

Updated for 3.6.4

Comment 24

[Shawn Wilsher :sdwilsh]

Looks to me like we are OK on perf.  I'm going to close this now, but I'll go through the dependents tomorrow when I'm 100% sure this didn't cause any regressions.

http://hg.mozilla.org/mozilla-central/rev/1ca16712f8a0
http://hg.mozilla.org/mozilla-central/rev/ad619c7d7f2e

Comment 25

[Shawn Wilsher :sdwilsh]

Comment on attachment 343407 [details] [diff] [review]
v1.1

We should probably take this on branch.  We have not seen any regressions or bad behavior on mozilla-central.  This also fixes a few crashers on branch, and will gets us back the performance regression we took when we upgrade to 3.5.9.

We will also need bug 460315 if we take this so we do not keep the Ts regression that we introduced in 3.5.9 upgrade.

Comment 26

[Daniel Veditz [:dveditz]]

Comment on attachment 343407 [details] [diff] [review]
v1.1

Approved for 1.9.0.5, a=dveditz for release-drivers

Comment 27

[Shawn Wilsher :sdwilsh]

Checking in configure.in;
new revision: 1.2000; previous revision: 1.1999
Checking in db/sqlite3/README.MOZILLA;
new revision: 1.29; previous revision: 1.28
Checking in db/sqlite3/src/sqlite.def;
new revision: 1.13; previous revision: 1.12
Checking in db/sqlite3/src/sqlite3.h;
new revision: 1.23; previous revision: 1.22
Checking in db/sqlite3/src/sqlite3.c;
new revision: 1.20; previous revision: 1.19

Comment 28

[Shawn Wilsher :sdwilsh]

This was backed out due to a possible dataloss regression.

Checking in configure.in;
new revision: 1.2001; previous revision: 1.2000
Checking in db/sqlite3/README.MOZILLA;
new revision: 1.30; previous revision: 1.29
Checking in db/sqlite3/src/sqlite.def;
new revision: 1.14; previous revision: 1.13
Checking in db/sqlite3/src/sqlite3.h;
new revision: 1.24; previous revision: 1.23
Checking in db/sqlite3/src/sqlite3.c;
new revision: 1.21; previous revision: 1.20

Comment 29

[Tony Chung [:tchung]]

1.9.1 and trunk are up to 3.6.10.  marking bug verified.

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre)
Gecko/20090407 Shiretoko/3.5b4pre

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre)
Gecko/20090407 Minefield/3.6a1pre