1310361 - (SQLite3.15.0) Upgrade to SQLite 3.15.0

Status: RESOLVED FIXED

(Toolkit :: Storage, defect, P3)

Description

[NVD]

https://sqlite.org/news.html

https://sqlite.org/releaselog/3_15_0.html

SQLite version 3.15.0 is a regularly scheduled maintenance release. The key feature in this release is the added support for row values. There are also other enhancements and fixes for a number of obscure bugs.

The 3.15.0 release uses about 7% fewer CPU cycles than 3.14.2. Most of the improvement in this release is in the SQL parser, query planner, and byte-code generator (the front-end) corresponding to the sqlite3_prepare_v2() interface. Overall, version 3.15.0 uses about half as much CPU time as version 3.8.1 (2013-10-17). These performance measurements are made using the "speedtest1.c" workload on x64 compiled with gcc and -Os. Performance improvements may vary with different platforms and workloads.

Comment 1

[Ryan VanderMeulen [:RyanVM]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b47b69dabfe198b93fb2270cae845dfe9373aa6a

Comment 2

[Ryan VanderMeulen [:RyanVM]]

Attachment: Upgrade SQLite to version 3.15.0

Try looks fine, but not feeling any terrible sense of urgency in getting this reviewed and landed either. Let's give it a week or so and be on the lookout for any point releases before we commit to taking it.

Comment 3

[Ryan VanderMeulen [:RyanVM]]

Talos results are mostly uninteresting, but there does appear to be a pretty catastrophic hit to startup file IO.
https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&originalRevision=de5d73a0568d&newProject=try&newRevision=b47b69dabfe198b93fb2270cae845dfe9373aa6a&framework=1&filter=nonmain_startup_fileio&showOnlyImportant=1

Comment 4

[Joel Maher ( :jmaher ) (UTC -8)]

that is huge, it is not on the main thread, but it would be really interesting to know why we are going from ~40K bytes -> ~400K bytes for the non main thread during startup.  Is it possible that we are doing something that can be fixed with a config change or other options.  Maybe some caching mode or other fileIO internal to SQLite?  It could be that this depends on other libraries so we are loading more dll's.

Comment 5

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

The changelog doesn't raise any possible culprits, may be some internal changes. let's see if Richard has any insights about this.

Note it's also possible Talos is now measuring something that was happening just after startup and now happens earlier. But we clearly need to clarify this before we can upgrade.

Comment 6

[D. Richard Hipp]

(In reply to Marco Bonardo [::mak] from comment #5)
> let's see if Richard has any insights about this.

No ideas.  But we will puzzle over it today and see what we can come up with.  Nothing like this appears in our own instrumentation.

Am I correct in understanding that the problem only comes up on Windows7 with 32-bit compiles?

Comment 7

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

afaict yes, from what I see Ryan triggered a full run on all OSes and this is the only result that came out with a large delta.

Joel, do we have aby other talos measurement that could tell us of IO remained the same in a full run, rather than only on startup? While it would still be bad to regress startup, at least we'd know it's a shifting and not new IO.

Comment 8

[Joel Maher ( :jmaher ) (UTC -8)]

we only do this measurement on windows 7, xperf allows us to easily measure this.

I don't have overall measurements, only startup based measurements- that is a good point that we could be seeing data from post startup taking place during startup.  I don't have a lot of data from the try push, but I do not see any large changes in startup tests (ts_paint, sessionrestore)

Comment 9

[Ryan VanderMeulen [:RyanVM]]

(In reply to D. Richard Hipp from comment #6)
> No ideas.  But we will puzzle over it today and see what we can come up
> with.  Nothing like this appears in our own instrumentation.

I'm happy to run Try pushes of various intermediate revisions if you want to try bisecting it on our end. Let me know :)

Comment 10

[D. Richard Hipp]

Our best hypothesis is that sqlite3_mutex_enter() is being called on a mutex that has previously been freed.  I'm working on some patches that will assert() if this happens.  Once I get those patches going, running them on the Try server may help to confirm or refute our theory.

Comment 11

[D. Richard Hipp]

Ugh.  My previous comment was intended for [Bug 1310152], not this one.  Please ignore.

Comment 12

[D. Richard Hipp]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #9)
> 
> I'm happy to run Try pushes of various intermediate revisions if you want to
> try bisecting it on our end. Let me know :)

There are 173 check-ins in between version 3.14.2 and 3.15.0, so about 8 Trys to isolate the problem.  The first bisect can be found in https://www.sqlite.org/tmp/bug-1310361-bisect-1.zip - if you can run that on the Try server that would be great.  Let me know the result and I can give you the next one.

Should we continue the bisect off-line?  You can contact me directly at [e-mail] or by phone at 704.948.4565 if that helps.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Richard and I chatted more via email. I ran a series of Try pushes and convinced at this point that whatever we're seeing is a phantom regression. I pushed an empty commit to Try on top of mozilla-central tip and I see the exact same regression relative to m-c as I saw with the SQLite 3.15.0 update applied (the difference is negligible). So for some reason, we're doing something awful on trunk vs. Try that's causing massive perceived regressions in that test, but those don't translate to an actual problem AFAICT.

Comment 14

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

At this point, I'd suggest from now on to push current tip to Try and the Sqlite update on top of it and do a Try to Try comparison. Could we start doing that now to be 100% sure we have proper data?

Comment 15

[Joel Maher ( :jmaher ) (UTC -8)]

sorry I didn't see this earlier, but it could be related to bug 1274018.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

(In reply to Marco Bonardo [::mak] from comment #14)
> At this point, I'd suggest from now on to push current tip to Try and the
> Sqlite update on top of it and do a Try to Try comparison. Could we start
> doing that now to be 100% sure we have proper data?

That's exactly what I did in comment 13. The difference was negligible.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

FYI, I'm waiting on the outcome of bug 1310152 before pushing forward with landing here in case we end up needing a fix in SQLite.

Comment 18

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

(In reply to Ryan VanderMeulen [:RyanVM] from comment #17)
> FYI, I'm waiting on the outcome of bug 1310152 before pushing forward with
> landing here in case we end up needing a fix in SQLite.

I don't think we should block on that, from the first analysis it sounds unlikely to be a problem in SQLite, it's more likely we are doing something wrong with its mutex. Regardless, we are not making the situation worse by upgrading.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b42a7d4ec7a2
Upgrade to SQLite 3.15.0. r=mak

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b42a7d4ec7a2