681525 - Provide ability to blow away sqlite caches based on inactivity

Status: RESOLVED INACTIVE

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

Description

[(dormant account)]

I think sqlite cache is helpful for general sqlite performance. However it might make sense to enable sqlite cache ondemand (or turn it off after a certain amount of db idle time)

Comment 1

[(dormant account)]

It might make sense to implement the sqlite cache using ashmem http://www.sqlite.org/c3ref/pcache_methods.html

Comment 2

[Nathan Froyd [:froydnj]]

I will poke at this.  I am leery of the bit that suggests SQLite can use a single page cache for multiple databases; seems like that would make it hard to drop things on the floor.

Comment 3

[(dormant account)]

(In reply to Nathan Froyd from comment #2)
> I will poke at this.  I am leery of the bit that suggests SQLite can use a
> single page cache for multiple databases; seems like that would make it hard
> to drop things on the floor.

It does support returning NULLs, so as long as there aren't any threading issues,it *should* work

Comment 4

[Nathan Froyd [:froydnj]]

Do we even know that the cache is helping us?  Is it worth having a TelemetryPageCache for measuring how much we hit the databases in the first place?  We can't measuring cache hits precisely without modifying sqlite itself, but maybe we can get useful numbers.

I can imagine, for instance, that caching stuff that was in flash on a mobile device might not be worthwhile?  Of course, since we're using "I think" and "I imagine", we don't have any idea what's really going on. :)

Comment 5

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

(In reply to Nathan Froyd from comment #4)
> Do we even know that the cache is helping us?

In the pre-WAL world, I believe it was very important to have a page cache large enough to hold all the dirty pages in a transaction to avoid having to spill them to disk earlier than required and perform additional fsyncs required for coherency (assuming fsyncs were enabled).  In the Write-Ahead-Log world, I would not be surprised if this is less important.

I do agree that having actual numbers to back things up would be great.  I did some work a while back using systemtap to attribute page fetch costs (and so ignoring write costs) to query opcodes that could be useful, but is probably overkill, that might be of interest:
http://www.visophyte.org/blog/?p=485

Comment 6

[Nathan Froyd [:froydnj]]

(In reply to Taras Glek (:taras) from comment #3)
> (In reply to Nathan Froyd from comment #2)
> > I will poke at this.  I am leery of the bit that suggests SQLite can use a
> > single page cache for multiple databases; seems like that would make it hard
> > to drop things on the floor.
> 
> It does support returning NULLs, so as long as there aren't any threading
> issues,it *should* work

Fetching a page from the cache "pins" the page until it is unpinned; I don't think the kernel yanking the cache out from underneath is going to go well.  And then if you wait until there are no pinned pages, you might as well not use ashmem.

I see, however, that ashmem supports pinning individual memory pages in the ashmem region...ugh.  We could use that, but only if page sizes match between the kernel and the page cache, and we'd have extra syscall overhead on page fetches and unpins.

Hopefully Andi Kleen's promised mmap flag will have similar page pin/unpin functionality?

This bug feels like a rathole.

Comment 7

[(dormant account)]

Nathan, as we found out one has to explicitly unpin ashmem. That matches sqlite expectations.

Comment 8

[Justin Lebar (not reading bugmail)]

Can we just close and re-open all the database connections?  Shouldn't that completely shut down sqlite and clear its caches?


Something of this form is important for mobile.  When Firefox goes into the background, we need to release as much memory as humanly possible so that we're less likely to get kicked out of memory altogether and shut down.

Comment 9

[(dormant account)]

(In reply to Justin Lebar [:jlebar] from comment #8)
> Can we just close and re-open all the database connections?  Shouldn't that
> completely shut down sqlite and clear its caches?
> 
> 
> Something of this form is important for mobile.  When Firefox goes into the
> background, we need to release as much memory as humanly possible so that
> we're less likely to get kicked out of memory altogether and shut down.

I think that's a great idea. We'd still have issues with re-preparing statements, etc.

Comment 10

[Shawn Wilsher :sdwilsh]

(In reply to Justin Lebar [:jlebar] from comment #8)
> Can we just close and re-open all the database connections?  Shouldn't that
> completely shut down sqlite and clear its caches?
We cannot do that in the general case because consumers may be using temporary tables or triggers.

Comment 11

[Justin Lebar (not reading bugmail)]

It sounds like we need to ensure that we do a clean shutdown of the consumers.  That way, they'll write their temporary data back to disk and wait for triggers to finish running.

Or is that not sufficient, for some reason?

Comment 12

[Shawn Wilsher :sdwilsh]

(In reply to Justin Lebar [:jlebar] from comment #11)
> It sounds like we need to ensure that we do a clean shutdown of the
> consumers.  That way, they'll write their temporary data back to disk and
> wait for triggers to finish running.
> 
> Or is that not sufficient, for some reason?
That would work, but at that point you could only do that if you shutdown xpcom since services are cached by the service manager.  It's doable, but risky.

Comment 13

[Jesse Ruderman]

Observing memory-pressure is covered by bug 411894.

"Based on inactivity" might not be the right heuristic, since Places is active on every page load. Maybe just "periodically"? Or make the cache itself be time-based?

Comment 14

[Marco Bonardo [:mak]]

Just FYI, from tomorrow's nightly, places cache will be limited to 4/8MB per connection, that considering the 3 connections we have (right now) is a memory usage between 12MB and 24MB (this is calculated from current db size). Inline autocomplete feature will likely add another connection.
Bug 692487 will limit any other connection (that is not setting its own limit) to 4MB.

Comment 15

[Marco Bonardo [:mak]]

Oh, I forgot, those are the upper limits.

Comment 16

[Nicholas Nethercote [inactive]]

(In reply to Marco Bonardo [:mak] from comment #14)
> Just FYI, from tomorrow's nightly, places cache will be limited to 4/8MB per
> connection, that considering the 3 connections we have (right now) is a
> memory usage between 12MB and 24MB (this is calculated from current db
> size). Inline autocomplete feature will likely add another connection.

Oh, we used to have four Places connections, when did one of them go away?

Comment 17

[Marco Bonardo [:mak]]

(In reply to Nicholas Nethercote [:njn] from comment #16)
> Oh, we used to have four Places connections, when did one of them go away?

I killed one in bug 695554, since I was able to delay bookmarks service initialization.

Comment 18

[Nicholas Nethercote [inactive]]

Downgrading to MemShrink:P3 because SQLite isn't used for storage on mobile any more.

Comment 19

[(dormant account)]

(In reply to Nicholas Nethercote [:njn] from comment #18)
> Downgrading to MemShrink:P3 because SQLite isn't used for storage on mobile
> any more.

sqlite is very much used on mobile. Places wrapper around sqlite was rewritten, but that's it.

Comment 20

[Richard Newman [:rnewman]]

(In reply to Taras Glek (:taras) from comment #19)
> (In reply to Nicholas Nethercote [:njn] from comment #18)
> > Downgrading to MemShrink:P3 because SQLite isn't used for storage on mobile
> > any more.
> 
> sqlite is very much used on mobile. Places wrapper around sqlite was
> rewritten, but that's it.

Mobile doesn't use Places, but it uses sqlite for all kinds of things that aren't Places.

And FHR uses sqlite extensively.

Back up to P2, therefore?

Comment 21

[Marco Bonardo [:mak]]

Fwiw, Sqlite.jsm will have these memory eviction features: bug 831578.

And yes, this is still useful as well as bug 708882.
And no, Places is not at all related to this (just as a reminder, Places is just a consumer of Storage, like any other component in our codebase).

Comment 22

[Gabriele Svelto [:gsvelto]]

Quick question: wasn't this functionality provided as part of bug 833609 (specifically attachment 706018 [details] [diff] [review])?

Comment 23

[Gregory Szorc [:gps]]

(In reply to Gabriele Svelto [:gsvelto] from comment #22)
> Quick question: wasn't this functionality provided as part of bug 833609
> (specifically attachment 706018 [details] [diff] [review])?

We have this for Sqlite.jsm, but not for low-level Storage. IMO, we should rewrite all JS in the tree using Storage XPCOM interfaces to use Sqlite.jsm. That leaves the surface area of this bug as C++.

Alternatively, we could port Sqlite.jsm's API to the XPCOM interface. The main reason I didn't do that in bug 833609 was because I didn't want to get dirty with storage's C++ :)

Comment 24

[Gabriele Svelto [:gsvelto]]

(In reply to Gregory Szorc [:gps] from comment #23)
> We have this for Sqlite.jsm, but not for low-level Storage. IMO, we should
> rewrite all JS in the tree using Storage XPCOM interfaces to use Sqlite.jsm.
> That leaves the surface area of this bug as C++.

So if I understand this correctly you mean that all JS code should use Sqlite.jsm and all C++ code should use mozIStorageService instead, correct? A quick grep shows these JS files using mozIStorageService directly:

toolkit/modules/Services.jsm
toolkit/components/passwordmgr/storage-mozStorage.js
toolkit/components/satchel/nsFormHistory.js
toolkit/components/contentprefs/nsContentPrefService.js
dom/ipc/preload.js

Plus a bunch of unit-tests.

> Alternatively, we could port Sqlite.jsm's API to the XPCOM interface. The
> main reason I didn't do that in bug 833609 was because I didn't want to get
> dirty with storage's C++ :)

I'm adding functionality to shrink a mozStorageConnection's memory in bug 411894 (plus an event handler in mozStorageService to respond to memory pressure events). I could re-use the functionality added to mozStorageConnection to implement this for both JS and C++ and then move the existing shrink-when-idle functionality from Sqlite.jsm to mozStorageConnection.(h|cpp). Does this sound reasonable?

Comment 25

[Gregory Szorc [:gps]]

I think JS should use Sqlite.jsm over the XPCOM interfaces because Sqlite.jsm is without a doubt much simpler to use and therefore less bug prone. There may be some perf considerations for wanting to use the raw XPCOM interfaces. This is my personal opinion and that topic is separate from this bug.

If Sqlite.jsm's functionality can be moved into Storage, I think that's best for all consumers. But this may be difficult to do completely. e.g. Sqlite.jsm abstracts the statement instances from callers and keeps track of whether they are active. It therefore can delete inactive statement handles at any time with no API implications for callers. "Deactivating" C++ statements could be dangerous and may entail API breakage. You can, however, issue PRAGMA's during memory pressure events from C++ just fine.

Comment 26

[Gabriele Svelto [:gsvelto]]

(In reply to Gregory Szorc [:gps] from comment #25)
> I think JS should use Sqlite.jsm over the XPCOM interfaces because
> Sqlite.jsm is without a doubt much simpler to use and therefore less bug
> prone. There may be some perf considerations for wanting to use the raw
> XPCOM interfaces. This is my personal opinion and that topic is separate
> from this bug.

OK, I wouldn't block on perf considerations either, especially before having proved that it's effectively slower. Sqlite.jsm looks slim enough and I imagine that most operations would spend enough time in sqlite to make the shim we use around it pale in comparison.

> If Sqlite.jsm's functionality can be moved into Storage, I think that's best
> for all consumers.

I meant only the shrink-on-idle functionality; not all of Sqlite.jsm but I realize I worded my previous comment poorly. Moving all Sqlite.jsm functionality in mozIStorageService is definitely out of scope for this bug :-)

Comment 27

[Gregory Szorc [:gps]]

(In reply to Gabriele Svelto [:gsvelto] from comment #26)
> > If Sqlite.jsm's functionality can be moved into Storage, I think that's best
> > for all consumers.
> 
> I meant only the shrink-on-idle functionality; not all of Sqlite.jsm but I
> realize I worded my previous comment poorly. Moving all Sqlite.jsm
> functionality in mozIStorageService is definitely out of scope for this bug
> :-)

And I meant only the memory shrinking functionality in that comment. I think we're both on the same page :)

Comment 28

[David Teller [:Yoric] - still alive but not very active]

(In reply to Gabriele Svelto [:gsvelto] from comment #24)
> (In reply to Gregory Szorc [:gps] from comment #23)
> > We have this for Sqlite.jsm, but not for low-level Storage. IMO, we should
> > rewrite all JS in the tree using Storage XPCOM interfaces to use Sqlite.jsm.
> > That leaves the surface area of this bug as C++.
> 
> So if I understand this correctly you mean that all JS code should use
> Sqlite.jsm and all C++ code should use mozIStorageService instead, correct?
> A quick grep shows these JS files using mozIStorageService directly:
> 
> toolkit/modules/Services.jsm
> toolkit/components/passwordmgr/storage-mozStorage.js
> toolkit/components/satchel/nsFormHistory.js
> toolkit/components/contentprefs/nsContentPrefService.js
> dom/ipc/preload.js
> 
> Plus a bunch of unit-tests.

Yes, all of this needs to be ported, just as plenty of C++ code needs to be ported to mozIStorageAsyncConnection instead of mozIStorageConnection.

Comment 29

[Marco Bonardo [:mak]]

you already got a good answer, fwiw I don't think we want an idl api to shrink memory, since it's a plain pragma to execute, what Sqlite.jsm does is fine, abstraction on top of a "simple" API.

Comment 30

[BMO Automation]

Per policy at https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Inactive_Bugs. If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.