Bug 1310361 (SQLite3.15.0)

Upgrade to SQLite 3.15.0

RESOLVED FIXED in Firefox 52

Status

()

Toolkit
Storage
P3
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: NVD, Assigned: RyanVM)

Tracking

unspecified
mozilla52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

(URL)

Attachments

(1 attachment)

(Reporter)

Description

a year ago
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.
(Reporter)

Updated

a year ago
Depends on: 1293367
(Assignee)

Comment 1

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b47b69dabfe198b93fb2270cae845dfe9373aa6a
Assignee: nobody → ryanvm
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Priority: -- → P3
(Assignee)

Comment 2

a year ago
Created attachment 8801486 [details] [diff] [review]
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.
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.
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.
Flags: needinfo?(drh)

Comment 6

a year ago
(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?
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.
Flags: needinfo?(jmaher)
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)
Flags: needinfo?(jmaher)
(Assignee)

Comment 9

a year ago
(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 hidden (obsolete)
Comment hidden (obsolete)

Comment 12

a year ago
(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 drh@sqlite.org or by phone at 704.948.4565 if that helps.
(Assignee)

Comment 13

a year ago
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.
Flags: needinfo?(drh)
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?
Flags: needinfo?(ryanvm)
sorry I didn't see this earlier, but it could be related to bug 1274018.
(Assignee)

Comment 16

a year ago
(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.
Flags: needinfo?(ryanvm)
(Assignee)

Comment 17

a year ago
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.
(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.
Attachment #8801486 - Flags: review+
(Assignee)

Comment 20

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b42a7d4ec7a2
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Duplicate of this bug: 1312455
(Assignee)

Updated

a year ago
Blocks: 1315244
You need to log in before you can comment on or make changes to this bug.