Don't use 32k page_size for databases on B2G

RESOLVED FIXED in Firefox 23

Status

()

Core
DOM: IndexedDB
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Ben Turner (not reading bugmail, use the needinfo flag!), Assigned: Ben Turner (not reading bugmail, use the needinfo flag!))

Tracking

unspecified
mozilla23
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking-b2g:tef+, firefox21 wontfix, firefox22 wontfix, firefox23 fixed, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 fixed)

Details

Attachments

(3 attachments, 5 obsolete attachments)

66.30 KB, application/pdf
Details
16.51 KB, patch
mak
: review+
Ben Turner (not reading bugmail, use the needinfo flag!)
: superreview+
Details | Diff | Splinter Review
16.75 KB, patch
Ben Turner (not reading bugmail, use the needinfo flag!)
: review+
Details | Diff | Splinter Review
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.
Preliminary numbers look like 2k is better.
Created attachment 733045 [details] [diff] [review]
1. Patch for making page size pref-able
Assignee: nobody → bent.mozilla
Status: NEW → ASSIGNED
Attachment #733045 - Flags: review?(mak77)
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.
Ben, what are the expected performance win here? Additionally, what is the risk of this patch?
Flags: needinfo?(bent.mozilla)
Created attachment 733140 [details]
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.
Flags: needinfo?(bent.mozilla)
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).
Attachment #733045 - Flags: review?(mak77) → feedback+
Created attachment 733437 [details] [diff] [review]
Patch for mozilla-central
Attachment #733045 - Attachment is obsolete: true
Attachment #733437 - Flags: review?(mak77)
Created attachment 733517 [details] [diff] [review]
Patch for mozilla-b2g18
Attachment #733517 - Flags: review?(mak77)
Attachment #733437 - Attachment description: Trunk patch → Patch for mozilla-central
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 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.
Attachment #733517 - Flags: review?(mak77) → review+
(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 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.
Attachment #733437 - Flags: review?(mak77) → review+
Comment on attachment 733437 [details] [diff] [review]
Patch for mozilla-central

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

This has sr=Mossop over irc.
Attachment #733437 - Flags: superreview+

Updated

5 years ago
blocking-b2g: tef? → tef+
status-b2g18: --- → affected
status-b2g18-v1.0.1: --- → affected
Blocks: 858693
https://hg.mozilla.org/mozilla-central/rev/d4130427c313
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
https://hg.mozilla.org/releases/mozilla-b2g18/rev/aa6f30aa779c
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/3ee00ea7014f
status-b2g18: affected → fixed
status-b2g18-v1.0.0: --- → wontfix
status-b2g18-v1.0.1: affected → fixed
status-firefox21: --- → wontfix
status-firefox22: --- → wontfix
status-firefox23: --- → fixed
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
status-b2g18: fixed → affected
status-b2g18-v1.0.1: fixed → affected
Created attachment 734968 [details] [diff] [review]
Patch for mozilla-b2g18

Updated with test fix.
Attachment #733517 - Attachment is obsolete: true
Attachment #734968 - Flags: review+
Keywords: checkin-needed
Created attachment 734971 [details] [diff] [review]
Patch for mozilla-b2g18

Oops, now with extra test fix.
Attachment #734968 - Attachment is obsolete: true
Attachment #734971 - Flags: review+
Created attachment 734976 [details] [diff] [review]
Patch for mozilla-b2g18

Ugh, git and I do not get along. This should have the removed file as well.
Attachment #734971 - Attachment is obsolete: true
Attachment #734976 - Flags: review+
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
Keywords: checkin-needed
Created attachment 736130 [details] [diff] [review]
Patch for mozilla-b2g18

Ok, this patch reworks test_cookies_async_failure.js to work with the new page size.
Attachment #734976 - Attachment is obsolete: true
Attachment #736130 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/releases/mozilla-b2g18/rev/7d1f5875b8af
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/f83d169babee
status-b2g18: affected → fixed
status-b2g18-v1.0.1: affected → fixed
Keywords: checkin-needed
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
You need to log in before you can comment on or make changes to this bug.