IndexedDB: Add histogram statistics for backing store errors
Created attachment 167205 [details] Patch
We may want to sprinkle InternalError() over other failure sites.
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)
Created attachment 167551 [details] Patch
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.
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.
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?
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?
(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?
(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".
Created attachment 170725 [details] Patch
(In reply to comment #11) > Created an attachment (id=170725) [details] > Patch Waiting on an internal review of the histograms metadata file. abarth@ - r?
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?
Created attachment 170755 [details] Patch
(In reply to comment #13) > This looks a bit strange. Is there a typo here? Yes, half-aborted change. Fixed.
Comment on attachment 170755 [details] Patch Clearing flags on attachment: 170755 Committed r132568: <http://trac.webkit.org/changeset/132568>
All reviewed patches have been landed. Closing bug.