Closed
Bug 832067
Opened 12 years ago
Closed 12 years ago
Shrink sqlite connection after use
Categories
(Firefox Health Report Graveyard :: Client: Desktop, defect)
Firefox Health Report Graveyard
Client: Desktop
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 21
People
(Reporter: gps, Assigned: rnewman)
References
Details
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
gps
:
review+
|
Details | Diff | Splinter Review |
I think there are a number of SQLite PRAGMA statements we could add to FHR's DB init that would result in less memory usage without sacrificing too much performance.
What these all are, I'm not really sure. cache_size seems interesting (https://www.sqlite.org/pragma.html#pragma_cache_size). I'm not sure what the default is. Perhaps it is too large?
Reporter | ||
Comment 1•12 years ago
|
||
Also investigate alternate journaling options.
Summary: Investigate SQLite PRAGMAs to make FHR database use less memory → Investigate SQLite PRAGMAs to make FHR database use less memory, I/O
Comment 2•12 years ago
|
||
First you should investigate why memory usage is high.
the cache size is set for all connections to 4MB (this is our default), though it's rarely filled up. if that happens it's a sign some query is not optimized and uses a scan table (that copies a bunch of data to the memory cache) instead of an index.
In some case the above is not-avoidable (for example autocomplete) but in most cases shouldn't happen.
Comment 3•12 years ago
|
||
I think I'd also be fine trying to reduce the default to 2-3MB provided talos doesn't complain (we could push both to try and compare numbers with a vanilla build). I initially reduced the cache from 64MB (yes!) to 4MB, but was planning to reduce it further in future when we had more async consumers.
Reporter | ||
Comment 4•12 years ago
|
||
I restarted my Nightly on OS X, let things run for a few mintues, and about:memory reports:
│ │ ├─────517,160 B (00.10%) -- healthreport.sqlite
│ │ │ ├──462,792 B (00.09%) ── cache-used
│ │ │ ├───45,680 B (00.01%) ── schema-used
│ │ │ └────8,688 B (00.00%) ── stmt-used
For comparison:
├───24,004,320 B (04.59%) -- storage
│ ├──22,492,640 B (04.30%) -- sqlite
│ │ ├──13,874,744 B (02.65%) -- places.sqlite
│ │ │ ├──13,534,752 B (02.59%) ── cache-used [4]
│ │ │ ├─────235,072 B (00.04%) ── stmt-used [4]
│ │ │ └─────104,920 B (00.02%) ── schema-used [4]
│ │ ├───3,347,904 B (00.64%) ── other
│ │ ├───1,331,488 B (00.25%) -- webappsstore.sqlite
│ │ │ ├──1,320,792 B (00.25%) ── cache-used
│ │ │ ├──────8,464 B (00.00%) ── stmt-used
│ │ │ └──────2,232 B (00.00%) ── schema-used
│ │ ├─────914,080 B (00.17%) -- cookies.sqlite
│ │ │ ├──891,792 B (00.17%) ── cache-used
│ │ │ ├───18,896 B (00.00%) ── stmt-used
│ │ │ └────3,392 B (00.00%) ── schema-used
│ │ ├─────621,168 B (00.12%) -- extensions.sqlite
│ │ │ ├──462,792 B (00.09%) ── cache-used
│ │ │ ├──145,984 B (00.03%) ── stmt-used
│ │ │ └───12,392 B (00.00%) ── schema-used
│ │ ├─────517,160 B (00.10%) -- healthreport.sqlite
│ │ │ ├──462,792 B (00.09%) ── cache-used
│ │ │ ├───45,680 B (00.01%) ── schema-used
│ │ │ └────8,688 B (00.00%) ── stmt-used
│ │ ├─────471,776 B (00.09%) -- addons.sqlite
│ │ │ ├──429,792 B (00.08%) ── cache-used
│ │ │ ├───30,592 B (00.01%) ── stmt-used
│ │ │ └───11,392 B (00.00%) ── schema-used
│ │ ├─────270,648 B (00.05%) -- formhistory.sqlite
│ │ │ ├──264,792 B (00.05%) ── cache-used
│ │ │ ├────3,312 B (00.00%) ── schema-used
│ │ │ └────2,544 B (00.00%) ── stmt-used
│ │ ├─────267,672 B (00.05%) -- signons.sqlite
│ │ │ ├──231,792 B (00.04%) ── cache-used
│ │ │ ├───30,416 B (00.01%) ── stmt-used
│ │ │ └────5,464 B (00.00%) ── schema-used
│ │ ├─────226,832 B (00.04%) -- CertPatrol.sqlite
│ │ │ ├──165,792 B (00.03%) ── cache-used
│ │ │ ├───56,304 B (00.01%) ── stmt-used
│ │ │ └────4,736 B (00.00%) ── schema-used
│ │ ├─────213,152 B (00.04%) -- content-prefs.sqlite
│ │ │ ├──198,792 B (00.04%) ── cache-used
│ │ │ ├───10,304 B (00.00%) ── stmt-used
│ │ │ └────4,056 B (00.00%) ── schema-used
│ │ ├─────144,832 B (00.03%) -- :memory:
│ │ │ ├──132,792 B (00.03%) ── cache-used
│ │ │ ├────7,696 B (00.00%) ── stmt-used
│ │ │ └────4,344 B (00.00%) ── schema-used
│ │ ├─────111,824 B (00.02%) -- downloads.sqlite
│ │ │ ├───99,792 B (00.02%) ── cache-used
│ │ │ ├────7,696 B (00.00%) ── stmt-used
│ │ │ └────4,336 B (00.00%) ── schema-used
│ │ ├─────109,904 B (00.02%) -- permissions.sqlite
│ │ │ ├───99,792 B (00.02%) ── cache-used
│ │ │ ├────7,984 B (00.00%) ── stmt-used
│ │ │ └────2,128 B (00.00%) ── schema-used
│ │ └──────69,456 B (00.01%) -- heatmap17.sqlite
│ │ ├──66,792 B (00.01%) ── cache-used
│ │ ├───2,664 B (00.00%) ── schema-used
│ │ └───────0 B (00.00%) ── stmt-used
Generally speaking, we don't care about performance except at startup or shutdown. I'm not sure if that means we could look into forcibly reducing the cache size. At the very least, I suspect if we periodically forced a cleanup or triggered cleanup after known high-use events (initial start, payload generation), we would be in a much happier place. I for one would love to see the cache-used figure drop below 250k if not 100k. Not sure if that is possible...
Comment 5•12 years ago
|
||
(In reply to Gregory Szorc [:gps] from comment #4)
> I restarted my Nightly on OS X, let things run for a few mintues, and
> about:memory reports:
>
> │ │ ├─────517,160 B (00.10%) -- healthreport.sqlite
> │ │ │ ├──462,792 B (00.09%) ── cache-used
> │ │ │ ├───45,680 B (00.01%) ── schema-used
> │ │ │ └────8,688 B (00.00%) ── stmt-used
that's not a lot globally, but indeed it's a lot for the kind of things this is supposed to retain. Maybe you have some queries not well optimized that is copying far more data than needed to memory.
> At the very least, I suspect if we periodically forced a
> cleanup or triggered cleanup after known high-use events (initial start,
> payload generation), we would be in a much happier place.
there is an important fact to remember about the cache, it always grows. that means if you run 100 queries, and just 1 of those requires 500KB of cache while the others could work with 10KB, the cache grows to 500KB and stays there, until you invoke shrink_memory pragma. This is a perf optimization of SQLite itself.
So, yes, periodic cleanup will keep that down, but you must really ensure you don't have some query reading all the table/database in one shot.
Assignee | ||
Comment 6•12 years ago
|
||
> So, yes, periodic cleanup will keep that down, but you must really ensure
> you don't have some query reading all the table/database in one shot.
Well, we do store and slurp large chunks: a JSON serialization of all active addons, for example. But we should track the queries we execute and the amount of cache they inflate. I'll take a look at that.
Still probably worth shrinking memory after serializing and after initing, seeing as these things happen infrequently.
Assignee | ||
Comment 7•12 years ago
|
||
I'd really like to be able to get to
::sqlite3_db_status
via mozStorageService/StorageSQLiteMultiReporter, but it's late, and I can't figure out if there's a way to get to that through JS.
On the plus side, I fixed some doc bugs!
Assignee | ||
Comment 8•12 years ago
|
||
If I extend storage with a pragma that drops all cached statements, then runs PRAGMA shrink_memory:
│ │ ├────770,000 B (00.51%) -- healthreport.sqlite
│ │ │ ├──693,792 B (00.46%) ── cache-used
│ │ │ ├───47,072 B (00.03%) ── schema-used
│ │ │ └───29,136 B (00.02%) ── stmt-used
drops to
│ │ └─────80,864 B (00.05%) -- healthreport.sqlite
│ │ ├──47,072 B (00.03%) ── schema-used
│ │ ├──33,792 B (00.02%) ── cache-used
│ │ └───────0 B (00.00%) ── stmt-used
So I'll explore that; the cost of keeping the connection open is very small once the cache goes away.
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → rnewman
Status: NEW → ASSIGNED
Reporter | ||
Comment 9•12 years ago
|
||
rnewman: I suggest you look at bug 833609. FHR's involvement thus becomes a call to shrinkMemory() after startup and after payload retrieval. Sqlite.jsm will handle the rest.
Assignee | ||
Comment 10•12 years ago
|
||
(In reply to Gregory Szorc [:gps] from comment #9)
> rnewman: I suggest you look at bug 833609. FHR's involvement thus becomes a
> call to shrinkMemory() after startup and after payload retrieval. Sqlite.jsm
> will handle the rest.
shrink_memory did nothing, as far as I could see, until I cleared _cachedStatements. Bug 833609 doesn't address that, right?
Reporter | ||
Comment 11•12 years ago
|
||
Bug 833609 does not yet address cached statements although the plan is to file a follow-up to do so.
From my personal measurements, a shink_memory immediately after startup reduced cache-used from >500k to <50k.
I've also never seen stmt-used over 100k for FHR. We should clear them out eventually, yes. But cache-used is the bigger fish.
Assignee | ||
Comment 12•12 years ago
|
||
(In reply to Gregory Szorc [:gps] from comment #11)
> I've also never seen stmt-used over 100k for FHR. We should clear them out
> eventually, yes. But cache-used is the bigger fish.
Are we sure that our cached statements don't ever affect the sqlite page cache?
Comment 13•12 years ago
|
||
shrink_memory acts only on the connection you call it, so if you opened another connection to the same db, like through sqliteManager, you didn't affect the original connection.
Assignee | ||
Comment 14•12 years ago
|
||
(In reply to Marco Bonardo [:mak] from comment #13)
> shrink_memory acts only on the connection you call it, so if you opened
> another connection to the same db, like through sqliteManager, you didn't
> affect the original connection.
It was the same connection; I just added code to finalize all the open statements prior to the call I'd put in to shrink_memory, and it started making a difference. Probably a timing issue if the statements weren't holding resources.
Comment 15•12 years ago
|
||
the statements and the page cache should be well separated areas... though srink_memory can only release memory it thinks is unused, I wonder if was just a bad timing as you said, like something was still using the cache.
Assignee | ||
Comment 16•12 years ago
|
||
With forthcoming patch, *after* generating JSON payload:
│ │ └─────84,432 B (00.08%) -- healthreport.sqlite
│ │ ├──50,640 B (00.05%) ── schema-used
│ │ ├──33,792 B (00.03%) ── cache-used
│ │ └───────0 B (00.00%) ── stmt-used
Reporter | ||
Comment 18•12 years ago
|
||
Comment on attachment 706243 [details] [diff] [review]
Clean up statements and shrink memory. v1
Review of attachment 706243 [details] [diff] [review]:
-----------------------------------------------------------------
I think we should also compact after startup has finished. We read a lot of data from the database that we cache in memory. I believe this cached data will also exist in SQLite's cache forever.
Cancelling because I suspect the Sqlite.jsm may change the API here (e.g. no more throwing).
::: services/metrics/storage.jsm
@@ +1221,5 @@
> + CommonUtils.exceptionStr(ex));
> + }
> +
> + // Try to shrink anyway.
> + return self._connection.shrinkMemory();
This will be a common pattern, I think. I predict it will eventually exist as an API in Sqlite.jsm. Possibly even as a connection-level option to control shrinkMemory's behavior.
Attachment #706243 -
Flags: review?(gps)
Assignee | ||
Comment 19•12 years ago
|
||
Now cleans up after init, too.
Attachment #706243 -
Attachment is obsolete: true
Attachment #706266 -
Flags: review?(gps)
Reporter | ||
Comment 20•12 years ago
|
||
Comment on attachment 706266 [details] [diff] [review]
Clean up statements and shrink memory. v2
Review of attachment 706266 [details] [diff] [review]:
-----------------------------------------------------------------
I love it when a plan comes together.
Attachment #706266 -
Flags: review?(gps) → review+
Assignee | ||
Comment 21•12 years ago
|
||
https://hg.mozilla.org/services/services-central/rev/8efd1355f4ac
We can file further follow-ups if there are other pragmas we wish to pursue.
Summary: Investigate SQLite PRAGMAs to make FHR database use less memory, I/O → Shrink sqlite connection after use
Whiteboard: [fixed in services]
Reporter | ||
Comment 22•12 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [fixed in services]
Target Milestone: --- → mozilla21
Updated•12 years ago
|
Component: Metrics and Firefox Health Report → Client: Desktop
Product: Mozilla Services → Firefox Health Report
Target Milestone: mozilla21 → Firefox 21
Updated•6 years ago
|
Product: Firefox Health Report → Firefox Health Report Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•