Closed Bug 705845 Opened 8 years ago Closed 8 years ago

Add telemetry for size of values stored in localStorage

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla11

People

(Reporter: froydnj, Assigned: froydnj)

Details

(Whiteboard: [Snappy:P1])

Attachments

(1 file, 3 obsolete files)

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.
Whiteboard: [Snappy:P1]
Whiteboard: [Snappy:P1] → [Snappy]
Attached patch patch (obsolete) — Splinter Review
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.)
Assignee: nobody → nfroyd
Status: NEW → ASSIGNED
Attachment #577632 - Flags: review?(mak77)
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.
Attachment #577632 - Flags: review?(mak77) → feedback+
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?
Attached patch patch v2 (obsolete) — Splinter Review
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.
Attachment #577632 - Attachment is obsolete: true
Attachment #578312 - Flags: review?(honzab.moz)
Marking [Snappy:P1] because this will help us measure a major source of hangs.
Whiteboard: [Snappy] → [Snappy:P1]
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.
Attachment #578312 - Flags: review?(honzab.moz) → review+
Attached patch patch v3 (obsolete) — Splinter Review
New patch addressing Honza's comments.  Carrying over r+.
Attachment #578312 - Attachment is obsolete: true
Attachment #578394 - Flags: review+
Keywords: checkin-needed
Patch doesn't seem to apply cleanly in TelemetryHistograms.h.
Keywords: checkin-needed
That's probably my fault.  The fix should be trivial.  I can land it if it is.
Rebased patch to fix conflicts.
Attachment #578394 - Attachment is obsolete: true
Attachment #578571 - Flags: review+
Shucks, didn't see Andrew's tweaking.  Thanks, Andrew!
https://hg.mozilla.org/mozilla-central/rev/653fde03989c
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.