658303 - mozIStorageConnection::Clone() should copy over #pragmas

Status: RESOLVED FIXED

(Core :: SQLite and Embedded Database Bindings, defect)

Description

[Alon Zakai (:azakai)]

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.

Comment 1

[Marco Bonardo [:mak]]

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.

Comment 2

[Marco Bonardo [:mak]]

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)

Comment 3

[Shawn Wilsher :sdwilsh]

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

Comment 4

[Marco Bonardo [:mak]]

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?

Comment 5

[Shawn Wilsher :sdwilsh]

(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.

Comment 6

[Marco Bonardo [:mak]]

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.

Comment 7

[Marco Bonardo [:mak]]

Attachment: wip 1.0

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

Comment 8

[Marco Bonardo [:mak]]

Attachment: patch v1.0

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 9

[Shawn Wilsher :sdwilsh]

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?

Comment 10

[Marco Bonardo [:mak]]

(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.

Comment 11

[Marco Bonardo [:mak]]

Attachment: patch v1.1

Address comments.

Comment 12

[Marco Bonardo [:mak]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ba2483679e59

Comment 13

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/ba2483679e59

Comment 14

[Matt Brubeck (:mbrubeck)]

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

Comment 15

[Marco Bonardo [:mak]]

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.

Comment 16

[Marco Bonardo [:mak]]

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.

Comment 17

[Marco Bonardo [:mak]]

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.

Comment 18

[Marco Bonardo [:mak]]

Attachment: patch v1.2

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.

Comment 19

[Marco Bonardo [:mak]]

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

I meant bug 689894

Comment 20

[Marco Bonardo [:mak]]

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.

Comment 21

[Marco Bonardo [:mak]]

Attachment: patch v1.3

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

Comment 22

[Shawn Wilsher :sdwilsh]

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.

Comment 23

[Marco Bonardo [:mak]]

Attachment: patch v1.4

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

Comment 24

[Marco Bonardo [:mak]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8b6d54721fa1

Comment 25

[Marco Bonardo [:mak]]

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).

Comment 26

[Marco Bonardo [:mak]]

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.

Comment 27

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8b6d54721fa1