Bug 107772 – Adds usage instrumentation for indexedDB

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Kassy Coan]

Adds usage instrumentation for indexedDB

[Kassy Coan]

Created attachment 184377 [details]
Patch

[Kassy Coan]

Tracking these API calls came via request from Chrome DevRel.

[Adam Barth]

Comment on attachment 184377 [details]
Patch

Looks reasonable to me, but jsbell should probably take a look.

[Joshua Bell]

(+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?

[Kassy Coan]

Created attachment 184583 [details]
Patch

[Kassy Coan]

createIndex and deleteIndex now included.
Whitespace corrected.

[David Grogan]

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.

[Joshua Bell]

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.

[Kassy Coan]

Created attachment 185159 [details]
Patch

[Kassy Coan]

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) .

[Build Bot]

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

[Alec Flett]

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*)

[Joshua Bell]

(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?

[Joshua Bell]

Comment on attachment 185159 [details]
Patch

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

> Source/WebCore/ChangeLog:6
> +        [e-mail]

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

> Source/WebCore/ChangeLog:7
> +        [e-mail]

...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.

[Kassy Coan]

Yes, it's the front end calls that I am wanting to track.
This information is being gathered because of a request from DevRel.

[Kassy Coan]

Yes, there will be a histograms xml update following.

[Kassy Coan]

Created attachment 185552 [details]
Patch

[Kassy Coan]

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.

[Joshua Bell]

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.

[Joshua Bell]

(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.

[Kassy Coan]

tony@ or abarth@ could you review this? Thank you!

[Joshua Bell]

Oops - the new header file is going to need to be added to the project files - gypi and so forth.

[Kassy Coan]

Created attachment 185660 [details]
Patch

[WebKit Review Bot]

Comment on attachment 185660 [details]
Patch

Attachment 185660 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16251111

[Kassy Coan]

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

[Kassy Coan]

Tools/Scripts/build-webkit --chromium --release builds fine with ninja locally for me. Let's try again.

[Kassy Coan]

Created attachment 185676 [details]
Retry cr-linux ews

[WebKit Review Bot]

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

[Kassy Coan]

Created attachment 185679 [details]
Retry cr-linux ews

[WebKit Review Bot]

Comment on attachment 185679 [details]
Retry cr-linux ews

Clearing flags on attachment: 185679

Committed r141735: <http://trac.webkit.org/changeset/141735>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.