Bug 107772 - Adds usage instrumentation for indexedDB
Summary: Adds usage instrumentation for indexedDB
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: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-01-23 18:31 PST by Kassy Coan
Modified: 2013-02-03 21:13 PST (History)
9 users (show)

See Also:


Attachments
Patch (8.68 KB, patch)
2013-01-23 18:40 PST, Kassy Coan
no flags Details | Formatted Diff | Diff
Patch (9.82 KB, patch)
2013-01-24 14:49 PST, Kassy Coan
no flags Details | Formatted Diff | Diff
Patch (6.89 KB, patch)
2013-01-28 22:35 PST, Kassy Coan
no flags Details | Formatted Diff | Diff
Patch (6.83 KB, patch)
2013-01-30 13:37 PST, Kassy Coan
no flags Details | Formatted Diff | Diff
Patch (11.77 KB, patch)
2013-01-30 19:45 PST, Kassy Coan
no flags Details | Formatted Diff | Diff
Retry cr-linux ews (11.78 KB, patch)
2013-01-30 21:55 PST, Kassy Coan
no flags Details | Formatted Diff | Diff
Retry cr-linux ews (10.93 KB, patch)
2013-01-30 22:34 PST, Kassy Coan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kassy Coan 2013-01-23 18:31:16 PST
Adds usage instrumentation for indexedDB
Comment 1 Kassy Coan 2013-01-23 18:40:57 PST
Created attachment 184377 [details]
Patch
Comment 2 Kassy Coan 2013-01-23 18:53:18 PST
Tracking these API calls came via request from Chrome DevRel.
Comment 3 Adam Barth 2013-01-23 21:30:49 PST
Comment on attachment 184377 [details]
Patch

Looks reasonable to me, but jsbell should probably take a look.
Comment 4 Joshua Bell 2013-01-24 09:17:31 PST
(+dgrogan@ who's been working on reporting for the IDB histograms; he's unavailable for the next week or so, and won't be able to look at this.)

This looks fine to me - I assume there will be a Chromium-side histogram XML update coming too.

Nits: whitespace around the histogram calls is inconsistent.

Questions: Why not instrument Create/DeleteIndex at the same time? Is there a specific reason to target just these methods?
Comment 5 Kassy Coan 2013-01-24 14:49:40 PST
Created attachment 184583 [details]
Patch
Comment 6 Kassy Coan 2013-01-24 14:53:40 PST
createIndex and deleteIndex now included.
Whitespace corrected.
Comment 7 David Grogan 2013-01-24 16:40:02 PST
Comment on attachment 184583 [details]
Patch

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

> Source/WebCore/Modules/indexeddb/IDBBackingStore.cpp:356
> +    IDBCreateObjectStore,

FYI, the enum at the top of this file, IDBBackingStoreErrorSource, essentially lists methods in this file and might be a better place for these new entries. Or you might want a new enum that is just for API methods. Whatever jsbell@ is ok with is fine.
Comment 8 Joshua Bell 2013-01-25 08:04:26 PST
Comment on attachment 184583 [details]
Patch

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

>> Source/WebCore/Modules/indexeddb/IDBBackingStore.cpp:356
>> +    IDBCreateObjectStore,
> 
> FYI, the enum at the top of this file, IDBBackingStoreErrorSource, essentially lists methods in this file and might be a better place for these new entries. Or you might want a new enum that is just for API methods. Whatever jsbell@ is ok with is fine.

David's right, I wasn't looking at the whole file.

The existing histograms/enums are used for (1) identifying the source of errors, and (2) flow through open() which has complex branching logic and ideally where on-disk corruption shows up.

This new histogram is just tracking API usage, but at the backing store level. That leads to a question: why track here, rather than at the entry point from script (e.g. for create/put/etc calls) and event delivery (e.g. transaction creation/complete/abort)? There are pros and cons to each approach (e.g. simpler to do it at the backing store level and maps to actual data changes, v.s. correlating exact with script calls). Without knowing what you're attempting to learn from this (i.e. what actions would we take based on this data? is it for helping users debug their script in local Chrome sessions?) it's hard to know which to recommend.

Either way, given how specific the other two enums are I'd actually recommend separating this out into a third enum, and leaving the existing ones alone (and focused on errors). It will probably make the code clearer and analysis of the histogram data easier.
Comment 9 Kassy Coan 2013-01-28 22:35:36 PST
Created attachment 185159 [details]
Patch
Comment 10 Kassy Coan 2013-01-28 22:39:00 PST
Separated out into 3rd enum.
Histograms also separated for clarity.
Includes 1 histogram with detailed open errors (WebCore.IndexedDB.BackingStore.OpenStatus) , and 1 histogram of general indexedDB method calls (WebCore.IndexedDB) .
Comment 11 Build Bot 2013-01-29 04:59:41 PST
Comment on attachment 185159 [details]
Patch

Attachment 185159 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16199111

New failing tests:
fast/workers/worker-document-leak.html
Comment 12 Alec Flett 2013-01-29 11:38:53 PST
Comment on attachment 185159 [details]
Patch

So I want to point out that you're adding histograms in the backend code, which does not necessarily have a 1:1 relationship with the frontend API calls. If you're interested in instrumenting Javascript API usage, this isn't the way.

For example, things like IDBBackingStore::Transaction::commit may or may not correspond directly with a frontend commit - for instance readonly transactions may never commit (I'm not sure if they do or do not right now) and internal changes to IDB (like internal version changes between chrome releases) may or may not result in a commit.

In addition, as the code changes and evolves, there may be caching layers between the layer you are instrumenting and the frontend APIs.

My suggestion would be to instrument the frontend (i.e. stuff like IDBTransaction.cpp and IDBObjectStore.cpp) and avoid anything in the backend. (i.e. IDBBackingStore* and IDB*BackendI*)
Comment 13 Joshua Bell 2013-01-29 14:49:01 PST
(In reply to comment #12)
> In addition, as the code changes and evolves, there may be caching layers between the layer you are instrumenting and the frontend APIs.
> 
> My suggestion would be to instrument the frontend (i.e. stuff like IDBTransaction.cpp and IDBObjectStore.cpp) and avoid anything in the backend. (i.e. IDBBackingStore* and IDB*BackendI*)

It's not clear from this bug/patch what data kassycoan@ is really trying to gather. If it's backend operations, this approach is fine. If it's script usage, I agree with Alec.

Kassy, can you share more about the intent?
Comment 14 Joshua Bell 2013-01-29 14:53:55 PST
Comment on attachment 185159 [details]
Patch

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

> Source/WebCore/ChangeLog:6
> +        abarth@webkit.org

This should be left as "Reviewed by NOBODY (OOPS!)." until the patch actually has an r+ from a reviewer.

> Source/WebCore/ChangeLog:7
> +        jsbell@chromium.org

...and I'm not a reviewer (which is a special role in WebKit), just an interested bystander. :)

> Source/WebCore/Modules/indexeddb/IDBBackingStore.cpp:376
> +    HistogramSupport::histogramEnumeration("WebCore.IndexedDB", IDBOpen, IDBMethodsMax);

In theory, this could be derived from the number of OpenResult histogram entries, but given the branching in this method I agree that adding this would be beneficial.
Comment 15 Kassy Coan 2013-01-29 15:29:27 PST
Yes, it's the front end calls that I am wanting to track.
This information is being gathered because of a request from DevRel.
Comment 16 Kassy Coan 2013-01-29 15:36:44 PST
Yes, there will be a histograms xml update following.
Comment 17 Kassy Coan 2013-01-30 13:37:26 PST
Created attachment 185552 [details]
Patch
Comment 18 Kassy Coan 2013-01-30 13:39:51 PST
This new patch adds the histogramming in the other .cpp files (not backing store). This should be tracking front end API calls.
I saw it best fit to declare the enum in a new .h file. Please let me know if you think there is a more appropriate place.
Comment 19 Joshua Bell 2013-01-30 16:52:02 PST
Comment on attachment 185552 [details]
Patch

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

Overall this seems fine to me. Sorry about the churn! It might be worth grepping for IDB_TRACE to see were we've instrumented other API call sites for performance.

> Source/WebCore/Modules/indexeddb/IDBHistograms.h:35
> +    IDBCreateObjectStoreCall,

About half the histograms explicitly set the first item to = 0. Shouldn't be necessary.

Would this benefit from a comment that this list should only be added to, since these values are sent over the wire? or is the fact that this lives in a Histograms file clear enough?

Also, although not important for this set, perhaps a convention of IDB<objectname><methodname> would be good to introduce, to eventually disambiguate IDBObjectStore::openCursor from IDBIndex::openCursor. That can also be done later, though.
Comment 20 Joshua Bell 2013-01-30 16:54:16 PST
(In reply to comment #19)
> ... see were we've instrumented other API call sites ...

er, entry points, not call sites. We may be getting an IDB-specific timeline in the Inspector at some point; some unified logging/tracing macro might be handy to introduce in the future.
Comment 21 Kassy Coan 2013-01-30 17:10:45 PST
tony@ or abarth@ could you review this? Thank you!
Comment 22 Joshua Bell 2013-01-30 17:14:27 PST
Oops - the new header file is going to need to be added to the project files - gypi and so forth.
Comment 23 Kassy Coan 2013-01-30 19:45:22 PST
Created attachment 185660 [details]
Patch
Comment 24 WebKit Review Bot 2013-01-30 19:47:55 PST
Comment on attachment 185660 [details]
Patch

Attachment 185660 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16251111
Comment 25 Kassy Coan 2013-01-30 19:49:19 PST
New header file now added to project files:
  Source/WebCore/GNUmakefile.list.am
  Source/WebCore/Target.pri
  Source/WebCore/WebCore.gypi
  Source/WebCore/WebCore.vcproj/WebCore.vcproj
  Source/WebCore/WebCore.xcodeproj/project.pbxproj
Comment 26 Kassy Coan 2013-01-30 21:52:24 PST
Tools/Scripts/build-webkit --chromium --release builds fine with ninja locally for me. Let's try again.
Comment 27 Kassy Coan 2013-01-30 21:55:59 PST
Created attachment 185676 [details]
Retry cr-linux ews
Comment 28 WebKit Review Bot 2013-01-30 22:02:09 PST
Comment on attachment 185676 [details]
Retry cr-linux ews

Attachment 185676 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16264062
Comment 29 Kassy Coan 2013-01-30 22:34:10 PST
Created attachment 185679 [details]
Retry cr-linux ews
Comment 30 WebKit Review Bot 2013-02-03 21:13:10 PST
Comment on attachment 185679 [details]
Retry cr-linux ews

Clearing flags on attachment: 185679

Committed r141735: <http://trac.webkit.org/changeset/141735>
Comment 31 WebKit Review Bot 2013-02-03 21:13:15 PST
All reviewed patches have been landed.  Closing bug.