Closed Bug 832067 Opened 9 years ago Closed 9 years ago

Shrink sqlite connection after use

Categories

(Firefox Health Report Graveyard :: Client: Desktop, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 21

People

(Reporter: gps, Assigned: rnewman)

References

Details

Attachments

(1 file, 1 obsolete file)

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?
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
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.
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.
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...
(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.
Depends on: 833609
> 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.
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!
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: nobody → rnewman
Status: NEW → ASSIGNED
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.
(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?
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.
(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?
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.
(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.
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.
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
Depends on: 834449
Builds on Bug 834449.
Attachment #706243 - Flags: review?(gps)
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)
Now cleans up after init, too.
Attachment #706243 - Attachment is obsolete: true
Attachment #706266 - Flags: review?(gps)
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+
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]
https://hg.mozilla.org/mozilla-central/rev/8efd1355f4ac
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [fixed in services]
Target Milestone: --- → mozilla21
Component: Metrics and Firefox Health Report → Client: Desktop
Product: Mozilla Services → Firefox Health Report
Target Milestone: mozilla21 → Firefox 21
Product: Firefox Health Report → Firefox Health Report Graveyard
You need to log in before you can comment on or make changes to this bug.