Closed Bug 449443 Opened 12 years ago Closed 11 years ago

Upgrade to SQLite 3.6.4

Categories

(Toolkit :: Storage, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla1.9.1b2

People

(Reporter: sdwilsh, Assigned: sdwilsh)

References

Details

(Keywords: verified1.9.1)

Attachments

(1 file, 1 obsolete file)

 
Flags: blocking1.9.1?
Depends on: 444357
Whiteboard: [needs perf analysis]
I'm only seeing a 12ms increase in startup time locally :(
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.
Summary: Upgrade to SQLite 3.6.1 → Upgrade to SQLite 3.6.2
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.
Whiteboard: [needs perf analysis]
Attached patch v1.0 (obsolete) — Splinter Review
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
Attachment #336952 - Flags: review?(mconnor)
Flags: in-testsuite-
Flags: in-litmus-
Whiteboard: [has patch][needs review mconnor]
Target Milestone: --- → mozilla1.9.1b1
Going to talk to drivers soon about landing this
Whiteboard: [has patch][needs review mconnor] → [has patch][needs review]
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
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [has patch][needs review]
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.
This got backed out.
Status: RESOLVED → REOPENED
Depends on: 455597
Resolution: FIXED → ---
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
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?
(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...
See http://shawnwilsher.com/archives/175 for details on what's going on with this regression.
No longer depends on: 455597
Depends on: 456910
Filed bug 456910 which is the likely cause of this regression.  Also, 3.6.3 is out...
Status: REOPENED → ASSIGNED
Summary: Upgrade to SQLite 3.6.2 → Upgrade to SQLite 3.6.3
Depends on: 457158
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)
Status: ASSIGNED → RESOLVED
Closed: 11 years ago11 years ago
Flags: blocking1.9.0.4?
Resolution: --- → FIXED
We have to backout bug 456910 because of a Tp regression, which means we need to back this out :(
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
SQLite is going to provide a way to use the old journal mode (truncate) for 3.6.4.

Let's try that...
No longer depends on: 456910
Summary: Upgrade to SQLite 3.6.3 → Upgrade to SQLite 3.6.4
Target Milestone: mozilla1.9.1b1 → mozilla1.9.1
Depends on: 457577
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.
Flags: blocking1.9.0.4? → blocking1.9.0.4-
(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.
Flags: blocking1.9.0.4- → blocking1.9.0.4?
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.
Flags: blocking1.9.0.4? → blocking1.9.0.4-
(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.
Attached patch v1.1Splinter Review
Updated for 3.6.4
Attachment #336952 - Attachment is obsolete: true
Depends on: 460315
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
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
No longer depends on: 460315
Resolution: --- → FIXED
Target Milestone: mozilla1.9.1 → mozilla1.9.1b2
No longer depends on: 457577
Depends on: 460315
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.
Attachment #343407 - Flags: approval1.9.0.5?
Comment on attachment 343407 [details] [diff] [review]
v1.1

Approved for 1.9.0.5, a=dveditz for release-drivers
Attachment #343407 - Flags: approval1.9.0.5? → approval1.9.0.5+
Flags: blocking1.9.1? → blocking1.9.1+
Depends on: 464299
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
Keywords: fixed1.9.0.5
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
Keywords: fixed1.9.0.5
Attachment #343407 - Flags: approval1.9.0.5+ → approval1.9.0.5-
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.