Closed
Bug 703113
Opened 13 years ago
Closed 13 years ago
Fix overshadowed |pageSize| variable in Connection::initialize
Categories
(Toolkit :: Storage, defect)
Toolkit
Storage
Tracking
()
RESOLVED
FIXED
mozilla11
Tracking | Status | |
---|---|---|
firefox10 | --- | unaffected |
People
(Reporter: n.nethercote, Assigned: n.nethercote)
References
Details
(Keywords: regression)
Attachments
(1 file, 3 obsolete files)
4.90 KB,
patch
|
Details | Diff | Splinter Review |
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.
Attachment #575010 -
Flags: review?(mak77)
Comment 1•13 years ago
|
||
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.
Attachment #575010 -
Flags: review?(mak77) → review+
Comment 2•13 years ago
|
||
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
Updated•13 years ago
|
Flags: in-testsuite?
Comment 3•13 years ago
|
||
or you may split a new test file and use that one as example, since its name is a bit misleading...
Updated•13 years ago
|
tracking-firefox10:
--- → ?
Updated•13 years ago
|
Assignee: nobody → nnethercote
Comment 4•13 years ago
|
||
Marco's review is fine for this bug once you get a test. :)
Assignee | ||
Comment 5•13 years ago
|
||
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.
Attachment #575010 -
Attachment is obsolete: true
Attachment #575783 -
Flags: review?(mak77)
Assignee | ||
Comment 6•13 years ago
|
||
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.
Attachment #575783 -
Attachment is obsolete: true
Attachment #575783 -
Flags: review?(mak77)
Attachment #576022 -
Flags: review?(mak77)
Comment 7•13 years ago
|
||
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
Attachment #576022 -
Flags: review?(mak77) → review+
Assignee | ||
Comment 8•13 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/0be7cfaed624
Assignee | ||
Comment 9•13 years ago
|
||
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.
Attachment #576022 -
Attachment is obsolete: true
Attachment #576626 -
Flags: approval-mozilla-aurora?
Comment 10•13 years ago
|
||
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•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/0be7cfaed624
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
Comment 12•13 years ago
|
||
Comment on attachment 576626 [details] [diff] [review] landed patch Sounds good to me! Please land on mozilla-aurora.
Attachment #576626 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
status-firefox10:
--- → affected
Comment 13•13 years ago
|
||
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.
Attachment #576626 -
Flags: approval-mozilla-aurora+
Updated•13 years ago
|
tracking-firefox10:
+ → ---
Flags: in-testsuite? → in-testsuite+
Updated•13 years ago
|
Blocks: 692487
Keywords: regression
You need to log in
before you can comment on or make changes to this bug.
Description
•