Closed Bug 836039 Opened 12 years ago Closed 12 years ago

Reduce cache size per connection to 2MB

Categories

(Toolkit :: Storage, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: mak, Assigned: mak)

Details

(Whiteboard: [MemShrink:P2])

Attachments

(1 file, 1 obsolete file)

We reduced the cache size from 64 MB to 4 MB some time ago, we choose this size compared to the default of 2MB to be on the safe side. Looking at everyday data on my profile, most of the sqlite consumers are far below this usage, the only ones that are not are: - places.sqlite: it has the awesomebar connection taking 6MB and other 3 connections taking about the same size (globally). While we measured time ago that reducing the awesomebar connection cache has a direct effect on its performances (so better to not reduce it further), the other 3 connections can be reduced without any problem. - webappsstore.sqlite: the problem here is that while some webpage may actually use the whole cache, most of them would not need it, but the memory always grows (until the cap) thuse in most cases this large cache is not unused. Would be good to try doing the change and measure it effects through telemetry and other available measures, it's even possible the new DOM Storage implementation improves the webappsstore.sqlite memory usage so that this measurement is not even more needed. We could also shrink the webappsstore memory when localStorage is unused from some time. Regardless, this would be a good memshrink improvement of 2-4MB, in some cases. This is the related code: http://mxr.mozilla.org/mozilla-central/source/storage/src/mozStorageConnection.cpp#48
s/is not unused/is unused/
Whiteboard: [memshrink] → [MemShrink:P2]
After recent DOM Storage changes, its cache size reduced quite a bit (was basically cut in half), so I think we can now proceed with this change. The outcome here is that every SQLite connection will have 2MB of maximum cache, unless they opt-in for a larger cache (the only connection that does that in Firefox is the one driving the awesomebar, that is limited to 6MB). We could gain some MB internally, but mostly this will reduce memory used by third-party (add-ons) connections.
Assignee: nobody → mak77
Status: NEW → ASSIGNED
doesn't need the even-newer dom storage rewrite anymore.
No longer depends on: 600307
I doubt IDB needed such a large cache, this will probably help our memory usage.
It's possible, the current value is 4MB per connection, fwiw.
Attached patch patch v1.0 (obsolete) (deleted) — Splinter Review
The patch uses the fact cache_size can now be set to a negative value, where this value is max numer of kibibytes for the cache. So now we don't need anymore to read the page_size before.
Attachment #715949 - Flags: review?(bugmail)
Comment on attachment 715949 [details] [diff] [review] patch v1.0 Making consumers need to explicitly request a larger cache size seems very reasonable.
Attachment #715949 - Flags: review?(bugmail) → review+
Attached patch patch v1.1 (deleted) — Splinter Review
Try remembered me I should also updated test_cache_size.js, the change is trivial though, so not going to reask for review.
Attachment #715949 - Attachment is obsolete: true
Flags: in-testsuite+
Target Milestone: --- → mozilla22
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment on attachment 716429 [details] [diff] [review] patch v1.1 NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings. [Approval Request Comment] Bug caused by (feature/regressing bug #): Not a regression User impact if declined: Higher than necessary memory use for all database connections (indexedDB is the big one here - each IDB transaction is a new connection). Testing completed: m-c Risk to taking this patch (and alternatives if risky): Maybe slightly slower database queries? Honestly shouldn't matter, the only connection using more than the new limit was places (which is not used on b2g). IDB connections never needed this extra memory so it's essentially just waste. String or UUID changes made by this patch: None
Attachment #716429 - Flags: approval-mozilla-b2g18?
Comment on attachment 716429 [details] [diff] [review] patch v1.1 Ok, I talked with the SQLite folks today and it looks like the cache is not preallocated, it's more of a "maximum size that the cache can grow" setting. Given that I don't think we'll gain much on b2g here, sorry.
Attachment #716429 - Flags: approval-mozilla-b2g18?
(In reply to ben turner [:bent] from comment #12) > Ok, I talked with the SQLite folks today and it looks like the cache is not > preallocated, it's more of a "maximum size that the cache can grow" setting. Yes, I was aware of that. The gain we have is when something executes a large transaction that causes the cache to grow, since it never shrinks (until you invoke pragma shrink_memory) it may become larger than what we need, thus the will to limit that maximum to a meaningful value. One day we'll also invoke shrink_memory automatically.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: