Closed Bug 1313021 Opened 8 years ago Closed 8 years ago

Use a MEMORY SQLITE_TEMP_STORE on Android and 64bit builds

Categories

(Toolkit :: Storage, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: mak, Assigned: mak)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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)
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)
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.
do you know what file extension SQLite uses for these temp stores ?
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)
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.)
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: nobody → mak77
Summary: evaluate making SQLITE_TEMP_STORE default to MEMORY on desktop platforms → Use a MEORY SQLITE_TEMP_STORE on Android and 64bit builds
Summary: Use a MEORY SQLITE_TEMP_STORE on Android and 64bit builds → Use a MEMORY SQLITE_TEMP_STORE on Android and 64bit builds
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+
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
https://hg.mozilla.org/mozilla-central/rev/600464273237
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.