Add telemetry for size of values stored in localStorage

RESOLVED FIXED in mozilla11

Status

()

Core
DOM
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: froydnj, Assigned: froydnj)

Tracking

Trunk
mozilla11
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Snappy:P1])

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

6 years ago
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.

Updated

6 years ago
Whiteboard: [Snappy:P1]

Updated

6 years ago
Whiteboard: [Snappy:P1] → [Snappy]
(Assignee)

Comment 1

6 years ago
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.)
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+
(Assignee)

Comment 3

6 years ago
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?
(Assignee)

Comment 4

6 years ago
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.
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+
(Assignee)

Comment 7

6 years ago
Created attachment 578394 [details] [diff] [review]
patch v3

New patch addressing Honza's comments.  Carrying over r+.
Attachment #578312 - Attachment is obsolete: true
Attachment #578394 - Flags: review+
(Assignee)

Updated

6 years ago
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.
https://hg.mozilla.org/integration/mozilla-inbound/rev/653fde03989c
Target Milestone: --- → mozilla11
(Assignee)

Comment 11

6 years ago
Created attachment 578571 [details] [diff] [review]
patch v4, rebased to trunk

Rebased patch to fix conflicts.
Attachment #578394 - Attachment is obsolete: true
Attachment #578571 - Flags: review+
(Assignee)

Comment 12

6 years ago
Shucks, didn't see Andrew's tweaking.  Thanks, Andrew!
https://hg.mozilla.org/mozilla-central/rev/653fde03989c
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.