692487 - Decrease Storage connections default cache_size

Status: RESOLVED FIXED

(Toolkit :: Storage, defect)

Description

[Marco Bonardo [:mak]]

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.

Comment 1

[Marco Bonardo [:mak]]

Attachment: patch v1.0

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?

Comment 2

[Marco Bonardo [:mak]]

FWIW, Try doesn't show any regression.

Comment 3

[Nicholas Nethercote [inactive]]

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!

Comment 4

[Marco Bonardo [:mak]]

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.

Comment 5

[Marco Bonardo [:mak]]

Attachment: patch v1.1

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.

Comment 6

[Marco Bonardo [:mak]]

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 7

[Marco Bonardo [:mak]]

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.

Comment 8

[Marco Bonardo [:mak]]

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.

Comment 9

[Marco Bonardo [:mak]]

Attachment: patch v1.2

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.

Comment 10

[Shawn Wilsher :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.

Comment 11

[Marco Bonardo [:mak]]

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

Comment 12

[Marco Bonardo [:mak]]

Attachment: patch v1.3

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.

Comment 13

[Shawn Wilsher :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 14

[Shawn Wilsher :sdwilsh]

Comment on attachment 572497 [details] [diff] [review]
patch v1.3

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

r=sdwilsh

Comment 15

[Marco Bonardo [:mak]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/81d25ca914ec

Comment 16

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/81d25ca914ec