WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
105222
IndexedDB: Split BackingStore histogram
https://bugs.webkit.org/show_bug.cgi?id=105222
Summary
IndexedDB: Split BackingStore histogram
David Grogan
Reported
2012-12-17 15:47:29 PST
IndexedDB: Only log to histogram on fatal errors
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
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
David Grogan
Comment 1
2012-12-17 16:12:01 PST
Created
attachment 179823
[details]
Patch
David Grogan
Comment 2
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.
Joshua Bell
Comment 3
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?
Joshua Bell
Comment 4
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.
David Grogan
Comment 5
2012-12-17 19:23:42 PST
Created
attachment 179865
[details]
Patch
David Grogan
Comment 6
2012-12-17 19:28:33 PST
Created
attachment 179866
[details]
Patch
David Grogan
Comment 7
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?
Joshua Bell
Comment 8
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.
David Grogan
Comment 9
2012-12-18 12:50:53 PST
Tony, could you review this?
Tony Chang
Comment 10
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.
David Grogan
Comment 11
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.
David Grogan
Comment 12
2012-12-18 14:08:44 PST
Created
attachment 180021
[details]
Patch for landing
WebKit Review Bot
Comment 13
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
>
WebKit Review Bot
Comment 14
2012-12-18 14:33:33 PST
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug