Bug 98465 – IndexedDB: Add histogram statistics for backing store errors

• Component: New Bugs
• Status: RESOLVED FIXED

[Joshua Bell]

IndexedDB: Add histogram statistics for backing store errors

[Joshua Bell]

Created attachment 167205 [details]
Patch

[Joshua Bell]

We may want to sprinkle InternalError() over other failure sites.

[Joshua Bell]

Other possible TODOs (before or after landing):

* Give InternalError() a message param which is logged/asserted/appended to histogram name
* Distinguish I/O errors (get/put failed, etc) from data consistency errors (decode failed)

[Joshua Bell]

Created attachment 167551 [details]
Patch

[Joshua Bell]

Sprinkled in a few more InternalError() macro calls.

I experimented with instrumenting the duration of open() and deleteDatabase() calls in the backing store, which led to me track down http://code.google.com/p/leveldb/issues/detail?id=125 .. because it wasn't showing up as a slowdown with either open or deleteDatabase in the backing store.  So I did not include those in the patch, but adding them is easy:

double startTime = monotonicallyIncreasingTime();
...
double durationMS = (monotonicallyIncreasingTime() - startTime) * 1000.0;
HistogramSupport::histogramCustomCounts("WebCore.IndexedDB.BackingStore.OpenDuration", static_cast<int>(durationMS), 0, 2000, 20);

The question is just when to measure. The backing store time may not be as interesting as measuring the time from the script call to "success". Or should that be "upgradeneeded"? Front end or back end? And is that enqueue or dispatch? And what if "blocked" fires?

Needs further discussion.

[David Grogan]

Comment on attachment 167551 [details]
Patch

LGTM as is if we don't get any suggestions.  Do you know how to look at this data?  I don't.

[Joshua Bell]

abarth@ - could you take a look at this and see if this follows our general strategy for histograms in WebCore, and also the style of the "InternalError" macro?

[Adam Barth]

Comment on attachment 167551 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=167551&action=review

> Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:64
> +#define InternalError() \
> +    do { \
> +        ASSERT_NOT_REACHED(); \
> +        LOG_ERROR("Internal IndexedDB Error"); \
> +        HistogramSupport::histogramEnumeration("WebCore.IndexedDB.BackingStore.InternalError", LevelDBBackingStoreInternalError, LevelDBBackingStoreInternalErrorMax); \
> +    } while (0)

Why a macro and not an inline function?

[Adam Barth]

(In reply to comment #7)
> abarth@ - could you take a look at this and see if this follows our general strategy for histograms in WebCore, and also the style of the "InternalError" macro?

That all seems fine.  As I mentioned above, I'm not sure why you've used a macro here rather than an inline function.  Perhaps LOG_ERROR works better that way?

[Joshua Bell]

(In reply to comment #9)
 As I mentioned above, I'm not sure why you've used a macro here rather than an inline function.  Perhaps LOG_ERROR works better that way?

Exactly - the ASSERT_NOT_REACHED() and LOG_ERROR() are both more useful when they have the original source line.

One thing that I can do which will IMHO make the code more readable is to move the HistogramSupport::histogramEnumeration(...) call itself into an inline function referenced by the macro, so the macro has less "payload".

[Joshua Bell]

Created attachment 170725 [details]
Patch

[Joshua Bell]

(In reply to comment #11)
> Created an attachment (id=170725) [details]
> Patch

Waiting on an internal review of the histograms metadata file. 

abarth@ - r?

[Adam Barth]

Comment on attachment 170725 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=170725&action=review

> Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:484
> -    return transaction->commit();
> +    if (!transaction->commit()) {
> +    }
> +    return true;

This looks a bit strange.  Is there a typo here?

[Joshua Bell]

Created attachment 170755 [details]
Patch

[Joshua Bell]

(In reply to comment #13)
> This looks a bit strange.  Is there a typo here?

Yes, half-aborted change. Fixed.

[WebKit Review Bot]

Comment on attachment 170755 [details]
Patch

Clearing flags on attachment: 170755

Committed r132568: <http://trac.webkit.org/changeset/132568>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.