The default bug view has changed. See this FAQ.

mozIStorageConnection::Clone() should copy over #pragmas

RESOLVED FIXED in mozilla10

Status

()

Toolkit
Storage
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: azakai, Assigned: mak)

Tracking

unspecified
mozilla10
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink])

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

6 years ago
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.
(Reporter)

Updated

6 years ago
Blocks: 625981
tracking-fennec: --- → ?
(Assignee)

Comment 1

6 years ago
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.
(Assignee)

Comment 2

6 years ago
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)
Component: Places → Storage
QA Contact: places → storage
We should do this for a set of things we care about.  What that set is, I don't know offhand.
(Assignee)

Comment 4

6 years ago
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.
tracking-fennec: ? → 7+
tracking-fennec: 7+ → ---
(Assignee)

Updated

6 years ago
Assignee: nobody → mak77
(Assignee)

Comment 6

6 years ago
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.
(Assignee)

Comment 7

6 years ago
Created attachment 557491 [details] [diff] [review]
wip 1.0

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

Comment 8

6 years ago
Created attachment 557524 [details] [diff] [review]
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.
Attachment #557491 - Attachment is obsolete: true
Attachment #557524 - Flags: review?(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?
Attachment #557524 - Flags: review?(sdwilsh) → review+
(Assignee)

Comment 10

6 years ago
(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.
(Assignee)

Comment 11

6 years ago
Created attachment 564293 [details] [diff] [review]
patch v1.1

Address comments.
Attachment #557524 - Attachment is obsolete: true
(Assignee)

Comment 12

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba2483679e59
Target Milestone: --- → mozilla10
https://hg.mozilla.org/mozilla-central/rev/ba2483679e59
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla10 → ---
(Assignee)

Comment 15

6 years ago
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.
(Assignee)

Comment 16

6 years ago
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.
(Assignee)

Comment 17

6 years ago
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.
(Assignee)

Updated

6 years ago
Depends on: 692119
(Assignee)

Updated

6 years ago
Depends on: 692120
(Assignee)

Comment 18

6 years ago
Created attachment 564900 [details] [diff] [review]
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.
(Assignee)

Comment 19

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

I meant bug 689894
(Assignee)

Updated

6 years ago
Attachment #564293 - Attachment is obsolete: true
(Assignee)

Comment 20

6 years ago
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.
(Assignee)

Comment 21

6 years ago
Created attachment 570256 [details] [diff] [review]
patch v1.3

Minor update using bool and ArrayLength, per new platform conventions.
All dependencies are fixed, so this would be ready to land.
Attachment #564900 - Attachment is obsolete: true
Attachment #570256 - Flags: review?(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.
Attachment #570256 - Flags: review?(sdwilsh) → review+
(Assignee)

Comment 23

5 years ago
Created attachment 572435 [details] [diff] [review]
patch v1.4

yep, looks like trailing commas are allowed and ArrayLength behaves correctly.
Attachment #570256 - Attachment is obsolete: true
(Assignee)

Comment 24

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b6d54721fa1
Flags: in-testsuite+
(Assignee)

Comment 25

5 years ago
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).
Whiteboard: [MemShrink]
(Assignee)

Updated

5 years ago
Depends on: 700417
(Assignee)

Comment 26

5 years ago
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.
https://hg.mozilla.org/mozilla-central/rev/8b6d54721fa1
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
You need to log in before you can comment on or make changes to this bug.