Use a MEMORY SQLITE_TEMP_STORE on Android and 64bit builds

RESOLVED FIXED in Firefox 52

Status

()

Toolkit
Storage
P3
normal
RESOLVED FIXED
8 months ago
6 months ago

People

(Reporter: mak, Assigned: mak)

Tracking

(Blocks: 1 bug)

Trunk
mozilla52
Points:
---

Firefox Tracking Flags

(firefox52 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

8 months ago
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?
Flags: needinfo?(jvarga)
Flags: needinfo?(bugmail)

Comment 1

8 months ago
I believe we don't track temp stuff by IDB or quota manager, so it should be ok from this point of view.
Flags: needinfo?(jvarga)
(Assignee)

Comment 2

8 months ago
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

8 months ago
do you know what file extension SQLite uses for these temp stores ?
(Assignee)

Comment 4

8 months ago
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.
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.
Flags: needinfo?(bugmail)
(Assignee)

Comment 6

8 months ago
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
(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.)
(Assignee)

Comment 8

8 months ago
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.
(Assignee)

Updated

8 months ago
Assignee: nobody → mak77
(Assignee)

Updated

8 months ago
Summary: evaluate making SQLITE_TEMP_STORE default to MEMORY on desktop platforms → Use a MEORY SQLITE_TEMP_STORE on Android and 64bit builds
(Assignee)

Updated

8 months ago
Summary: Use a MEORY SQLITE_TEMP_STORE on Android and 64bit builds → Use a MEMORY SQLITE_TEMP_STORE on Android and 64bit builds
Comment hidden (mozreview-request)

Comment 10

8 months ago
mozreview-review
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.
Attachment #8807516 - Flags: review?(bugmail) → review+
Comment hidden (mozreview-request)

Comment 12

8 months ago
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/600464273237
Use a MEMORY SQLITE_TEMP_STORE on Android and 64bit builds. r=asuth

Comment 13

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/600464273237
Status: NEW → RESOLVED
Last Resolved: 8 months ago
status-firefox52: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
(Assignee)

Updated

6 months ago
Duplicate of this bug: 1324541
You need to log in before you can comment on or make changes to this bug.