857376 - Don't use 32k page_size for databases on B2G

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, defect)

Description

[Ben Turner (not reading bugmail, use the needinfo flag!)]

The default page size is currently set at 32k unconditionally and it's way too big for databases on B2G. Profiling shows lots of time spent copying data to and from user space and kernel space.

Comment 1

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Preliminary numbers look like 2k is better.

Comment 2

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: 1. Patch for making page size pref-able

Comment 3

[Ben Turner (not reading bugmail, use the needinfo flag!)]

The alternative is to hard-code a number in to mozStorageConnection for gonk. 2k looks to be the right number based on my read/write tests.

Comment 4

[Daniel Coloma:dcoloma]

Ben, what are the expected performance win here? Additionally, what is the risk of this patch?

Comment 5

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Numbers

These are some of the numbers I collected on the Unagi while doing some pretty basic tests (each test either wrote or read 56b or 4kb 10000 times).

The numbers in the sheet are a representation of 7 different measurements (in milliseconds). The highest and lowest times were discarded and the remaining 5 were averaged. For the chart, write times (blue and red) are on the left and read times (yellow and green) are on the right.

The two conclusions I draw from this data are:

1. Read speeds are almost unaffected by changes in page size.
2. Write speeds are significantly slower at large and very small page sizes.

Our current page size (32k, the worst for write speeds!) was tuned based on desktop measurements and are pretty clearly not optimal on the Unagi.

If we fix this on b2g18 I expect some increase in write speeds. How this actually affects our built-in apps like Contacts or Email is a little unclear since the code for them is still kind of in flux.

There's also some risk that our final shipped device will be different in some fundamental way and the numbers will need to be tweaked again. We should make sure we test on one of those.

Comment 6

[Marco Bonardo [:mak]]

Comment on attachment 733045 [details] [diff] [review]
1. Patch for making page size pref-able

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

So, while I still have some doubts regarding the pref, in the end it is handy, also for this kind of measures, and the risks are limited since it's hidden and will likely only be set on mobile, where it's less likely users will touch it. There is a bit more overhead compared to the define case (where we'd just ifdef here http://mxr.mozilla.org/mozilla-central/source/db/sqlite3/src/Makefile.in#89) but it's once in app life and we are already accessing prefs.  Since we are not usually interested in micro-optimizations should not matter.

One of the problems to solve though, is that mozIStorageConnection::DEFAULT_PAGE_SIZE is used by the vacuum manager (its comsumers actually) to tell which page size is expected, since the page_size can only be changed through a vacuum and then this piece of code would still assume 32K when instead the expected value may be different.  It's not an issue right now since the only different consumer will be b2g that doesn't vacuum, though may become problematic in future, maybe we could add an expectedPageSize attribute to mozIStorageConnection and remove DEFAULT_PAGE_SIZE, it will return the right value based on the platform. Add-ons using DEFAULT_PAGE_SIZE will just return undefined, but should keep working, thanks to the fallback here: http://mxr.mozilla.org/mozilla-central/source/storage/src/VacuumManager.cpp#151 (should be updated to use the attribute clearly).

apart this thing that would be good to fix, the code looks fine (it's basically the same we have for the synchronous pref).

Comment 7

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch for mozilla-central

Comment 8

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch for mozilla-b2g18

Comment 9

[Ben Turner (not reading bugmail, use the needinfo flag!)]

The m-c patch is getting a little too big to justify landing on b2g18. I've attached a simple fix for b2g18 only, but I want to get this other in trunk still.

Comment 10

[Marco Bonardo [:mak]]

Comment on attachment 733517 [details] [diff] [review]
Patch for mozilla-b2g18

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

r=me for b2g18 branch

I'm not sure regarding comment 9, we can't take this patch on trunk since it's not ifdefed just for b2g, this patch is only good to build B2G, nothing else.
I suppose it meant we want a fix for page_size in both trunk and branch, just different fixes.

::: storage/public/mozIStorageConnection.idl
@@ +25,4 @@
>   *
>   * @threadsafe
>   */
> +[scriptable, uuid(eaca7250-2cbc-4a1f-ba12-2f30da42e4fc)]

there is no need to bump the uuid for a constant change, since it doesn't affect binary compatibility, in this case you also just change the value.

Comment 11

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to Marco Bonardo [:mak] from comment #10)
> I suppose it meant we want a fix for page_size in both trunk and branch,
> just different fixes.

Yep! Sorry that that was a little confusing.

> there is no need to bump the uuid for a constant change

Ok.

Comment 12

[Marco Bonardo [:mak]]

Comment on attachment 733437 [details] [diff] [review]
Patch for mozilla-central

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

this looks fine, still need a part to set the pref on b2g

::: db/sqlite3/src/Makefile.in
@@ +75,5 @@
>  # -DSQLITE_CORE=1 statically links that module into the SQLite library.
>  # -DSQLITE_DEFAULT_PAGE_SIZE=32768 and SQLITE_MAX_DEFAULT_PAGE_SIZE=32768
> +# increases the page size from 1k, see bug 416330.  It must be kept in sync with
> +# the value of PREF_TS_PAGESIZE_DEFAULT in mozStorageService.cpp.  The value can
> +# be overridden on a per-platform basis through the use of a hidden preference.

"through the use of the PREF_TS_PAGESIZE hidden preference."

::: storage/public/mozIStorageConnection.idl
@@ +84,5 @@
>    /**
> +   * The default size for SQLite database pages used by mozStorage for new
> +   * databases.
> +   */
> +  readonly attribute long defaultPageSize;

please get an SR on this, I suggest Mossop, off my head.

::: storage/src/mozStorageConnection.cpp
@@ +1047,5 @@
>  
>  NS_IMETHODIMP
> +Connection::GetDefaultPageSize(int32_t *_defailtPageSize)
> +{
> +  *_defailtPageSize = Service::getDefaultPageSize();

typo: defailt

::: storage/src/mozStorageService.cpp
@@ +42,5 @@
>  
> +#define PREF_TS_PAGESIZE "toolkit.storage.pageSize"
> +
> +// This value must be kept in sync with SQLITE_DEFAULT_PAGE_SIZE in the SQLite
> +// makefile!

better to also specify the path (db/sqlite3/src/Makefile.in)

::: storage/src/mozStorageService.h
@@ +70,5 @@
>  
>    /**
> +   * Obtains the default page size for this platform. The default value is
> +   * specified in the SQLite makefile (SQLITE_DEFAULT_PAGE_SIZE) but it may be
> +   * overriden with a hidden preference (toolkit.storage.pageSize).

with the PREF_TS_PAGESIZE hidden preference.

Comment 13

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 733437 [details] [diff] [review]
Patch for mozilla-central

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

This has sr=Mossop over irc.

Comment 14

[Ben Turner (not reading bugmail, use the needinfo flag!)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d4130427c313

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d4130427c313

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/aa6f30aa779c
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/3ee00ea7014f

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Backed out for xpcshell failures.
https://hg.mozilla.org/releases/mozilla-b2g18/rev/e7620c6e01d2
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/3b51ced33215

https://tbpl.mozilla.org/php/getParsedLog.php?id=21543488&tree=Mozilla-B2g18_v1_0_1

TEST-UNEXPECTED-FAIL | /Users/cltbld/talos-slave/test/build/xpcshell/tests/toolkit/components/places/tests/unit/test_telemetry.js | 2048 == 32768 - See following stack:
JS frame :: /Users/cltbld/talos-slave/test/build/xpcshell/head.js :: do_throw :: line 460
JS frame :: /Users/cltbld/talos-slave/test/build/xpcshell/head.js :: _do_check_eq :: line 554
JS frame :: /Users/cltbld/talos-slave/test/build/xpcshell/head.js :: do_check_eq :: line 575
JS frame :: /Users/cltbld/talos-slave/test/build/xpcshell/tests/toolkit/components/places/tests/unit/test_telemetry.js :: <TOP_LEVEL> :: line 19
JS frame :: /Users/cltbld/talos-slave/test/build/xpcshell/tests/toolkit/components/places/tests/unit/test_telemetry.js :: check_telemetry :: line 131
JS frame :: /Users/cltbld/talos-slave/test/build/xpcshell/tests/toolkit/components/places/tests/unit/test_telemetry.js :: maintenanceObserver :: line 122
JS frame :: resource://gre/modules/PlacesDBUtils.jsm :: PDBU__executeTasks :: line 71
JS frame :: resource://gre/modules/PlacesDBUtils.jsm :: PDBU__refreshUI :: line 141
JS frame :: resource://gre/modules/PlacesDBUtils.jsm :: PDBU__executeTasks :: line 53
JS frame :: resource://gre/modules/PlacesDBUtils.jsm :: <TOP_LEVEL> :: line 273
native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0

Comment 18

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch for mozilla-b2g18

Updated with test fix.

Comment 19

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch for mozilla-b2g18

Oops, now with extra test fix.

Comment 20

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch for mozilla-b2g18

Ugh, git and I do not get along. This should have the removed file as well.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Now this is causing xpcshell hangs in test_cookies_async_failure.js. Backed out again.

https://tbpl.mozilla.org/php/getParsedLog.php?id=21597353&tree=Mozilla-B2g18

Comment 22

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch for mozilla-b2g18

Ok, this patch reworks test_cookies_async_failure.js to work with the new page size.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/7d1f5875b8af
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/f83d169babee

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Disable a test that was failing intermittently on Linux opt builds that wasn't testing anything useful for b2g anyway.
https://hg.mozilla.org/releases/mozilla-b2g18/rev/51b02286773b
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/66fd11fee043