[IndexedDB] Adding async tracing for renderer calls.

[IndexedDB] Adding async tracing for renderer calls.

Screenshot of new async tracing:
https://screenshot.googleplex.com/empkTdUte65

R=pwnall
BUG=723129
Review-Url: https://codereview.chromium.org/2890023003
Cr-Commit-Position: refs/heads/master@{#476107}
Committed: https://chromium.googlesource.com/chromium/src/+/2cef226d150bca11b6151dcf39f9ecc3ed71715f

[dmurph, 2017-05-17 21:01:17]

Description was changed from

==========
[IndexedDB] Adding async tracing for renderer calls.

TODO: cursor iteration tracing, and figure out blink/chromium boundaries

R=pwnall
BUG=723129
==========

to

==========
[IndexedDB] Adding async tracing for renderer calls.

TODO: cursor iteration tracing, and figure out blink/chromium boundaries

Screenshot of new async tracing:
https://screenshot.googleplex.com/empkTdUte65

R=pwnall
BUG=723129
==========

[dmurph, 2017-05-17 22:15:50]

The CQ bit was checked by dmurph@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-17 22:17:03]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dmurph, 2017-05-17 22:17:05]

Hello Victor,

Can you ptal at this patch? It adds tracing to IDB async calls, with the ability
to add UMA timings in the future (next patch).

Thanks!
Daniel

[dmurph, 2017-05-17 22:17:13]

Description was changed from

==========
[IndexedDB] Adding async tracing for renderer calls.

TODO: cursor iteration tracing, and figure out blink/chromium boundaries

Screenshot of new async tracing:
https://screenshot.googleplex.com/empkTdUte65

R=pwnall
BUG=723129
==========

to

==========
[IndexedDB] Adding async tracing for renderer calls.

Screenshot of new async tracing:
https://screenshot.googleplex.com/empkTdUte65

R=pwnall
BUG=723129
==========

[commit-bot: I haz the power, 2017-05-17 22:38:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-17 22:38:12]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[dmurph, 2017-05-17 22:56:57]

The CQ bit was checked by dmurph@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-17 22:58:20]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-18 01:23:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-18 01:23:04]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[dmurph, 2017-05-18 19:49:43]

The CQ bit was checked by dmurph@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-18 19:50:46]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dmurph, 2017-05-18 20:53:11]

The CQ bit was checked by dmurph@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-18 20:53:42]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[pwnall, 2017-05-18 22:14:32]

Here is a wave of nits.

https://codereview.chromium.org/2890023003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.h (right):

https://codereview.chromium.org/2890023003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.h:123: // Used by
IDBCursor::update():ccccc
typo?

https://codereview.chromium.org/2890023003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/indexeddb/IDBRequest.cpp (right):

https://codereview.chromium.org/2890023003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/indexeddb/IDBRequest.cpp:73:
UMA_HISTOGRAM_MEDIUM_TIMES(uma_name_, WTF::TimeTicks::Now() - start_);
Does this not record UMA data?

https://codereview.chromium.org/2890023003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/indexeddb/IDBRequest.h (right):

https://codereview.chromium.org/2890023003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/indexeddb/IDBRequest.h:70: // Records async
tracing, starting oncontruction and ending on destruction.
onconstruction -> missing space?

https://codereview.chromium.org/2890023003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/indexeddb/IDBRequest.h:214: void
MaybeRecordMetrics() {
metrics_ only seems to be null in testing.

1) Can this be RecordMetrics()? It seems to me that the testing situation is an
implementation detail.

2) How difficult would it be to pass non-null data all the time, so we don't
have separate code paths in unit tests vs everywhere else?

[dmurph, 2017-05-18 23:47:42]

The CQ bit was checked by dmurph@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-18 23:48:33]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dmurph, 2017-05-18 23:51:15]

Thanks, PTAL

https://codereview.chromium.org/2890023003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.h (right):

https://codereview.chromium.org/2890023003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.h:123: // Used by
IDBCursor::update():ccccc
On 2017/05/18 22:14:32, pwnall wrote:
> typo?

Done.

https://codereview.chromium.org/2890023003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/indexeddb/IDBRequest.cpp (right):

https://codereview.chromium.org/2890023003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/indexeddb/IDBRequest.cpp:73:
UMA_HISTOGRAM_MEDIUM_TIMES(uma_name_, WTF::TimeTicks::Now() - start_);
On 2017/05/18 22:14:32, pwnall wrote:
> Does this not record UMA data?

not anymore :P

https://codereview.chromium.org/2890023003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/indexeddb/IDBRequest.h (right):

https://codereview.chromium.org/2890023003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/indexeddb/IDBRequest.h:70: // Records async
tracing, starting oncontruction and ending on destruction.
On 2017/05/18 22:14:32, pwnall wrote:
> onconstruction -> missing space?

Done.

https://codereview.chromium.org/2890023003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/indexeddb/IDBRequest.h:214: void
MaybeRecordMetrics() {
On 2017/05/18 22:14:32, pwnall wrote:
> metrics_ only seems to be null in testing.
> 
> 1) Can this be RecordMetrics()? It seems to me that the testing situation is
an
> implementation detail.
> 
> 2) How difficult would it be to pass non-null data all the time, so we don't
> have separate code paths in unit tests vs everywhere else?

Done.

[commit-bot: I haz the power, 2017-05-19 03:03:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-19 03:03:45]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[pwnall, 2017-05-25 23:04:26]

This is useful information to surface, thank you very much for working on the
CL. Let's try to trim the overhead for the cases where tracing is not enabled :)

https://codereview.chromium.org/2890023003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/indexeddb/IDBCursor.cpp (right):

https://codereview.chromium.org/2890023003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IDBCursor.cpp:105:
IDB_TRACE("IDBCursor::updateCall");
The Call suffixes don't seem very informative to me. How about something like
RequestSetup / RequestCreation, or just Setup if length matters.

https://codereview.chromium.org/2890023003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/indexeddb/IDBCursor.h (right):

https://codereview.chromium.org/2890023003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IDBCursor.h:92: ExceptionState&,
I think ExceptionState should still be the last param, because it's an
out-param.

https://codereview.chromium.org/2890023003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/indexeddb/IDBRequest.cpp (right):

https://codereview.chromium.org/2890023003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IDBRequest.cpp:55:
IDBRequest::AsyncTraceState::AsyncTraceState()
You seem to use tracing_name_ for all the decisions, so you could save some
binary size and cycles by not setting id_ to nullptr ever (or you could just set
it in DCHECK-enabled builds, if you think it'd help catch bugs).

https://codereview.chromium.org/2890023003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IDBRequest.cpp:64:
IDBRequest::AsyncTraceState::AsyncTraceState(
I think that the constructor and assignment operators are small enough that we'd
save code size by having them in the header, especially after reducing the
number of nullptr assignments.

https://codereview.chromium.org/2890023003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/indexeddb/IDBRequest.h (right):

https://codereview.chromium.org/2890023003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IDBRequest.h:71: // a to
|RecordAndReset()|.
a call to?

[dmurph, 2017-05-31 19:26:25]

The CQ bit was checked by dmurph@chromium.org to run a CQ dry run

[dmurph, 2017-05-31 19:26:25]

https://codereview.chromium.org/2890023003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/indexeddb/IDBCursor.cpp (right):

https://codereview.chromium.org/2890023003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IDBCursor.cpp:105:
IDB_TRACE("IDBCursor::updateCall");
On 2017/05/25 23:04:25, pwnall wrote:
> The Call suffixes don't seem very informative to me. How about something like
> RequestSetup / RequestCreation, or just Setup if length matters.

Done.

https://codereview.chromium.org/2890023003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/indexeddb/IDBCursor.h (right):

https://codereview.chromium.org/2890023003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IDBCursor.h:92: ExceptionState&,
On 2017/05/25 23:04:25, pwnall wrote:
> I think ExceptionState should still be the last param, because it's an
> out-param.

Done.

https://codereview.chromium.org/2890023003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/indexeddb/IDBRequest.cpp (right):

https://codereview.chromium.org/2890023003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IDBRequest.cpp:55:
IDBRequest::AsyncTraceState::AsyncTraceState()
On 2017/05/25 23:04:25, pwnall wrote:
> You seem to use tracing_name_ for all the decisions, so you could save some
> binary size and cycles by not setting id_ to nullptr ever (or you could just
set
> it in DCHECK-enabled builds, if you think it'd help catch bugs).

Done.

https://codereview.chromium.org/2890023003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IDBRequest.cpp:64:
IDBRequest::AsyncTraceState::AsyncTraceState(
On 2017/05/25 23:04:25, pwnall wrote:
> I think that the constructor and assignment operators are small enough that
we'd
> save code size by having them in the header, especially after reducing the
> number of nullptr assignments.

Done.

https://codereview.chromium.org/2890023003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/indexeddb/IDBRequest.h (right):

https://codereview.chromium.org/2890023003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IDBRequest.h:71: // a to
|RecordAndReset()|.
On 2017/05/25 23:04:25, pwnall wrote:
> a call to?

Done.

[dmurph, 2017-05-31 21:01:28]

The CQ bit was checked by dmurph@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-31 21:02:40]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-31 21:23:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-31 21:23:10]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dmurph, 2017-05-31 22:02:34]

The CQ bit was checked by dmurph@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-31 22:03:10]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[pwnall, 2017-05-31 22:51:28]

LGTM.

Thank you for the revisions!

On 2017/05/31 19:26:25, dmurph wrote:
>
https://codereview.chromium.org/2890023003/diff/140001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/modules/indexeddb/IDBCursor.cpp (right):
> 
>
https://codereview.chromium.org/2890023003/diff/140001/third_party/WebKit/Sou...
> third_party/WebKit/Source/modules/indexeddb/IDBCursor.cpp:105:
> IDB_TRACE("IDBCursor::updateCall");
> On 2017/05/25 23:04:25, pwnall wrote:
> > The Call suffixes don't seem very informative to me. How about something
like
> > RequestSetup / RequestCreation, or just Setup if length matters.
> 
> Done.
> 
>
https://codereview.chromium.org/2890023003/diff/140001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/modules/indexeddb/IDBCursor.h (right):
> 
>
https://codereview.chromium.org/2890023003/diff/140001/third_party/WebKit/Sou...
> third_party/WebKit/Source/modules/indexeddb/IDBCursor.h:92: ExceptionState&,
> On 2017/05/25 23:04:25, pwnall wrote:
> > I think ExceptionState should still be the last param, because it's an
> > out-param.
> 
> Done.
> 
>
https://codereview.chromium.org/2890023003/diff/140001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/modules/indexeddb/IDBRequest.cpp (right):
> 
>
https://codereview.chromium.org/2890023003/diff/140001/third_party/WebKit/Sou...
> third_party/WebKit/Source/modules/indexeddb/IDBRequest.cpp:55:
> IDBRequest::AsyncTraceState::AsyncTraceState()
> On 2017/05/25 23:04:25, pwnall wrote:
> > You seem to use tracing_name_ for all the decisions, so you could save some
> > binary size and cycles by not setting id_ to nullptr ever (or you could just
> set
> > it in DCHECK-enabled builds, if you think it'd help catch bugs).
> 
> Done.
> 
>
https://codereview.chromium.org/2890023003/diff/140001/third_party/WebKit/Sou...
> third_party/WebKit/Source/modules/indexeddb/IDBRequest.cpp:64:
> IDBRequest::AsyncTraceState::AsyncTraceState(
> On 2017/05/25 23:04:25, pwnall wrote:
> > I think that the constructor and assignment operators are small enough that
> we'd
> > save code size by having them in the header, especially after reducing the
> > number of nullptr assignments.
> 
> Done.
> 
>
https://codereview.chromium.org/2890023003/diff/140001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/modules/indexeddb/IDBRequest.h (right):
> 
>
https://codereview.chromium.org/2890023003/diff/140001/third_party/WebKit/Sou...
> third_party/WebKit/Source/modules/indexeddb/IDBRequest.h:71: // a to
> |RecordAndReset()|.
> On 2017/05/25 23:04:25, pwnall wrote:
> > a call to?
> 
> Done.

[dmurph, 2017-05-31 23:27:16]

The CQ bit was unchecked by dmurph@chromium.org

[dmurph, 2017-05-31 23:27:20]

The CQ bit was checked by dmurph@chromium.org

[commit-bot: I haz the power, 2017-05-31 23:27:59]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-01 00:23:38]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1496273240384380,
"parent_rev": "f990567a85ba8196589a67de581fa2a5b16fae3e", "commit_rev":
"2cef226d150bca11b6151dcf39f9ecc3ed71715f"}

[commit-bot: I haz the power, 2017-06-01 00:23:45]

Description was changed from

==========
[IndexedDB] Adding async tracing for renderer calls.

Screenshot of new async tracing:
https://screenshot.googleplex.com/empkTdUte65

R=pwnall
BUG=723129
==========

to

==========
[IndexedDB] Adding async tracing for renderer calls.

Screenshot of new async tracing:
https://screenshot.googleplex.com/empkTdUte65

R=pwnall
BUG=723129

Review-Url: https://codereview.chromium.org/2890023003
Cr-Commit-Position: refs/heads/master@{#476107}
Committed:
https://chromium.googlesource.com/chromium/src/+/2cef226d150bca11b6151dcf39f9...
==========

[commit-bot: I haz the power, 2017-06-01 00:23:46]

Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/2cef226d150bca11b6151dcf39f9...