Closed Bug 681525 Opened 8 years ago Closed 2 years ago

Provide ability to blow away sqlite caches based on inactivity

Categories

(Toolkit :: Storage, defect)

defect
Not set

Tracking

()

RESOLVED INACTIVE

People

(Reporter: taras.mozilla, Unassigned)

Details

(Whiteboard: [MemShrink:P3])

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)
It might make sense to implement the sqlite cache using ashmem http://www.sqlite.org/c3ref/pcache_methods.html
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.
Assignee: nobody → nfroyd
(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
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. :)
(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
(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.
Nathan, as we found out one has to explicitly unpin ashmem. That matches sqlite expectations.
Whiteboard: [MemShrink]
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.
(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.
(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.
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?
(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.
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?
Summary: Provide ability to blow away sqlite caches based on inactivity or memory pressure → Provide ability to blow away sqlite caches based on inactivity
Whiteboard: [MemShrink] → [MemShrink:P2]
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.
Oh, I forgot, those are the upper limits.
(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?
(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.
Assignee: nfroyd → nobody
Downgrading to MemShrink:P3 because SQLite isn't used for storage on mobile any more.
Whiteboard: [MemShrink:P2] → [MemShrink:P3]
(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.
(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?
OS: Linux → All
Hardware: x86 → All
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).
Quick question: wasn't this functionality provided as part of bug 833609 (specifically attachment 706018 [details] [diff] [review])?
Flags: needinfo?(mak77)
(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++ :)
(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?
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.
(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 :-)
(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 :)
(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.
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.
Flags: needinfo?(mak77)
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.
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → INACTIVE
You need to log in before you can comment on or make changes to this bug.