703113 - Fix overshadowed |pageSize| variable in Connection::initialize

Status: RESOLVED FIXED

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

Description

[Nicholas Nethercote [inactive]]

Attachment: patch

GCC tells me:

  mozStorageConnection.cpp:564:15: warning: unused variable ‘pageSize’ [-Wunused-variable]

There's a shadowed variable declaration.  The attached patch fixes it.  Marco, can you explain the actual effect of this fix?  I don't understand all the cacheSize mumbo-jumbo.

Comment 1

[Marco Bonardo [:mak]]

Comment on attachment 575010 [details] [diff] [review]
patch

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

well, I'm not an official peer but I wrote this code and it's a really trivial fix, so I'll review it.

The effect of this unwanted shadowing, is that for existing databases using a different page_size than the default we may calculate a smaller cache size.

So for example for a db with the default page_size of 32768, the cache_size will be set to 128 pages (4iMB limit)
for a db with a page_size of 4096 the cache_size will still be set to 128 pages, putting the memory limit to 512KB, it should instead be set to 1024 pages (4MiB limit)

We should backport this simple fix to Aurora.

Comment 2

[Marco Bonardo [:mak]]

actually, could you make a really simple test?
it should just be a part of http://mxr.mozilla.org/mozilla-central/source/storage/test/unit/test_page_size_is_32k.js you should just add a 4096 page_sized database and check the cache_size

Comment 3

[Marco Bonardo [:mak]]

or you may split a new test file and use that one as example, since its name is a bit misleading...

Comment 4

[Shawn Wilsher :sdwilsh]

Marco's review is fine for this bug once you get a test. :)

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: patch, v2, test doesn't work

This patch:

- Adds a test.  But it doesn't work.  I change the page size with "PRAGMA
  page_size =" and that works, but this apparently happens after
  Connection::initialize() runs and so the cache_size isn't updated and it's
  not actually testing the change.  I can't work out how to create a new DB
  with a particular page size, rather than changing the page size after
  creation.  Suggestions are welcome!

- I changed DEFAULT_CACHE_SIZE_PAGES from 2000 to 2048 -- 2000 seems like it
  might cause some wasted space.

- In test_page_size_is_32k.js the ".sqlite" suffix was being added twice to
  the DB filenames, I fixed this.
  
- In test_vacuum.js I fixed a comment in a typo.

Comment 6

[Nicholas Nethercote [inactive]]

Attachment: patch v3

This time the test works -- Marco suggested I close then re-open the DB.  I also reverted the change to DEFAULT_CACHE_SIZE_PAGES on Marco's request.

Comment 7

[Marco Bonardo [:mak]]

Comment on attachment 576022 [details] [diff] [review]
patch v3

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

::: storage/test/unit/test_cache_size.js
@@ +1,3 @@
> +/* Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/
> + */

This is the wrong boilerplate, the correct one is here:
http://www.mozilla.org/MPL/boilerplate-1.1/pd-c

@@ +6,5 @@
> +// size (bug 703113).
> +
> +// In order to change the cache size, we must open a DB, change the page
> +// size, create a table, close the DB, then re-open the DB.  We then check
> +// the cache size after reopening.

nit: this may be a proper javadoc

@@ +15,5 @@
> +  db.executeSimpleSQL("PRAGMA page_size = " + pageSize);
> +
> +  // Check the page size change worked.
> +  var stmt = db.createStatement("PRAGMA page_size");
> +  stmt.executeStep();

do_check_true this

@@ +16,5 @@
> +
> +  // Check the page size change worked.
> +  var stmt = db.createStatement("PRAGMA page_size");
> +  stmt.executeStep();
> +  do_check_eq(stmt.getInt32(0), pageSize);

you can use stmt.row.page_size instead of stmt.getInt32(0)

@@ +28,5 @@
> +  db = dbOpener(file);
> +
> +  // Check cache size is as expected.
> +  var stmt = db.createStatement("PRAGMA cache_size");
> +  stmt.executeStep();

ditto on do_check_true

@@ +29,5 @@
> +
> +  // Check cache size is as expected.
> +  var stmt = db.createStatement("PRAGMA cache_size");
> +  stmt.executeStep();
> +  do_check_eq(stmt.getInt32(0), expectedCacheSize);

ditto on stmt.row.cache_size

@@ +45,5 @@
> +function run_test()
> +{
> +  // See Connection::initialize() for the logic dictating pageSize and
> +  // expectedCacheSize.
> +  var sizes = [

nit: use "let" in new tests

@@ +48,5 @@
> +  // expectedCacheSize.
> +  var sizes = [
> +    { pageSize:  1024, expectedCacheSize: 2000 },
> +    { pageSize:  4096, expectedCacheSize: 1024 },
> +    { pageSize: 32768, expectedCacheSize:  128 }

add a trailing comma, it is legal and helps preserving future blame

@@ +49,5 @@
> +  var sizes = [
> +    { pageSize:  1024, expectedCacheSize: 2000 },
> +    { pageSize:  4096, expectedCacheSize: 1024 },
> +    { pageSize: 32768, expectedCacheSize:  128 }
> +  ];

nit: you may define an helper function as Math.min(DEFAULT_PAGE_SIZE, 4Mib/pageSize), so mimic the code logic. That way it may be simpler to update the test in future rather having to calculate multiple hardcoded values

Comment 8

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0be7cfaed624

Comment 9

[Nicholas Nethercote [inactive]]

Attachment: landed patch

Nominating for Aurora on Marco's suggestion.  It's a one-line patch (ignoring the added test) that fixes the SQL cache sizing.  It doesn't affect correctness, but will affect time/space usage in some cases.

Comment 10

[Marco Bonardo [:mak]]

the effect on our users is that some of them may have performance issues due to a poorly choosen (too small) cache, I think a one-line patch (with test) removing a wrongly shadowed variable is a good exchange for avoiding perf issues.

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/0be7cfaed624

Comment 12

[LegNeato]

Comment on attachment 576626 [details] [diff] [review]
landed patch

Sounds good to me! Please land on mozilla-aurora.

Comment 13

[Marco Bonardo [:mak]]

Comment on attachment 576626 [details] [diff] [review]
landed patch

ugh, my memory failed here, the regression was introduced with bug 692487, but that bug didn't make the Aurora merge, it was near it and I thought it was in Aurora, my fault, sorry for unneeded noise.