Bug 105222 - IndexedDB: Split BackingStore histogram
Summary: IndexedDB: Split BackingStore histogram
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Grogan
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-12-17 15:47 PST by David Grogan
Modified: 2012-12-18 14:33 PST (History)
4 users (show)

See Also:


Attachments
Patch (4.91 KB, patch)
2012-12-17 16:12 PST, David Grogan
no flags Details | Formatted Diff | Diff
Patch (23.74 KB, patch)
2012-12-17 19:23 PST, David Grogan
no flags Details | Formatted Diff | Diff
Patch (23.76 KB, patch)
2012-12-17 19:28 PST, David Grogan
no flags Details | Formatted Diff | Diff
Patch for landing (23.74 KB, patch)
2012-12-18 14:08 PST, David Grogan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Grogan 2012-12-17 15:47:29 PST
IndexedDB: Only log to histogram on fatal errors
Comment 1 David Grogan 2012-12-17 16:12:01 PST
Created attachment 179823 [details]
Patch
Comment 2 David Grogan 2012-12-17 16:26:40 PST
Josh and Alec,

Are these indicative of logic errors rather than IO errors?  If so I will change them to emit ConsistencyError instead of InternalError.
Comment 3 Joshua Bell 2012-12-17 16:44:25 PST
Comment on attachment 179823 [details]
Patch

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

My intent was that InternalError would be used to flag (1) errors encountered at runtime due to bugs, bad disks, etc (2) errors generated during development by new bugs (e.g. developer forgets to write deletion code for a new field). 

Separating those concepts out into distinct macros is not a bad idea (I wanted to start off simple), but at first glance most of this patch is removing cases where #2 would be detected, which seems like a bad idea...

> Source/WebCore/Modules/indexeddb/IDBBackingStore.cpp:-620
> -            InternalError(IDBLevelDBBackingStoreReadErrorGetObjectStores);

This was to handle a case where we didn't correctly delete all object store metadata when a store was deleted, so we need skip leftover records. (Feel free to improve the comment.)

> Source/WebCore/Modules/indexeddb/IDBBackingStore.cpp:-633
> -            InternalError(IDBLevelDBBackingStoreReadErrorGetObjectStores);

The logic of this method could be improved significantly by doing a series of get() calls against the store rather than walking a cursor. It would simplify the method and make it more robust. Maybe it's time to do that?
Comment 4 Joshua Bell 2012-12-17 16:45:53 PST
(In reply to comment #2)
> Are these indicative of logic errors rather than IO errors?  If so I will change them to emit ConsistencyError instead of InternalError.

In a word: Yes.

If these trigger, it's because either a record has mysteriously vanished from the backing store or (more likely) we have a bug where we should have added/deleted a metadata entry but didn't.
Comment 5 David Grogan 2012-12-17 19:23:42 PST
Created attachment 179865 [details]
Patch
Comment 6 David Grogan 2012-12-17 19:28:33 PST
Created attachment 179866 [details]
Patch
Comment 7 David Grogan 2012-12-17 19:40:06 PST
This patch is the beginning of separating those concepts. The next will take advantage of the separation by making InternalReadError and maybe InternalWriteError also record successes.

This is being driven because GetRecord is responsible for 95% of the errors but we don't know if that's just because GetRecord is called 20x more than the rest of the functions.

Thoughts?
Comment 8 Joshua Bell 2012-12-18 11:01:43 PST
The latest patch LGTM.

(In reply to comment #7)
> This patch is the beginning of separating those concepts. The next will take advantage of the separation by making InternalReadError and maybe InternalWriteError also record successes.
> 
> This is being driven because GetRecord is responsible for 95% of the errors but we don't know if that's just because GetRecord is called 20x more than the rest of the functions.
> 
> Thoughts?

Sounds good. Being able to correlate with filesystem-level errors would be nice, e.g. knowing that a single write attempt is resulting in tight loop generating a million I/O errors. I guess we can iterate towards that.
Comment 9 David Grogan 2012-12-18 12:50:53 PST
Tony, could you review this?
Comment 10 Tony Chang 2012-12-18 13:39:32 PST
Comment on attachment 179866 [details]
Patch

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

> Source/WebCore/Modules/indexeddb/IDBBackingStore.cpp:57
> +    IDBLevelDBBackingStoreReadError_DEPRECATED,
> +    IDBLevelDBBackingStoreWriteError_DEPRECATED,
> +    IDBLevelDBBackingStoreConsistencyError_DEPRECATED,

Nit: I probably wouldn't name these. You could set FindKeyInIndex = 4 and have a comment here saying that 1-3 are no longer used.

> Source/WebCore/Modules/indexeddb/IDBBackingStore.cpp:96
> +#define InternalReadError(location) ReportError("Read", location)
> +#define InternalConsistencyError(location) ReportError("Consistency", location)
> +#define InternalWriteError(location) ReportError("Write", location)

Nit: It looks like most macros in WebCore are in all caps, even if it's a function.
Comment 11 David Grogan 2012-12-18 14:08:29 PST
Comment on attachment 179866 [details]
Patch

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

>> Source/WebCore/Modules/indexeddb/IDBBackingStore.cpp:57
>> +    IDBLevelDBBackingStoreConsistencyError_DEPRECATED,
> 
> Nit: I probably wouldn't name these. You could set FindKeyInIndex = 4 and have a comment here saying that 1-3 are no longer used.

Done.

>> Source/WebCore/Modules/indexeddb/IDBBackingStore.cpp:96
>> +#define InternalWriteError(location) ReportError("Write", location)
> 
> Nit: It looks like most macros in WebCore are in all caps, even if it's a function.

Fixed.
Comment 12 David Grogan 2012-12-18 14:08:44 PST
Created attachment 180021 [details]
Patch for landing
Comment 13 WebKit Review Bot 2012-12-18 14:33:30 PST
Comment on attachment 180021 [details]
Patch for landing

Clearing flags on attachment: 180021

Committed r138072: <http://trac.webkit.org/changeset/138072>
Comment 14 WebKit Review Bot 2012-12-18 14:33:33 PST
All reviewed patches have been landed.  Closing bug.