Last Comment Bug 705845 - Add telemetry for size of values stored in localStorage
: Add telemetry for size of values stored in localStorage
Status: RESOLVED FIXED
[Snappy:P1]
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla11
Assigned To: Nathan Froyd [:froydnj]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-28 12:58 PST by Nathan Froyd [:froydnj]
Modified: 2012-02-01 13:57 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (6.39 KB, patch)
2011-11-29 08:38 PST, Nathan Froyd [:froydnj]
mak77: feedback+
Details | Diff | Splinter Review
patch v2 (6.45 KB, patch)
2011-12-01 10:47 PST, Nathan Froyd [:froydnj]
honzab.moz: review+
Details | Diff | Splinter Review
patch v3 (6.47 KB, patch)
2011-12-01 14:27 PST, Nathan Froyd [:froydnj]
nfroyd: review+
Details | Diff | Splinter Review
patch v4, rebased to trunk (4.90 KB, patch)
2011-12-02 07:09 PST, Nathan Froyd [:froydnj]
nfroyd: review+
Details | Diff | Splinter Review

Description Nathan Froyd [:froydnj] 2011-11-28 12:58:05 PST
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] 2011-11-29 08:38:23 PST
Created attachment 577632 [details] [diff] [review]
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] (Away 6-20 Aug) 2011-12-01 07:11:44 PST
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] 2011-12-01 07:56:22 PST
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] 2011-12-01 10:47:58 PST
Created attachment 578312 [details] [diff] [review]
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] 2011-12-01 12:34:12 PST
Marking [Snappy:P1] because this will help us measure a major source of hangs.
Comment 6 Honza Bambas (:mayhemer) 2011-12-01 12:41:34 PST
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] 2011-12-01 14:27:35 PST
Created attachment 578394 [details] [diff] [review]
patch v3

New patch addressing Honza's comments.  Carrying over r+.
Comment 8 Dão Gottwald [:dao] 2011-12-02 04:31:09 PST
Patch doesn't seem to apply cleanly in TelemetryHistograms.h.
Comment 9 Andrew McCreight [:mccr8] 2011-12-02 05:48:52 PST
That's probably my fault.  The fix should be trivial.  I can land it if it is.
Comment 10 Andrew McCreight [:mccr8] 2011-12-02 06:07:23 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/653fde03989c
Comment 11 Nathan Froyd [:froydnj] 2011-12-02 07:09:18 PST
Created attachment 578571 [details] [diff] [review]
patch v4, rebased to trunk

Rebased patch to fix conflicts.
Comment 12 Nathan Froyd [:froydnj] 2011-12-02 07:09:57 PST
Shucks, didn't see Andrew's tweaking.  Thanks, Andrew!
Comment 13 Ed Morley [:emorley] 2011-12-02 12:04:51 PST
https://hg.mozilla.org/mozilla-central/rev/653fde03989c

Note You need to log in before you can comment on or make changes to this bug.