In bug 625981 comment 64 point 1 it was discovered that we set wal_autocheckpoint, but later we Clone() the db, and the clone does not have that #pragma enabled. We should apply the same pragmas as in the original we are copying.

I have no idea if this is something we could do in Storage, but I think that was the idea Shawn suggested, fix this in mozIStorageConnection. My only doubt is whether SQLite has a API to do that in an efficient way, since reading and cloning each single option without knowing if they changes sounds like an overkill.

I'm moving to Storage, if it should end up being inefficient or hard to do, we'll have to do it in callers (Places and so on)

We should do this for a set of things we care about. What that set is, I don't know offhand.

I'm scared by a couple things, for example, I don't want clones to get the same page cache size of the main connection. It's possible other consumers have different needs. Maybe we could make a clone-with-options api to accept a list of pragmas to copy over?

(In reply to comment #4) > Maybe we could make a clone-with-options api to accept a list of pragmas to > copy over? I think we can come up with a safe set of ones that you'd want to use for all databases pointing to the same file. I'm thinking journal_mode, and the various WAL settings would be a good start. I'd need to look through SQLite docs to figure out the rest.

I came up with this list: - cache_size, if smaller than default (to limit memory usage) - synchronous, if different than default (to avoid schema reparse) - foreign_keys, if different than default (to avoid schema reparse) - journal_size_limit - temp_store - wal_autocheckpoint it may not be optimal to get the default values though. Since Storage does not have access to those (defined in sqlite.c), we should copy over them, with risk that newer SQLite versions may change them. But if the schema reparse does not involve all the connections, but only the current one, it may be fine to always set the pragmas since no statements will have to be re-prepared. Will check that first.

needa a test, and I still have to verify the reparse issue.

Added a test. I've verified what happens regarding statements reparse. Setting foreign_keys to the same value as default expires all statements on the current connection, while setting it to a different value expires all statements across all connections. This is fine for our use case, since we need the same value across the clones and we don't yet have prepared statements on the cloned connection. Other pragmas don't cause reparse. in the cache_size case I read current value (that is the default) and compare it with the new value, I enforce the new value only if it's smaller, to save memory. Actually, I think in a separate bug we should change SQLITE_DEFAULT_CACHE_SIZE since we increased SQLITE_DEFAULT_PAGE_SIZE the current default size of the cache is 64MB (2000 * 32768) per connection, rather than the default value that was 2MB (2000 * 1024). We should probably set SQLITE_DEFAULT_CACHE_SIZE to 1/4 than it is.

Comment on attachment 557524 [details] [diff] [review] patch v1.0 Review of attachment 557524 [details] [diff] [review]: ----------------------------------------------------------------- r=sdwilsh with comments addressed ::: storage/src/mozStorageConnection.cpp @@ +892,5 @@ > + for (PRUint32 i = 0; i < NS_ARRAY_LENGTH(pragmas); ++i) { > + nsCAutoString pragmaQuery("PRAGMA "); > + pragmaQuery.Append(pragmas[i]); > + nsCOMPtr<mozIStorageStatement> stmt; > + (void)CreateStatement(pragmaQuery, getter_AddRefs(stmt)); `MOZ_ASSERT` that this doesn't fail please @@ +893,5 @@ > + nsCAutoString pragmaQuery("PRAGMA "); > + pragmaQuery.Append(pragmas[i]); > + nsCOMPtr<mozIStorageStatement> stmt; > + (void)CreateStatement(pragmaQuery, getter_AddRefs(stmt)); > + PRBool hasResult = PR_FALSE; We need to use `bool` now, right? @@ +907,5 @@ > + } > + } > + pragmaQuery.AppendLiteral(" = "); > + pragmaQuery.AppendInt(value); > + (void)clone->ExecuteSimpleSQL(pragmaQuery); `MOZ_ASSERT` this as well please. ::: storage/test/unit/test_storage_connection.js @@ +482,4 @@ > run_next_test(); > } > > +function test_clone_copies_pragmas() Can you also test that other pragmas don't get copied please?

(In reply to Shawn Wilsher :sdwilsh from comment #9) > We need to use `bool` now, right? Yes and no, it's a complex situation since there is still possibility for the great PRBool thing to be backed out (so far there is one perf regression and one correctness regression to figure out), but at the same time we should not use PRBool in new code... I'll use bool but still use PR_TRUE/PR_FALSE to simplify life to an eventual backout, if there won't be a backout Ehsan already offered to do whole-tree conversion of PR_TRUE/PR_FALSE.

Address comments.

Backed out on inbound because of a 15% regression in Ts, Paint on Linux. From the graphs it looks like Linux and Linux64 are both affected, and the regression also shows up in the plain "Ts" benchmark. https://hg.mozilla.org/integration/mozilla-inbound/rev/ecdec04463f7

That may be due to cloning a connection in the startup path, I am aware of at least one case where we do that, that is bookmarks service init (Though I thought we didn't init bookmarks service anymore in common startups, I will check). We may also need to do that for bug 689894 (though I'd try to reuse the bookmarks service connection). I may try to execute the pragmas through the async API, but at least "foreign_keys" should be synchronous, since otherwise the connection won't manage those.

Unfortunately copying asynchronously is not feasible, since it requires both connections to execute an asyncClose(), that would make the API more complicated to use than it already is.

When browser starts it invokes registerOpenPage, that clones the connection for autocomplete. This is really not needed till the user does an autocomplete search, so we may rather cache open pages till the connection is up. Then the Star button inits bookmarks service, to observe changes and this makes another clone in bookmarks service. Finally isVisited clones a third time, I have no ideas on how to delay this though. So there are some things I can do here (I'll file separate bugs for each): - fix autocomplete, so that it caches open pages till the first query. - in case of a readonly clone, copy only some of the pragmas. Most of them are useful only for writes - check if I can further delay the star button update, ideally it may not observe bookmarks till the bookmarks service is up. Have to figure out if I can observe categories for this. - fix clone() users so they don't run useless pragmas now that we do for them.

This is the modified patch that copies only cache_size and temp_store on read-only clones. I don't think there is a way to speed this up further. Now working on the other improvements to remove clone() from the startup path (we'll probably have to take a small hit when I implement bug 582703, but then I may probably remove the bookmarks separate connection and the hit should be minimal). Before asking for review I'll measure the global effect of all the fixes on try.

(In reply to Marco Bonardo [:mak] from comment #18) > when I implement bug 582703, I meant bug 689894

With all the dependencies there is only one Clone() invocation left in the startup path, that is the isVisited one (down from 3!). Not sure we can do much about that, we should probably delay isVisited resolution (I think Stechz was already working on that), but being a read-only connection, the modified patch here should reduce the hit to the minimum needed. We may also gain something from the dependencies (waiting on try results). I mostly care the global impact (also after bug 689894) is equal or better than before, since miracles are hard nowadays. For sure this patch, replicating cache_size, should help reducing memory usage and some wrong usese of clone() among addons.

Minor update using bool and ArrayLength, per new platform conventions. All dependencies are fixed, so this would be ready to land.

Comment on attachment 570256 [details] [diff] [review] patch v1.3 Review of attachment 570256 [details] [diff] [review]: ----------------------------------------------------------------- r=sdwilsh ::: storage/public/mozIStorageConnection.idl @@ +107,5 @@ > + * @note The following pragmas are copied over to a read-only clone: > + * cache_size, temp_store. > + * The following pragmas are copied over to a writeable clone: > + * cache_size, temp_store, foreign_keys, journal_size_limit, > + * synchronous, wal_autocheckpoint. I think a list here would be easier to read: - cache_size - temp_store etc ::: storage/src/mozStorageConnection.cpp @@ +888,5 @@ > + , "temp_store" > + , "foreign_keys" > + , "journal_size_limit" > + , "synchronous" > + , "wal_autocheckpoint" I *think* trailing commas are OK there. If so, just do that since it looks cleaner.

yep, looks like trailing commas are allowed and ArrayLength behaves correctly.

Marking as MemShrink, since if this sticks, should slightly reduce places.sqlite page cache usage (on my 8GB system the reduction should be a minimum of 10MB).

Ans I didn't consider Places is special, as usual :(, so for some user this may actually mean to use more memory. I filed bug 700417 to track that issue, if I can't fix that in the FF10 timeframe I'll back this out of Aurora.