1313021 - Use a MEMORY SQLITE_TEMP_STORE on Android and 64bit builds

Status: RESOLVED FIXED

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

Description

[Marco Bonardo [:mak]]

In general, today's desktop platforms shouldn't have big issues with memory, so I'd like to default to a MEMORY temp store for them (I'd basically just exclude android).
This may help a bit with perf, and those cases where the temp partition becomes full.
Thoughts?

Comment 1

[Jan Varga [:janv]]

I believe we don't track temp stuff by IDB or quota manager, so it should be ok from this point of view.

Comment 2

[Marco Bonardo [:mak]]

note that some queries automatically create temporary views in that store (for sorting, for example), even if you don't use temp tables or such.

Comment 3

[Jan Varga [:janv]]

do you know what file extension SQLite uses for these temp stores ?

Comment 4

[Marco Bonardo [:mak]]

no, all I know is written in https://www.sqlite.org/tempfiles.html and more specifically Section 5 points out the destination of the files, not names nor extension (provided it uses an extension at all, I'd not expect it to).
If you need more details you likely must ask Sqlite Support.

Comment 5

[Andrew Sutherland [:asuth] (he/him)]

We have a real problem with address-space exhaustion/fragmentation on 32-bit Firefox, so I'd say let's enable this only on 64-bit non-fennec.  In the event the user experiences memory pressure, the OS can page the memory out as it needs to thanks to the power of virtual memory.  (A similar rationale holds for restricting use of SQLite's mmap support to 64-bit builds; see bug 1143308 for the IndexedDB bug on that.)

Note: Before I settled on address space being the more important issue, I did speak with :pdol, :frank, and :robotblake in #telemetry because I could've sworn I'd recently heard that many of our users only have 1 gig of memory.  It turns out that was wrong.  The take-aways were these:
- :pdol said:
  - "Most users have at least 2gigs."
  - "If it helps, we will be rolling the 64 bit version out to any user who has at least 3.8GBs of RAM on Windows."
- The stats at https://sql.telemetry.mozilla.org/queries/1525#table (some kind of LDAP required, maybe MoCo).  My script is based on pdol's https://sql.telemetry.mozilla.org/queries/1217 but using :robotblake's suggested approx_distinct() so that a machine only has to show up 1 day during the 7 day window rather than all 7 days in order to count for one full unit.  It uses the 1% "longitudinal" dataset which is a random sample and so shouldn't have biases, but 1% is obviously not 100%.  If anyone wants better data/etc. I'd suggest asking the helpful crew in #telemtry.

Comment 6

[Marco Bonardo [:mak]]

Thanks, that's useful data. Yes, even just enabling this for 64bit would help, since we already plan to move some users to it, and it's a shrinking market.
We could even detect memory and use a pragma depending on that, but sounds like an overkill.

While looking around for info, I hit these:
https://codereview.chromium.org/10809015/
https://android.googlesource.com/platform/external/sqlite/+/master/dist/Android.mk
Looks like Android doesn't have a temp dir, and as such Sqlite should always use SQLITE_TEMP_STORE=3.
Fwiw I don't think our android code uses Storage at all, but at this point it should also use that setting.

TL;DR
SQLITE_TEMP_STORE=3 on Android
SQLITE_TEMP_STORE=2 on 64bit

Comment 7

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Marco Bonardo [::mak] from comment #6)
> SQLITE_TEMP_STORE=3 on Android
> SQLITE_TEMP_STORE=2 on 64bit

Sounds good.  Since 3 is only an option at build-time I'm assuming we'll do this at build-time and we won't be too concerned about the --enable-system-sqlite case?  Or we can ifdef invoking a PRAGMA for 2 if system sqlite is in use if we think that set of users will be negatively impacted by the shipped default of 1.

For reference, this is what the debian build CFLAGS look like from the debian.tar.xz's "rules" file at https://packages.debian.org/jessie/libsqlite3-0:

export CFLAGS += -O2 -fno-strict-aliasing \
	-DSQLITE_SECURE_DELETE -DSQLITE_ENABLE_COLUMN_METADATA \
	-DSQLITE_ENABLE_FTS3 -DSQLITE_ENABLE_RTREE=1 -DSQLITE_SOUNDEX=1 \
	-DSQLITE_ENABLE_UNLOCK_NOTIFY \
	-DSQLITE_OMIT_LOOKASIDE=1 \
	-DSQLITE_ENABLE_UPDATE_DELETE_LIMIT=1 \
	-DSQLITE_MAX_SCHEMA_RETRY=25 \
	-DSQLITE_MAX_VARIABLE_NUMBER=250000

(Note that there's also a page size patch for 32k in the patch queue.)

Comment 8

[Marco Bonardo [:mak]]

I think we should use the compile_time option and on system sqlite we can also use the pragma. While we could always use the pragma, it looks like a pointless overhead to run a query just for this.

Comment 9

[Marco Bonardo [:mak]]

Attachment: Bug 1313021 - Use a MEMORY SQLITE_TEMP_STORE on Android and 64bit builds.

Review commit: https://reviewboard.mozilla.org/r/90654/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/90654/

Comment 10

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8807516 [details]
Bug 1313021 - Use a MEMORY SQLITE_TEMP_STORE on Android and 64bit builds.

https://reviewboard.mozilla.org/r/90654/#review90360

PRAGMA lgtm=1;

::: storage/mozStorageConnection.cpp:766
(Diff revision 1)
>      mDBConn = nullptr;
>      return convertResultCode(srv);
>    }
>  
> +#if defined(MOZ_MEMORY_TEMP_STORE_PRAGMA)
> +  (void)ExecuteSimpleSQL(NS_LITERAL_CSTRING("PRAGMA TEMP_STORE = 2;"));

nit: All the other uses of PRAGMA in this file use the lower-case pragma-name, including the use of "temp_store" for clone propagation.

Comment 11

[Marco Bonardo [:mak]]

Comment on attachment 8807516 [details]
Bug 1313021 - Use a MEMORY SQLITE_TEMP_STORE on Android and 64bit builds.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/90654/diff/1-2/

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/600464273237
Use a MEMORY SQLITE_TEMP_STORE on Android and 64bit builds. r=asuth

Comment 13

[Phil Ringnalda (:philor)]

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