1371398 - Add telemetry for time spent getting and setting browser.storage.local

Status: RESOLVED FIXED

(WebExtensions :: General, enhancement, P2)

Description

[Bob Silverberg [:bsilverberg]]

We'd like to know how much time is spent during local.storage.get and local.storage.set, so we will introduce a couple of temporary (i.e., for the next 6 months) histograms to measure those.

Comment 1

[Bob Silverberg [:bsilverberg]]

We're talking about browser.storage.local here, despite my original typos.

Comment 2

[Bob Silverberg [:bsilverberg]]

There was some discussion about this on IRC, but nobody has added any comments to this bug, so, as per the discussion in today's meeting, I am going to proceed as planned.

Comment 3

[Bob Silverberg [:bsilverberg]]

Attachment: Bug 1371398 - Add telemetry for time spent getting and setting browser.storage.local,

Review commit: https://reviewboard.mozilla.org/r/151040/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/151040/

Comment 4

[Kris Maglione [:kmag]]

Comment on attachment 8879690 [details]
Bug 1371398 - Add telemetry for time spent getting and setting browser.storage.local,

https://reviewboard.mozilla.org/r/151040/#review158594

::: toolkit/components/extensions/ExtensionStorage.jsm:152
(Diff revision 1)
> +    let extensionId = context.extension.id;
> +    let extData = await this.read(extensionId);
> -      let changes = {};
> +    let changes = {};
> -      for (let prop in items) {
> +    for (let prop in items) {
> -        let item = items[prop];
> +      let item = items[prop];
> -        changes[prop] = {oldValue: extData[prop], newValue: item};
> +      changes[prop] = {oldValue: extData[prop], newValue: item};
> -        extData[prop] = item;
> +      extData[prop] = item;
> -      }
> +    }
>  
> -      this.notifyListeners(extensionId, changes);
> +    this.notifyListeners(extensionId, changes);
>  
> -      return this.write(extensionId);
> +    let result = await this.write(extensionId);

Some issues here:

1) We need to catch exceptions between the start() and finish().
2) We need to do this from ext-storage.js so we capture the overhead from messaging, serializing, and so forth.
3) The context is not a good enough context object for this, since we can have many parallel storage operations for the same context.

Comment 5

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8879690 [details]
Bug 1371398 - Add telemetry for time spent getting and setting browser.storage.local,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151040/diff/1-2/

Comment 6

[Kris Maglione [:kmag]]

Comment on attachment 8879690 [details]
Bug 1371398 - Add telemetry for time spent getting and setting browser.storage.local,

https://reviewboard.mozilla.org/r/151040/#review158948

::: toolkit/components/extensions/ext-c-storage.js:37
(Diff revision 2)
>      }
>      return {
>        storage: {
>          local: {
> -          get: function(keys) {
> +          get: async function(keys) {
> +            const stopwatchKey = {context, keys};

This can just be an empty object.

::: toolkit/components/extensions/ext-c-storage.js:47
(Diff revision 2)
> -              keys,
> +                keys,
> -            ]);
> +              ]);
> +              TelemetryStopwatch.finish(storageGetHistogram, stopwatchKey);
> +              return result;
> +            } catch (e) {
> +              TelemetryStopwatch.cancel(storageGetHistogram, stopwatchKey);

We need to re-throw the exception here.

::: toolkit/components/extensions/ext-c-storage.js:61
(Diff revision 2)
> -              items,
> +                items,
> -            ]);
> +              ]);
> +              TelemetryStopwatch.finish(storageSetHistogram, stopwatchKey);
> +              return result;
> +            } catch (e) {
> +              TelemetryStopwatch.cancel(storageSetHistogram, stopwatchKey);

We need to re-throw the exception here.

Comment 7

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8879690 [details]
Bug 1371398 - Add telemetry for time spent getting and setting browser.storage.local,

https://reviewboard.mozilla.org/r/151040/#review158948

> This can just be an empty object.

You mean I can just set it to an empty object? Is so, why would that work? I thought it was meant to be something unique? Or is it because an object created here will be unique each time it is created? If that's the case can I make the same change to the `set` method?

Comment 8

[Kris Maglione [:kmag]]

Comment on attachment 8879690 [details]
Bug 1371398 - Add telemetry for time spent getting and setting browser.storage.local,

https://reviewboard.mozilla.org/r/151040/#review158948

> You mean I can just set it to an empty object? Is so, why would that work? I thought it was meant to be something unique? Or is it because an object created here will be unique each time it is created? If that's the case can I make the same change to the `set` method?

A new object is created each time the method is called, and all that matters is the exact identity of the object, not what the object contains.

Comment 9

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8879690 [details]
Bug 1371398 - Add telemetry for time spent getting and setting browser.storage.local,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151040/diff/2-3/

Comment 10

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8879690 [details]
Bug 1371398 - Add telemetry for time spent getting and setting browser.storage.local,

Requesting data review for a temporary telemetry probe which will gather information about the performance of browser.storage.local in the wild, which we hope to use to make decisions about changing the implementation of the API.

Comment 11

[Benjamin Smedberg]

Comment on attachment 8879690 [details]
Bug 1371398 - Add telemetry for time spent getting and setting browser.storage.local,

https://reviewboard.mozilla.org/r/151040/#review160292

data-r=me

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a05f39ab6781
Add telemetry for time spent getting and setting browser.storage.local, r=bsmedberg,kmag

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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