Closed Bug 692487 Opened 8 years ago Closed 8 years ago

Decrease Storage connections default cache_size

Categories

(Toolkit :: Storage, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla11

People

(Reporter: mak, Assigned: mak)

References

Details

(Whiteboard: [MemShrink:P2])

Attachments

(1 file, 3 obsolete files)

We increased SQLITE_DEFAULT_PAGE_SIZE from 1024 to 32768 but we didn't touch SQLITE DEFAULT CACHE SIZE.
As a result the maximum memory SQLite may use for caches has grown from the original 2MiB to 64MiB per connection. We should thus reduce SQLITE DEFAULT CACHE SIZE from 2000 to a more acceptable value.
Attached patch patch v1.0 (obsolete) — Splinter Review
So, the problem here is that we cannot just set the SQLite default value and sleep quietly, since it's likely possible SQLite ignores that value when opening existing databases. In such a case we'd end up limiting the cache size too much.
The only way I can see is to check what's the current page_size and, if the cache is going to take up larger space than we want, override cache_size. The drawback is that this requires 2 additional calls to ExecuteSimpleSQL() (only 1 for page_size <= 4096) on connection open, but may save some memory, especially with add-ons.
Thoughts?
Attachment #566067 - Flags: feedback?(sdwilsh)
FWIW, Try doesn't show any regression.
Currently I see this in about:memory:

│      ├──21,129,864 B (06.71%) -- places.sqlite
│      │  ├──20,736,912 B (06.58%) -- cache-used [4]
│      │  ├─────315,264 B (00.10%) -- stmt-used [4]
│      │  └──────77,688 B (00.02%) -- schema-used [4]

There are 4 places.sqlite connections, so it's an average of over 5MB per connection.  With this patch, we'd max out at 2MB per connection, reducing the size from 21MB to 8MB -- is that right?  If so, that sounds great!
Whiteboard: [MemShrink]
this wouldn't affect places.sqlite, this affects those databasa handlers that don't specify their own cache size. Places calculates its own cache size for performance reasons thus this does not affect it.
Whiteboard: [MemShrink] → [MemShrink:P2]
Attached patch patch v1.1 (obsolete) — Splinter Review
A minor update, I decided to go for 4MiB, that is small enough, but should not create problems with having too much fragmented pages in memory, compared to the default of 2MiB.

A defect of this idea is that, on connection creation, this runs at least 1 more pragma to get the page_size, and an eventual pragma to set cache_size.
But globally I expect SQLite to have the page_size value cached, and cache_size should be a volatile property, so should not matter.  Btw I'm going to do a try run with Talos, to check eventual misunderstanding.
Attachment #566067 - Attachment is obsolete: true
Attachment #566067 - Flags: feedback?(sdwilsh)
Attachment #570265 - Flags: review?(sdwilsh)
looks like a satchel test using a corrupt db (toolkit/components/satchel/test/unit/test_db_corrupt.js) hits the moz_assert failing to execute the PRAGMA cache_size, so I will have to make that a simple warning.
Comment on attachment 570265 [details] [diff] [review]
patch v1.1

clearing request, looks like there's something more.
Running the pragma changes the srv of the next "select * from sqlite_master" check to a SQLITE_IOERR, that we do not interpret as a NS_ERROR_FILE_CORRUPTED. That's interesting, I wonder if it's possible we could avoid the select by just running the pragma.
Attachment #570265 - Flags: review?(sdwilsh)
so yeah, the first failing query returns SQLITE_NOTADB, any next query returns SQLITE_IOERR, so replacing SELECT * FROM sqlite_master with PRAGMA cache_size = XYZ is feasible, and we get back the eventual overhead here.
Attached patch patch v1.2 (obsolete) — Splinter Review
Ok, this one should be better.

The idea is to use pragma to force the database open, instead of the existing sqlite_master query. This avoids the overhead of having an additional query.

So I set page_size, read the actually used value, then set cache_size to the minimum between the default value or the value calculated from the actual page_size.

I copied the corrupt database from satchel tests (wonder who wrote it, from its contents) since it adds an interesting corruption test with a file that is really not looking like a database, and added a test using it.
Attachment #570265 - Attachment is obsolete: true
Attachment #570334 - Flags: review?(sdwilsh)
Comment on attachment 570334 [details] [diff] [review]
patch v1.2

Review of attachment 570334 [details] [diff] [review]:
-----------------------------------------------------------------

We need to update the SQLite constant too so that NSS uses that new value in new profiles at least.

::: storage/src/mozStorageConnection.cpp
@@ +73,5 @@
>  #define MIN_AVAILABLE_BYTES_PER_CHUNKED_GROWTH 524288000 // 500 MiB
>  
> +// Maximum size of the pages cache per connection.  If the default cache_size
> +// value evaluates to a larger size, it will be reduced to save memory.
> +#define MAX_CACHE_SIZE_BYTES 4194304 // 4MiB

nit: space after 4 like above

@@ +581,5 @@
> +  nsCAutoString cacheSizeQuery("PRAGMA cache_size = ");
> +  cacheSizeQuery.AppendInt(NS_MIN(DEFAULT_CACHE_SIZE_PAGES,
> +                                  PRInt32(MAX_CACHE_SIZE_BYTES / pageSize)));
> +  rv = ExecuteSimpleSQL(cacheSizeQuery);
> +  NS_ENSURE_SUCCESS(rv, rv);

We are supposed to be returning SQLite codes here, not `nsresult`s.  I'd rather see us using raw SQLite functions here in `initialize` instead of methods on this class since we are not fully setup yet.  (In reality, you want to use `prepareStmt`).

@@ -590,5 @@
> -    ::sqlite3_close(mDBConn);
> -    mDBConn = nsnull;
> -
> -    return convertResultCode(srv);
> -  }

We don't close the pointer and null out `mDBConn` in the case where we fail to open now.

::: storage/test/unit/fakeDB.sqlite
@@ +1,1 @@
> +BACON

Signs point to dolske

::: storage/test/unit/test_page_size_is_32k.js
@@ +12,5 @@
>    do_check_eq(stmt.getInt32(0), expected_block_size);
>    stmt.finalize();
> +  stmt = db.createStatement("PRAGMA cache_size");
> +  stmt.executeStep();
> +  const expected_cache_size = 128; // (4MiB / 32KiB)

`const kExpectedCacheSize` (I should have caught the block size one in review, but clearly missed that.
Attachment #570334 - Flags: review?(sdwilsh) → review-
(In reply to Shawn Wilsher :sdwilsh from comment #10)
> Comment on attachment 570334 [details] [diff] [review] [diff] [details] [review]
> patch v1.2
> 
> Review of attachment 570334 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> We need to update the SQLite constant too so that NSS uses that new value in
> new profiles at least.

I'm not sure that's practical to do, if setting the page_size fails (for example if the db already exists) they may end up with a too small cache_size.
That's the original reason I didn't do that here but preferred to do calculation.

> @@ +581,5 @@
> > +  nsCAutoString cacheSizeQuery("PRAGMA cache_size = ");
> > +  cacheSizeQuery.AppendInt(NS_MIN(DEFAULT_CACHE_SIZE_PAGES,
> > +                                  PRInt32(MAX_CACHE_SIZE_BYTES / pageSize)));
> > +  rv = ExecuteSimpleSQL(cacheSizeQuery);
> > +  NS_ENSURE_SUCCESS(rv, rv);
> 
> We are supposed to be returning SQLite codes here, not `nsresult`s.

The method signature is
nsresult Connection::initialize(nsIFile *aDatabaseFile,
                                const char* aVFSName)

Indeed we always return convertResultCode(srv), so it's correct to return a nsresult. But I see your next comment is right about the fact a bailout here doesn't cleanup correctly, my bad.

I was trying to avoid the more expensive prepareStmt path, may I rather just use ::sqlite3_exec here?
Attached patch patch v1.3Splinter Review
this fixes comments, but the ones I'd like to get more details about:

- uses ::sqlite_exec instead of prepareStmt, I don't understand why we have to go the prepare/exec path here when direct exec should require less resources
- doesn't change the SQLite const, see comment 11 for the risk of touching that const

This is not urgent btw, while it may have an effect on used memory, that's likely just going to affect misbehaving add-ons.
Attachment #570334 - Attachment is obsolete: true
Attachment #572497 - Flags: review?(sdwilsh)
(In reply to Marco Bonardo [:mak] from comment #11)
> I'm not sure that's practical to do, if setting the page_size fails (for
> example if the db already exists) they may end up with a too small
> cache_size.
> That's the original reason I didn't do that here but preferred to do
> calculation.
Forgot about that.  Let's leave it as-is.

> I was trying to avoid the more expensive prepareStmt path, may I rather just
> use ::sqlite3_exec here?
Whoops, yeah.  Forgot that we use `sqlite3_exec` instead of preparing and then executing.
Comment on attachment 572497 [details] [diff] [review]
patch v1.3

Review of attachment 572497 [details] [diff] [review]:
-----------------------------------------------------------------

r=sdwilsh
Attachment #572497 - Flags: review?(sdwilsh) → review+
Summary: Decrease SQLITE DEFAULT CACHE SIZE → Decrease Storage connections default cache_size
https://hg.mozilla.org/integration/mozilla-inbound/rev/81d25ca914ec
Flags: in-testsuite+
Target Milestone: --- → mozilla11
https://hg.mozilla.org/mozilla-central/rev/81d25ca914ec
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Depends on: 703113
You need to log in before you can comment on or make changes to this bug.