| Summary: | Adds usage instrumentation for indexedDB | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Kassy Coan <kassycoan> | ||||||||||||||||
| Component: | New Bugs | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||
| Severity: | Normal | CC: | abarth, alecflett, buildbot, dglazkov, dgrogan, jsbell, mikelawther, rniwa, webkit.review.bot | ||||||||||||||||
| Priority: | P2 | ||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||
| Attachments: |
|
||||||||||||||||||
|
Description
Kassy Coan
2013-01-23 18:31:16 PST
Created attachment 184377 [details]
Patch
Tracking these API calls came via request from Chrome DevRel. Comment on attachment 184377 [details]
Patch
Looks reasonable to me, but jsbell should probably take a look.
(+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? Created attachment 184583 [details]
Patch
createIndex and deleteIndex now included. Whitespace corrected. 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 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. Created attachment 185159 [details]
Patch
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 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 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*)
(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 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. Yes, it's the front end calls that I am wanting to track. This information is being gathered because of a request from DevRel. Yes, there will be a histograms xml update following. Created attachment 185552 [details]
Patch
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 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. (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. tony@ or abarth@ could you review this? Thank you! Oops - the new header file is going to need to be added to the project files - gypi and so forth. Created attachment 185660 [details]
Patch
Comment on attachment 185660 [details] Patch Attachment 185660 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16251111 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 Tools/Scripts/build-webkit --chromium --release builds fine with ninja locally for me. Let's try again. Created attachment 185676 [details]
Retry cr-linux ews
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 Created attachment 185679 [details]
Retry cr-linux ews
Comment on attachment 185679 [details] Retry cr-linux ews Clearing flags on attachment: 185679 Committed r141735: <http://trac.webkit.org/changeset/141735> All reviewed patches have been landed. Closing bug. |