705845 - Add telemetry for size of values stored in localStorage

Status: RESOLVED FIXED

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

Description

[Nathan Froyd [:froydnj]]

We have a couple of bugs open regarding slowness of localStorage (bug 627635, bug 701880).  Before we consider solutions like sharding the database based on size of things being stored, we should have some idea of what people are storing in localStorage.

Getting the same for IndexedDB would be interesting as well.

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: patch

Here's a patch which adds telemetry for recording sizes of keys and values stored in localStorage.  There are separate telemetry histograms for local, session, and global storage, and also separate histograms for the size of keys and values.  (Keys should be short and values long(er), but we won't know until we get some data.)

Comment 2

[Marco Bonardo [:mak]]

Comment on attachment 577632 [details] [diff] [review]
patch

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

Once you address these comments, I'd like Honza (honzab.moz) to take a look at the next patch and approve it.

::: dom/src/storage/nsDOMStorage.cpp
@@ +1596,5 @@
>    return NS_OK;
>  }
>  
> +static Telemetry::ID
> +TelemetryIDForStorageType(nsPIDOMStorage::nsDOMStorageType type, bool forKey)

Add a javadoc before the method explaining what it does, what it return and the params.

The name of the second argument is cryptic, personally I'd just make 2 separate helper functions with decent self-documenting names, rather than passing an obscure bool.

@@ +1612,5 @@
> +    return (forKey
> +            ? Telemetry::SESSIONSTORAGE_KEY_SIZE_BYTES
> +            : Telemetry::SESSIONSTORAGE_VALUE_SIZE_BYTES);
> +  default:
> +    NS_RUNTIMEABORT("Unknown StorageType");

Doesn't look nice, while it's important we catch this, it should not hit users. rather use a MOZ_ASSERT(true) here, so it will always break in debug mode.

@@ +1614,5 @@
> +            : Telemetry::SESSIONSTORAGE_VALUE_SIZE_BYTES);
> +  default:
> +    NS_RUNTIMEABORT("Unknown StorageType");
> +    // Not reached.
> +    return Telemetry::HistogramCount;

imo the comment is useless.

::: dom/tests/unit/test_domstorage_aboutpages.js
@@ +18,5 @@
> +  let sum = 0;
> +  for (let i = 0; i < a.length; ++i) {
> +    sum += a[i];
> +  }
> +  return sum;

javascript is your friend!
https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/Array/Reduce

::: toolkit/components/telemetry/TelemetryHistograms.h
@@ +259,5 @@
> +HISTOGRAM(GLOBALSTORAGE_VALUE_SIZE_BYTES, 1024, 32768, 10, EXPONENTIAL, "DOM storage: size of values stored in global storage")
> +HISTOGRAM(LOCALSTORAGE_KEY_SIZE_BYTES, 1024, 32768, 10, EXPONENTIAL, "DOM storage: size of keys set in local storage")
> +HISTOGRAM(LOCALSTORAGE_VALUE_SIZE_BYTES, 1024, 32768, 10, EXPONENTIAL, "DOM storage: size of values stored in local storage")
> +HISTOGRAM(SESSIONSTORAGE_KEY_SIZE_BYTES, 1024, 32768, 10, EXPONENTIAL, "DOM storage: size of keys set in session storage")
> +HISTOGRAM(SESSIONSTORAGE_VALUE_SIZE_BYTES, 1024, 32768, 10, EXPONENTIAL, "DOM storage: size of values stored in session storage")

just to namespace these better, I'd probably suggest naming as
GLOBALDOMSTORAGE, LOCALDOMSTORAGE, SESSIONDOMSTORAGE

nit: you may probably make macro generating KEY/VALUE pairs and just
DOMSTORAGE_KEY_VAL_SIZE(GLOBAL, "global")
DOMSTORAGE_KEY_VAL_SIZE(LOCAL, "local")
DOMSTORAGE_KEY_VAL_SIZE(SESSION, "session")
would then be easier to adjust values without having to copy paste a bunch of times.

Comment 3

[Nathan Froyd [:froydnj]]

Thanks for the review.  One comment here:

(In reply to Marco Bonardo [:mak] from comment #2)
> @@ +1612,5 @@
> > +    return (forKey
> > +            ? Telemetry::SESSIONSTORAGE_KEY_SIZE_BYTES
> > +            : Telemetry::SESSIONSTORAGE_VALUE_SIZE_BYTES);
> > +  default:
> > +    NS_RUNTIMEABORT("Unknown StorageType");
> 
> Doesn't look nice, while it's important we catch this, it should not hit
> users. rather use a MOZ_ASSERT(true) here, so it will always break in debug
> mode.
> 
> @@ +1614,5 @@
> > +            : Telemetry::SESSIONSTORAGE_VALUE_SIZE_BYTES);
> > +  default:
> > +    NS_RUNTIMEABORT("Unknown StorageType");
> > +    // Not reached.
> > +    return Telemetry::HistogramCount;
> 
> imo the comment is useless.

The comment should have been more verbose.  We have to return something in debug and optimized builds; the compiler is going to complain that we're not returning anything in the default case.  We could fix MOZ_ASSERT (and/or NS_RUNTIMEABORT) so the compiler knows what's going on: that the underlying functions have the same behavior as abort().

I guess we can return one of the IDs we returned in the previous cases; I don't think it'd be a good idea to return HistogramCount here.  WDYT?

Comment 4

[Nathan Froyd [:froydnj]]

Attachment: patch v2

New patch addressing Mak's comments.  The only one not addressed was the request for javadocs; I figured splitting the function into two with the self-documenting names removed the ned for comments.

Comment 5

[Andrew McCreight [:mccr8]]

Marking [Snappy:P1] because this will help us measure a major source of hangs.

Comment 6

[Honza Bambas (:mayhemer)]

Comment on attachment 578312 [details] [diff] [review]
patch v2

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

r=honzab, let's see how this works.

Please address the comments:

::: dom/tests/unit/test_domstorage_aboutpages.js
@@ +14,5 @@
>  }
>  
> +function sum(a)
> +{
> +  return a.reduce(function (prev, current, index, array) {

No space between function and (

@@ +24,5 @@
>  {
>    print("Testing: " + aURI.spec);
>    let storage = getStorageForURI(aURI);
> +  let Telemetry = Components.classes["@mozilla.org/base/telemetry;1"].
> +	getService(Components.interfaces.nsITelemetry);

Indention, looks like you are using tabs, use spaces please:

let Telemetry = Components.classes["@mozilla.org/base/telemetry;1"].
                getService(Components.interfaces.nsITelemetry);

::: toolkit/components/telemetry/TelemetryHistograms.h
@@ +252,5 @@
>  HISTOGRAM(XUL_INITIAL_FRAME_CONSTRUCTION, 1, 3000, 10, EXPONENTIAL, "initial xul frame construction")
>  HISTOGRAM(XMLHTTPREQUEST_ASYNC_OR_SYNC, 0, 1, 2, BOOLEAN, "Type of XMLHttpRequest, async or sync")
> +
> +/**
> + * DOM telemetry.

DOM Storage telemetry ?

@@ +256,5 @@
> + * DOM telemetry.
> + */
> +#define DOMSTORAGE_HISTOGRAM(PREFIX, TYPE, TYPESTRING, DESCRIPTION) \
> +  HISTOGRAM(PREFIX ## DOMSTORAGE_ ## TYPE ## _SIZE_BYTES, \
> +            1024, 32768, 10, EXPONENTIAL, "DOM storage: size of " TYPESTRING "s stored in " DESCRIPTION " storage")

s/" storage"/"Storage"/ to display "localStorage", "sessionStorage" etc.. It is the name of the feature/object.

Comment 7

[Nathan Froyd [:froydnj]]

Attachment: patch v3

New patch addressing Honza's comments.  Carrying over r+.

Comment 8

[Dão Gottwald [:dao]]

Patch doesn't seem to apply cleanly in TelemetryHistograms.h.

Comment 9

[Andrew McCreight [:mccr8]]

That's probably my fault.  The fix should be trivial.  I can land it if it is.

Comment 10

[Andrew McCreight [:mccr8]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/653fde03989c

Comment 11

[Nathan Froyd [:froydnj]]

Attachment: patch v4, rebased to trunk

Rebased patch to fix conflicts.

Comment 12

[Nathan Froyd [:froydnj]]

Shucks, didn't see Andrew's tweaking.  Thanks, Andrew!

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/653fde03989c