1110063 - Make mozSettings use readonly indexedDB transaction when possible

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[:gerard-majax]

When a lock only queues get requests, there is no need to open a readwrite transaction. This could help speedup things.

Comment 1

[:gerard-majax]

Attachment: Open readonly transaction for settings lock when possible r=qdot

Whenever a settings lock does only read operations (i.e. lock.get()),
there is no need that the underlying indexedDB transaction should be
opened as readwrite. Doing so, we will be able to parallelize some
request and avoid locking that should in the end allow for faster
operations.

Comment 2

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8534897 [details] [diff] [review]
Open readonly transaction for settings lock when possible r=qdot

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

r+ with expansion of comment.

::: dom/settings/SettingsRequestManager.jsm
@@ +119,5 @@
>      // Lets us know if this lock has been used to clear at any point.
>      hasCleared: false,
> +    // Whether we want to do a read only transaction. Define be default, and let
> +    // queueTask() set this to false if we queue any "set" task.
> +    forceReadOnly: true,

We should expand this to explain the assumption we're making. Something like:

forceReadOnly sets whether we want to do a read only transaction. Define true by default, and let queueTask() set this to false if we queue any "set" task. Since users of settings locks will queue all tasks before any idb transaction is created, we know we will have all needed information to set this before creating a transaction.

Comment 3

[:gerard-majax]

Attachment: Open readonly transaction for settings lock when possible r=qdot

Whenever a settings lock does only read operations (i.e. lock.get()),
there is no need that the underlying indexedDB transaction should be
opened as readwrite. Doing so, we will be able to parallelize some
request and avoid locking that should in the end allow for faster
operations.

Comment 4

[:gerard-majax]

Rebased on top of master, and updated the comment.

Comment 5

[:gerard-majax]

Try: https://tbpl.mozilla.org/?tree=Try&rev=5d8ea303ee5b&showall=1

Comment 6

[:gerard-majax]

Attachment: Open readonly transaction for settings lock when possible r=qdot

Whenever a settings lock does only read operations (i.e. lock.get()),
there is no need that the underlying indexedDB transaction should be
opened as readwrite. Doing so, we will be able to parallelize some
request and avoid locking that should in the end allow for faster
operations. Enabling this feature is controlled by the preference
dom.mozSettings.allowForceReadOnly, defaulting to false for now.

Comment 7

[:gerard-majax]

Comment on attachment 8537740 [details] [diff] [review]
Open readonly transaction for settings lock when possible r=qdot

Hiding this behind a pref, so that we can land and ask a set of people to dogfood and/or easily toggle off.

I've been using it on some devices for several days, but given how critical settings is, I'd prefer that we bake it properly.

Comment 8

[:gerard-majax]

Try with pref off: https://tbpl.mozilla.org/?tree=Try&rev=34aed7bd49a5

Comment 9

[:gerard-majax]

Try with pref on: https://tbpl.mozilla.org/?tree=Try&rev=4b80904713e8

Comment 10

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8537740 [details] [diff] [review]
Open readonly transaction for settings lock when possible r=qdot

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

r+ with comments addressed

::: b2g/app/b2g.js
@@ +1073,5 @@
> +
> +// Controlling whether we want to allow forcing some Settings
> +// IndexedDB transactions to be opened as readonly or keep everything as
> +// readwrite.
> +pref("dom.mozSettings.allowForceReadOnly", false);

This and the debug prefs above it should also be added to modules/libpref/init/all.js.

Comment 11

[:gerard-majax]

Attachment: Open readonly transaction for settings lock when possible r=qdot

Whenever a settings lock does only read operations (i.e. lock.get()),
there is no need that the underlying indexedDB transaction should be
opened as readwrite. Doing so, we will be able to parallelize some
request and avoid locking that should in the end allow for faster
operations. Enabling this feature is controlled by the preference
dom.mozSettings.allowForceReadOnly, defaulting to false for now.

Comment 12

[:gerard-majax]

Comment on attachment 8537879 [details] [diff] [review]
Open readonly transaction for settings lock when possible r=qdot

Should be addressing latest comment.

Comment 13

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8537879 [details] [diff] [review]
Open readonly transaction for settings lock when possible r=qdot

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

r+ with comments addressed. Which means you don't have to ask for r? again. :)

::: modules/libpref/init/all.js
@@ +4420,5 @@
> +// MozSettings debugging prefs for each component
> +pref("dom.mozSettings.SettingsDB.debug.enabled", true);
> +pref("dom.mozSettings.SettingsManager.debug.enabled", true);
> +pref("dom.mozSettings.SettingsRequestManager.debug.enabled", true);
> +pref("dom.mozSettings.SettingsService.debug.enabled", true);

You want all of these off by default here, most likely.

Comment 14

[:gerard-majax]

Attachment: Open readonly transaction for settings lock when possible r=qdot

Whenever a settings lock does only read operations (i.e. lock.get()),
there is no need that the underlying indexedDB transaction should be
opened as readwrite. Doing so, we will be able to parallelize some
request and avoid locking that should in the end allow for faster
operations. Enabling this feature is controlled by the preference
dom.mozSettings.allowForceReadOnly, defaulting to false for now.

Comment 15

[:gerard-majax]

Comment on attachment 8537924 [details] [diff] [review]
Open readonly transaction for settings lock when possible r=qdot

Carrying r+ from :qdot, just changing values of debug prefs.

Comment 16

[:gerard-majax]

See try in comment 8

Comment 17

[Ryan VanderMeulen [:RyanVM]]

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

Comment 18

[Carsten Book [:Tomcat]]

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