[IndexedDB] Adding traces, perf tests

[IndexedDB] Adding traces, perf tests

BUG=525141
CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:linux_perf_bisect;tryserver.chromium.perf:win_perf_bisect;tryserver.chromium.perf:android_nexus5_perf_bisect

Committed: https://crrev.com/3723846d9bf63d802a8cae98fbff565a7b7dc526
Cr-Commit-Position: refs/heads/master@{#349853}

[dmurph, 2015-08-03 10:59:24]

dmurph@chromium.org changed reviewers:
+ cmumford@chromium.org, dgrogan@chromium.org, nednguyen@google.com

[dmurph, 2015-08-03 10:59:25]

Hey all,

This patch:
* Adds more tracing to IDB to give us a better idea of where the performance is
going
* Adds performance metrics for the important traces
* Adds an easy way to track a any accumulated timeline metric in a pageset!

Can you all PTAL?

Thanks,
Daniel

[nednguyen, 2015-08-03 15:11:08]

nednguyen@google.com changed reviewers:
+ aiolos@chromium.org

[nednguyen, 2015-08-03 16:12:21]

You are adding a lot of metrics at once! Have you run the benchmarks dozen time
to see how noisy each of these are?

Also please follow the process listed in Annie's doc:
https://docs.google.com/document/d/1bBKyYCW3VlUUPDpQE4xvrMFdA6tovQMZoqO9KCcmq...

https://codereview.chromium.org/1238393003/diff/40001/tools/telemetry/telemet...
File tools/telemetry/telemetry/web_perf/metrics/trace_event_aggregator.py
(right):

https://codereview.chromium.org/1238393003/diff/40001/tools/telemetry/telemet...
tools/telemetry/telemetry/web_perf/metrics/trace_event_aggregator.py:34:
"""Reports durations and counts of given trace events.
This seems similar to single_event in web_perf/metrics. Though I like your
version better.

[cmumford, 2015-08-03 17:31:34]

Can you either add a bug for this, or remove the empty 'BUG=' from the
description and add a descriptive comment with the motivation behind this
change?

https://codereview.chromium.org/1238393003/diff/40001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_backing_store.cc (right):

https://codereview.chromium.org/1238393003/diff/40001/content/browser/indexed...
content/browser/indexed_db/indexed_db_backing_store.cc:1479:
IDB_TRACE("IndexedDBBackingStore::DeleteDatabase::DeleteEntries");
Nit: I believe that all other traces we use <class>::<method>, but DeleteEntries
isn't a method. What about a different syntax, like
IndexedDBBackingStore::DeleteDatabase.DeleteEntries,
IndexedDBBackingStore::DeleteDatabase=>DeleteEntries, or something do indicate
it is a part of a method instead of an entire method.

Just wanted to throw that out for discussion.

https://codereview.chromium.org/1238393003/diff/40001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_transaction.cc (right):

https://codereview.chromium.org/1238393003/diff/40001/content/browser/indexed...
content/browser/indexed_db/indexed_db_transaction.cc:319:
IDB_TRACE1("IndexedDBTransaction:TransactionCompleteCallbacks:", "txn.id",
colon in wrong place.

https://codereview.chromium.org/1238393003/diff/40001/content/browser/indexed...
File content/browser/indexed_db/leveldb/leveldb_database.cc (right):

https://codereview.chromium.org/1238393003/diff/40001/content/browser/indexed...
content/browser/indexed_db/leveldb/leveldb_database.cc:111:
IDB_TRACE("LevelDBDatabase::Open");
What do you think about actually making this a static member function named
LevelDBDatabase::Open?

https://codereview.chromium.org/1238393003/diff/40001/content/browser/indexed...
content/browser/indexed_db/leveldb/leveldb_database.cc:429:
TRACE_EVENT0("LevelDB", "LevelDBDatabase::Compact");
Why TRACE_EVENT instead of IDB_TRACE?

https://codereview.chromium.org/1238393003/diff/40001/tools/perf/benchmarks/i...
File tools/perf/benchmarks/indexeddb_perf.py (right):

https://codereview.chromium.org/1238393003/diff/40001/tools/perf/benchmarks/i...
tools/perf/benchmarks/indexeddb_perf.py:61:
tab.WaitForJavaScriptExpression('window.done == true', 600)
Nit: why not just 'window.done'?

https://codereview.chromium.org/1238393003/diff/40001/tools/perf/benchmarks/i...
tools/perf/benchmarks/indexeddb_perf.py:117: if 'idb' not in value.name:
This logic makes me think way too much. Why not just:

return 'idb' in value.name

https://codereview.chromium.org/1238393003/diff/40001/tools/perf/page_sets/in...
File tools/perf/page_sets/indexeddb_endure_page.py (right):

https://codereview.chromium.org/1238393003/diff/40001/tools/perf/page_sets/in...
tools/perf/page_sets/indexeddb_endure_page.py:22:
action_runner.WaitForJavaScriptCondition('window.done == true', 600)
'window.done'

https://codereview.chromium.org/1238393003/diff/40001/tools/perf/page_sets/in...
tools/perf/page_sets/indexeddb_endure_page.py:25: """The IndexedDB Endurance
page set."""
Documentation doesn't really tell me anything.

https://codereview.chromium.org/1238393003/diff/40001/tools/perf/page_sets/in...
tools/perf/page_sets/indexeddb_endure_page.py:29:
self.AddStory(IndexedDBEndurePage('testCreateAndDeleteDatabases', self))
Nit: Since these lines are identical what do you think about:

tests = [
  'testCreateAndDeleteDatabases',
  'testCreateKeysInStores',
  ...
]
for test in tests:
  self.AddStory(IndexedDBEndurePage(test, self)

I don't know - more of a style type change, but something to consider.

https://codereview.chromium.org/1238393003/diff/40001/tools/perf/page_sets/in...
File tools/perf/page_sets/indexeddb_perf/endure/app.html (right):

https://codereview.chromium.org/1238393003/diff/40001/tools/perf/page_sets/in...
tools/perf/page_sets/indexeddb_perf/endure/app.html:8: <li>When "online", the
events are played back to a (fake) server,
So what's the plan for chrome/test/data/indexeddb/endure/app.html?

https://codereview.chromium.org/1238393003/diff/40001/tools/perf/page_sets/in...
File tools/perf/page_sets/indexeddb_perf/perf_test.js (right):

https://codereview.chromium.org/1238393003/diff/40001/tools/perf/page_sets/in...
tools/perf/page_sets/indexeddb_perf/perf_test.js:19: // Create 30 databases,
populate them with 10 object stores with 10 items
20 object stores right?

https://codereview.chromium.org/1238393003/diff/40001/tools/perf/page_sets/in...
tools/perf/page_sets/indexeddb_perf/perf_test.js:48:
[testRandomReadsAndWritesWithoutIndex, 1000,  5,    5,  50],
Will moving 1000, 5, 5, 50, w/o up here change the results?

https://codereview.chromium.org/1238393003/diff/40001/tools/perf/page_sets/in...
tools/perf/page_sets/indexeddb_perf/perf_test.js:82: [testReadCacheWithoutIndex,
50],
Previously 50 used the index, and 10 didn't. Was this switch intentional? If so
the results may not be consistent.

https://codereview.chromium.org/1238393003/diff/40001/tools/perf/page_sets/in...
tools/perf/page_sets/indexeddb_perf/perf_test.js:109: numTransactions, true,
onTestComplete)
Missing semicolon.

https://codereview.chromium.org/1238393003/diff/40001/tools/perf/page_sets/in...
tools/perf/page_sets/indexeddb_perf/perf_test.js:149:
console.time('Action_IDBTest')
;

https://codereview.chromium.org/1238393003/diff/40001/tools/perf/page_sets/in...
tools/perf/page_sets/indexeddb_perf/perf_test.js:155: var test, f;
This 'test' overrides the function above. Can you disambiguate?

https://codereview.chromium.org/1238393003/diff/40001/tools/perf/page_sets/in...
tools/perf/page_sets/indexeddb_perf/perf_test.js:178: done = true;
"done" never seems to be read anywhere?

https://codereview.chromium.org/1238393003/diff/40001/tools/telemetry/telemet...
File tools/telemetry/telemetry/web_perf/metrics/trace_event_aggregator.py
(right):

https://codereview.chromium.org/1238393003/diff/40001/tools/telemetry/telemet...
tools/telemetry/telemetry/web_perf/metrics/trace_event_aggregator.py:9: class
TraceEventAggregatorInput(object):
This class doesn't seem to do anything. Why not simply use a dictionary?

https://codereview.chromium.org/1238393003/diff/40001/tools/telemetry/telemet...
tools/telemetry/telemetry/web_perf/metrics/trace_event_aggregator.py:53: if
event.thread_duration:
Could you ever have a thread_duration of zero?

https://codereview.chromium.org/1238393003/diff/40001/tools/telemetry/telemet...
tools/telemetry/telemetry/web_perf/metrics/trace_event_aggregator.py:93: if
len(times) == 0:
Not pythonic. Just do:

if not times:
  continue

[dmurph, 2015-08-05 15:30:33]

ned:

I imagine this will be part of our sheriff group, not the perf sheriffs.  I'll
run it a bunch and give you some numbers.

https://codereview.chromium.org/1238393003/diff/40001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_backing_store.cc (right):

https://codereview.chromium.org/1238393003/diff/40001/content/browser/indexed...
content/browser/indexed_db/indexed_db_backing_store.cc:1479:
IDB_TRACE("IndexedDBBackingStore::DeleteDatabase::DeleteEntries");
On 2015/08/03 at 17:31:33, cmumford wrote:
> Nit: I believe that all other traces we use <class>::<method>, but
DeleteEntries isn't a method. What about a different syntax, like
IndexedDBBackingStore::DeleteDatabase.DeleteEntries,
IndexedDBBackingStore::DeleteDatabase=>DeleteEntries, or something do indicate
it is a part of a method instead of an entire method.
> 
> Just wanted to throw that out for discussion.

Sure, that sounds good. I'll switch to the '.' version.

https://codereview.chromium.org/1238393003/diff/40001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_transaction.cc (right):

https://codereview.chromium.org/1238393003/diff/40001/content/browser/indexed...
content/browser/indexed_db/indexed_db_transaction.cc:319:
IDB_TRACE1("IndexedDBTransaction:TransactionCompleteCallbacks:", "txn.id",
On 2015/08/03 at 17:31:33, cmumford wrote:
> colon in wrong place.

Done.

https://codereview.chromium.org/1238393003/diff/40001/content/browser/indexed...
File content/browser/indexed_db/leveldb/leveldb_database.cc (right):

https://codereview.chromium.org/1238393003/diff/40001/content/browser/indexed...
content/browser/indexed_db/leveldb/leveldb_database.cc:111:
IDB_TRACE("LevelDBDatabase::Open");
On 2015/08/03 at 17:31:33, cmumford wrote:
> What do you think about actually making this a static member function named
LevelDBDatabase::Open?

There already is one, I'll just move this trace there.

https://codereview.chromium.org/1238393003/diff/40001/content/browser/indexed...
content/browser/indexed_db/leveldb/leveldb_database.cc:429:
TRACE_EVENT0("LevelDB", "LevelDBDatabase::Compact");
On 2015/08/03 at 17:31:33, cmumford wrote:
> Why TRACE_EVENT instead of IDB_TRACE?

Whoops :)

https://codereview.chromium.org/1238393003/diff/40001/tools/perf/benchmarks/i...
File tools/perf/benchmarks/indexeddb_perf.py (right):

https://codereview.chromium.org/1238393003/diff/40001/tools/perf/benchmarks/i...
tools/perf/benchmarks/indexeddb_perf.py:61:
tab.WaitForJavaScriptExpression('window.done == true', 600)
On 2015/08/03 at 17:31:33, cmumford wrote:
> Nit: why not just 'window.done'?

ah, true.

https://codereview.chromium.org/1238393003/diff/40001/tools/perf/benchmarks/i...
tools/perf/benchmarks/indexeddb_perf.py:117: if 'idb' not in value.name:
On 2015/08/03 at 17:31:34, cmumford wrote:
> This logic makes me think way too much. Why not just:
> 
> return 'idb' in value.name

Done.

https://codereview.chromium.org/1238393003/diff/40001/tools/perf/page_sets/in...
File tools/perf/page_sets/indexeddb_endure_page.py (right):

https://codereview.chromium.org/1238393003/diff/40001/tools/perf/page_sets/in...
tools/perf/page_sets/indexeddb_endure_page.py:22:
action_runner.WaitForJavaScriptCondition('window.done == true', 600)
On 2015/08/03 at 17:31:34, cmumford wrote:
> 'window.done'

Done.

https://codereview.chromium.org/1238393003/diff/40001/tools/perf/page_sets/in...
tools/perf/page_sets/indexeddb_endure_page.py:25: """The IndexedDB Endurance
page set."""
On 2015/08/03 at 17:31:34, cmumford wrote:
> Documentation doesn't really tell me anything.

Done.

https://codereview.chromium.org/1238393003/diff/40001/tools/perf/page_sets/in...
tools/perf/page_sets/indexeddb_endure_page.py:29:
self.AddStory(IndexedDBEndurePage('testCreateAndDeleteDatabases', self))
On 2015/08/03 at 17:31:34, cmumford wrote:
> Nit: Since these lines are identical what do you think about:
> 
> tests = [
>   'testCreateAndDeleteDatabases',
>   'testCreateKeysInStores',
>   ...
> ]
> for test in tests:
>   self.AddStory(IndexedDBEndurePage(test, self)
> 
> I don't know - more of a style type change, but something to consider.

Nice, done.

https://codereview.chromium.org/1238393003/diff/40001/tools/perf/page_sets/in...
File tools/perf/page_sets/indexeddb_perf/endure/app.html (right):

https://codereview.chromium.org/1238393003/diff/40001/tools/perf/page_sets/in...
tools/perf/page_sets/indexeddb_perf/endure/app.html:8: <li>When "online", the
events are played back to a (fake) server,
On 2015/08/03 at 17:31:34, cmumford wrote:
> So what's the plan for chrome/test/data/indexeddb/endure/app.html?

Dunno. We could just remove it.  I'm not sure what else is depending on it.
WDYT?

https://codereview.chromium.org/1238393003/diff/40001/tools/perf/page_sets/in...
File tools/perf/page_sets/indexeddb_perf/perf_test.js (right):

https://codereview.chromium.org/1238393003/diff/40001/tools/perf/page_sets/in...
tools/perf/page_sets/indexeddb_perf/perf_test.js:48:
[testRandomReadsAndWritesWithoutIndex, 1000,  5,    5,  50],
On 2015/08/03 at 17:31:34, cmumford wrote:
> Will moving 1000, 5, 5, 50, w/o up here change the results?

Not sure. Hm..... you want to avoid differences for the current IDB test, right?

https://codereview.chromium.org/1238393003/diff/40001/tools/perf/page_sets/in...
tools/perf/page_sets/indexeddb_perf/perf_test.js:82: [testReadCacheWithoutIndex,
50],
On 2015/08/03 at 17:31:34, cmumford wrote:
> Previously 50 used the index, and 10 didn't. Was this switch intentional? If
so the results may not be consistent.

It was intentional here.  Previously we just did two 10's with no index, and
then to 50's with the index.  It looked like a mistake.  Same question as
before.  Should we fork these and have a pre-me-messing-around version for the
current perf test?

https://codereview.chromium.org/1238393003/diff/40001/tools/perf/page_sets/in...
tools/perf/page_sets/indexeddb_perf/perf_test.js:109: numTransactions, true,
onTestComplete)
On 2015/08/03 at 17:31:34, cmumford wrote:
> Missing semicolon.

done.

https://codereview.chromium.org/1238393003/diff/40001/tools/perf/page_sets/in...
tools/perf/page_sets/indexeddb_perf/perf_test.js:149:
console.time('Action_IDBTest')
On 2015/08/03 at 17:31:34, cmumford wrote:
> ;

Done.

https://codereview.chromium.org/1238393003/diff/40001/tools/perf/page_sets/in...
tools/perf/page_sets/indexeddb_perf/perf_test.js:155: var test, f;
On 2015/08/03 at 17:31:34, cmumford wrote:
> This 'test' overrides the function above. Can you disambiguate?

sure.

https://codereview.chromium.org/1238393003/diff/40001/tools/perf/page_sets/in...
tools/perf/page_sets/indexeddb_perf/perf_test.js:178: done = true;
On 2015/08/03 at 17:31:34, cmumford wrote:
> "done" never seems to be read anywhere?

It's read by the python to tell if the test is done.

https://codereview.chromium.org/1238393003/diff/40001/tools/telemetry/telemet...
File tools/telemetry/telemetry/web_perf/metrics/trace_event_aggregator.py
(right):

https://codereview.chromium.org/1238393003/diff/40001/tools/telemetry/telemet...
tools/telemetry/telemetry/web_perf/metrics/trace_event_aggregator.py:9: class
TraceEventAggregatorInput(object):
On 2015/08/03 at 17:31:34, cmumford wrote:
> This class doesn't seem to do anything. Why not simply use a dictionary?

I'm going to defer to you and Ned for this. It seems nice to have the
documentation, but maybe this is easier as a dictionary? I don't know python. 
How do I define a dictionary with the documentation?

https://codereview.chromium.org/1238393003/diff/40001/tools/telemetry/telemet...
tools/telemetry/telemetry/web_perf/metrics/trace_event_aggregator.py:34:
"""Reports durations and counts of given trace events.
On 2015/08/03 at 16:12:21, nednguyen wrote:
> This seems similar to single_event in web_perf/metrics. Though I like your
version better.

yay!  I tried to make it as performant as possible by filtering as much as it
possibly can.

https://codereview.chromium.org/1238393003/diff/40001/tools/telemetry/telemet...
tools/telemetry/telemetry/web_perf/metrics/trace_event_aggregator.py:53: if
event.thread_duration:
On 2015/08/03 at 17:31:34, cmumford wrote:
> Could you ever have a thread_duration of zero?

I imagine so.

https://codereview.chromium.org/1238393003/diff/40001/tools/telemetry/telemet...
tools/telemetry/telemetry/web_perf/metrics/trace_event_aggregator.py:93: if
len(times) == 0:
On 2015/08/03 at 17:31:34, cmumford wrote:
> Not pythonic. Just do:
> 
> if not times:
>   continue

Done.

[dmurph, 2015-08-05 16:14:16]

Chris & Ned:

I changed the benchmark class to have 3 benchmarks now:
 * The original, chrome/test/data/... test so that we keep those metrics around.
 * The same test as before, but using the new pageset, which splits up the tests
so we have less noisy results.
 * The new tracing test, which uses timeline metrics and the split up tests.

As for alerts/sheriffing:
We can specifically choose which values & page name are monitored, yes?  Some of
the values are super noise-less for every test (for example, the puts) because
we hit that operation a lot.  Other values are only relevant for a specific page
in the pageset (for example, database delete time).  If we can specify which
page-result combinations we care about, then this would be awesome.

Daniel

[nednguyen, 2015-08-06 04:13:14]

I think this is landable. Can you also add unittest for trace_event_aggregator?
Have few comments below:

https://codereview.chromium.org/1238393003/diff/110001/tools/telemetry/teleme...
File tools/telemetry/telemetry/web_perf/metrics/trace_event_aggregator.py
(right):

https://codereview.chromium.org/1238393003/diff/110001/tools/telemetry/teleme...
tools/telemetry/telemetry/web_perf/metrics/trace_event_aggregator.py:39:
self._inputs_by_process_name = dict()
self._inputs_by_process_name = collections.defaultdict(list)

https://codereview.chromium.org/1238393003/diff/110001/tools/telemetry/teleme...
tools/telemetry/telemetry/web_perf/metrics/trace_event_aggregator.py:47: if
input_obj.process_name not in self._inputs_by_process_name:
You don't need this line and the next one

https://codereview.chromium.org/1238393003/diff/110001/tools/telemetry/teleme...
tools/telemetry/telemetry/web_perf/metrics/trace_event_aggregator.py:63: None in
self._inputs_by_process_name):
How do we end up with having process with name =None in the
_inputs_by_process_name dict?

[dmurph, 2015-08-11 14:20:10]

Ned & Chris, there are a decent amount of changes here, can you take a look?

The one problem left is for the Browser process if we're doing content shell.  I
posted in the forums here:
https://groups.google.com/a/google.com/forum/#!topic/telemetry-team/8UooDdYDjBU

I'm trying to run these tests in content-shell, but there is no "Browser"
process, just a "process ###" that has all of the traces I need.

I also tried to make this more efficient for tests w/o matching events.  Let me
know if there is anything else I can change to make it more efficient.

https://codereview.chromium.org/1238393003/diff/110001/tools/telemetry/teleme...
File tools/telemetry/telemetry/web_perf/metrics/trace_event_aggregator.py
(right):

https://codereview.chromium.org/1238393003/diff/110001/tools/telemetry/teleme...
tools/telemetry/telemetry/web_perf/metrics/trace_event_aggregator.py:39:
self._inputs_by_process_name = dict()
On 2015/08/06 at 04:13:14, nednguyen wrote:
> self._inputs_by_process_name = collections.defaultdict(list)

Done.

https://codereview.chromium.org/1238393003/diff/110001/tools/telemetry/teleme...
tools/telemetry/telemetry/web_perf/metrics/trace_event_aggregator.py:47: if
input_obj.process_name not in self._inputs_by_process_name:
On 2015/08/06 at 04:13:14, nednguyen wrote:
> You don't need this line and the next one

Done.

https://codereview.chromium.org/1238393003/diff/110001/tools/telemetry/teleme...
tools/telemetry/telemetry/web_perf/metrics/trace_event_aggregator.py:63: None in
self._inputs_by_process_name):
On 2015/08/06 at 04:13:14, nednguyen wrote:
> How do we end up with having process with name =None in the
_inputs_by_process_name dict?

I changed the behavior to no longer accept 'None'. There is a possibility of
using this to look for events in all processes, but the architecture would have
to change a bit, as that would currently result in having multiple of the
'count' values recorded, one per process (which messes up the test results
class, and I'm imagining wouldn't be expected anyways).

[nednguyen, 2015-08-13 17:39:06]

https://codereview.chromium.org/1238393003/diff/130001/tools/telemetry/teleme...
File tools/telemetry/telemetry/web_perf/metrics/blob_timeline.py (right):

https://codereview.chromium.org/1238393003/diff/130001/tools/telemetry/teleme...
tools/telemetry/telemetry/web_perf/metrics/blob_timeline.py:48: for event in
model.IterAllEvents(
Hmm, this change should be a different cl.

https://codereview.chromium.org/1238393003/diff/130001/tools/telemetry/teleme...
File tools/telemetry/telemetry/web_perf/metrics/trace_event_aggregator.py
(right):

https://codereview.chromium.org/1238393003/diff/130001/tools/telemetry/teleme...
tools/telemetry/telemetry/web_perf/metrics/trace_event_aggregator.py:9: 
nits: 2 blank lines

https://codereview.chromium.org/1238393003/diff/130001/tools/telemetry/teleme...
tools/telemetry/telemetry/web_perf/metrics/trace_event_aggregator.py:14:
"""Input for the TraceEventAggregator.
nits: Add

Args: 
  event_category:...

https://codereview.chromium.org/1238393003/diff/130001/tools/telemetry/teleme...
tools/telemetry/telemetry/web_perf/metrics/trace_event_aggregator.py:20:
list_metric_description: Description of the list metric.
If you follow the suggestion below, we can simplify this name to "metric_name" &
"metric_description"

https://codereview.chromium.org/1238393003/diff/130001/tools/telemetry/teleme...
tools/telemetry/telemetry/web_perf/metrics/trace_event_aggregator.py:22:
count_metric_name: The name of the metric that is the count of the events.
count_metric_name & count_metric_description can be inferred from
list_metric_name & list_metric_description?

https://codereview.chromium.org/1238393003/diff/130001/tools/telemetry/teleme...
tools/telemetry/telemetry/web_perf/metrics/trace_event_aggregator.py:42: class
TraceEventAggregator(object):
Let call this TraceEventsStats

https://codereview.chromium.org/1238393003/diff/130001/tools/telemetry/teleme...
tools/telemetry/telemetry/web_perf/metrics/trace_event_aggregator.py:54: for
input_obj in input_list:
we should blow up if there are duplicate metric names.

https://codereview.chromium.org/1238393003/diff/130001/tools/telemetry/teleme...
File
tools/telemetry/telemetry/web_perf/metrics/trace_event_aggregator_unittest.py
(right):

https://codereview.chromium.org/1238393003/diff/130001/tools/telemetry/teleme...
tools/telemetry/telemetry/web_perf/metrics/trace_event_aggregator_unittest.py:12:
from telemetry.web_perf.metrics.trace_event_aggregator import
TraceEventAggregator
We deal with long import line by:

from z.zafsfa.safasf import (
    foo)

https://codereview.chromium.org/1238393003/diff/130001/tools/telemetry/teleme...
tools/telemetry/telemetry/web_perf/metrics/trace_event_aggregator_unittest.py:28:

nits: 2 blank lines

https://codereview.chromium.org/1238393003/diff/130001/tools/telemetry/teleme...
tools/telemetry/telemetry/web_perf/metrics/trace_event_aggregator_unittest.py:59:

extra blank line

[dmurph, 2015-08-14 12:33:32]

I moved the blob stuff out.

https://codereview.chromium.org/1238393003/diff/130001/tools/telemetry/teleme...
File tools/telemetry/telemetry/web_perf/metrics/blob_timeline.py (right):

https://codereview.chromium.org/1238393003/diff/130001/tools/telemetry/teleme...
tools/telemetry/telemetry/web_perf/metrics/blob_timeline.py:48: for event in
model.IterAllEvents(
On 2015/08/13 at 17:39:06, nednguyen wrote:
> Hmm, this change should be a different cl.

ok, removed.

https://codereview.chromium.org/1238393003/diff/130001/tools/telemetry/teleme...
File tools/telemetry/telemetry/web_perf/metrics/trace_event_aggregator.py
(right):

https://codereview.chromium.org/1238393003/diff/130001/tools/telemetry/teleme...
tools/telemetry/telemetry/web_perf/metrics/trace_event_aggregator.py:9: 
On 2015/08/13 at 17:39:06, nednguyen wrote:
> nits: 2 blank lines

Done.

https://codereview.chromium.org/1238393003/diff/130001/tools/telemetry/teleme...
tools/telemetry/telemetry/web_perf/metrics/trace_event_aggregator.py:9: 
On 2015/08/13 at 17:39:06, nednguyen wrote:
> nits: 2 blank lines

Done.

https://codereview.chromium.org/1238393003/diff/130001/tools/telemetry/teleme...
tools/telemetry/telemetry/web_perf/metrics/trace_event_aggregator.py:14:
"""Input for the TraceEventAggregator.
On 2015/08/13 at 17:39:06, nednguyen wrote:
> nits: Add
> 
> Args: 
>   event_category:...

Done.

https://codereview.chromium.org/1238393003/diff/130001/tools/telemetry/teleme...
tools/telemetry/telemetry/web_perf/metrics/trace_event_aggregator.py:20:
list_metric_description: Description of the list metric.
On 2015/08/13 at 17:39:06, nednguyen wrote:
> If you follow the suggestion below, we can simplify this name to "metric_name"
& "metric_description"

Done.

https://codereview.chromium.org/1238393003/diff/130001/tools/telemetry/teleme...
tools/telemetry/telemetry/web_perf/metrics/trace_event_aggregator.py:22:
count_metric_name: The name of the metric that is the count of the events.
On 2015/08/13 at 17:39:06, nednguyen wrote:
> count_metric_name & count_metric_description can be inferred from
list_metric_name & list_metric_description?

Done.

https://codereview.chromium.org/1238393003/diff/130001/tools/telemetry/teleme...
tools/telemetry/telemetry/web_perf/metrics/trace_event_aggregator.py:42: class
TraceEventAggregator(object):
On 2015/08/13 at 17:39:06, nednguyen wrote:
> Let call this TraceEventsStats

Done.

https://codereview.chromium.org/1238393003/diff/130001/tools/telemetry/teleme...
tools/telemetry/telemetry/web_perf/metrics/trace_event_aggregator.py:54: for
input_obj in input_list:
On 2015/08/13 at 17:39:06, nednguyen wrote:
> we should blow up if there are duplicate metric names.

Done.

https://codereview.chromium.org/1238393003/diff/130001/tools/telemetry/teleme...
File
tools/telemetry/telemetry/web_perf/metrics/trace_event_aggregator_unittest.py
(right):

https://codereview.chromium.org/1238393003/diff/130001/tools/telemetry/teleme...
tools/telemetry/telemetry/web_perf/metrics/trace_event_aggregator_unittest.py:12:
from telemetry.web_perf.metrics.trace_event_aggregator import
TraceEventAggregator
On 2015/08/13 at 17:39:06, nednguyen wrote:
> We deal with long import line by:
> 
> from z.zafsfa.safasf import (
>     foo)

No longer needed thankfully.

https://codereview.chromium.org/1238393003/diff/130001/tools/telemetry/teleme...
tools/telemetry/telemetry/web_perf/metrics/trace_event_aggregator_unittest.py:28:

On 2015/08/13 at 17:39:06, nednguyen wrote:
> nits: 2 blank lines

Done.

https://codereview.chromium.org/1238393003/diff/130001/tools/telemetry/teleme...
tools/telemetry/telemetry/web_perf/metrics/trace_event_aggregator_unittest.py:59:

On 2015/08/13 at 17:39:06, nednguyen wrote:
> extra blank line

Done.

[nednguyen, 2015-08-14 15:25:45]

lgtm on telemetry side with some nits

https://codereview.chromium.org/1238393003/diff/150001/tools/telemetry/teleme...
File tools/telemetry/telemetry/web_perf/metrics/indexeddb_timeline.py (right):

https://codereview.chromium.org/1238393003/diff/150001/tools/telemetry/teleme...
tools/telemetry/telemetry/web_perf/metrics/indexeddb_timeline.py:17:
self._aggregator = TraceEventStats()
nit: self._stats =

https://codereview.chromium.org/1238393003/diff/150001/tools/telemetry/teleme...
File tools/telemetry/telemetry/web_perf/metrics/trace_event_stats.py (right):

https://codereview.chromium.org/1238393003/diff/150001/tools/telemetry/teleme...
tools/telemetry/telemetry/web_perf/metrics/trace_event_stats.py:13:
EVENT_ID_DELIMITER = '^SERIALIZE-DELIM^'
This public variable can make it seem confusing at first. You can just make:

def GetEventId(event_category, event_name):
""" Return a unique id for the pair of event_category & event_name strings."""
  return event_category + '^SERIALIZE-DELIM^' + event_name

https://codereview.chromium.org/1238393003/diff/150001/tools/telemetry/teleme...
tools/telemetry/telemetry/web_perf/metrics/trace_event_stats.py:37:
self.event_id = event_category + self.EVENT_ID_DELIMITER + event_name
self.event_id = TraceEventStatsInput.GetEventId(event_...)

https://codereview.chromium.org/1238393003/diff/150001/tools/telemetry/teleme...
tools/telemetry/telemetry/web_perf/metrics/trace_event_stats.py:72: def
GetEventID(event):
This method should be in TraceEventStats. You can make it a @staticmethod

https://codereview.chromium.org/1238393003/diff/150001/tools/telemetry/teleme...
File tools/telemetry/telemetry/web_perf/metrics/trace_event_stats_unittest.py
(right):

https://codereview.chromium.org/1238393003/diff/150001/tools/telemetry/teleme...
tools/telemetry/telemetry/web_perf/metrics/trace_event_stats_unittest.py:87: 
Can you add a test with empty trace model & asserting that results contain no
values?

[dmurph, 2015-08-26 17:52:25]

Done.

Chris, can you take a look?  I added a bug.

Thanks,
Daniel.

https://codereview.chromium.org/1238393003/diff/150001/tools/telemetry/teleme...
File tools/telemetry/telemetry/web_perf/metrics/indexeddb_timeline.py (right):

https://codereview.chromium.org/1238393003/diff/150001/tools/telemetry/teleme...
tools/telemetry/telemetry/web_perf/metrics/indexeddb_timeline.py:17:
self._aggregator = TraceEventStats()
On 2015/08/14 at 15:25:45, nednguyen wrote:
> nit: self._stats =

Done.

https://codereview.chromium.org/1238393003/diff/150001/tools/telemetry/teleme...
File tools/telemetry/telemetry/web_perf/metrics/trace_event_stats.py (right):

https://codereview.chromium.org/1238393003/diff/150001/tools/telemetry/teleme...
tools/telemetry/telemetry/web_perf/metrics/trace_event_stats.py:13:
EVENT_ID_DELIMITER = '^SERIALIZE-DELIM^'
On 2015/08/14 at 15:25:45, nednguyen wrote:
> This public variable can make it seem confusing at first. You can just make:
> 
> def GetEventId(event_category, event_name):
> """ Return a unique id for the pair of event_category & event_name strings."""
>   return event_category + '^SERIALIZE-DELIM^' + event_name

Done

https://codereview.chromium.org/1238393003/diff/150001/tools/telemetry/teleme...
tools/telemetry/telemetry/web_perf/metrics/trace_event_stats.py:37:
self.event_id = event_category + self.EVENT_ID_DELIMITER + event_name
On 2015/08/14 at 15:25:45, nednguyen wrote:
> self.event_id = TraceEventStatsInput.GetEventId(event_...)

Done.

https://codereview.chromium.org/1238393003/diff/150001/tools/telemetry/teleme...
tools/telemetry/telemetry/web_perf/metrics/trace_event_stats.py:72: def
GetEventID(event):
On 2015/08/14 at 15:25:45, nednguyen wrote:
> This method should be in TraceEventStats. You can make it a @staticmethod

Done.

https://codereview.chromium.org/1238393003/diff/150001/tools/telemetry/teleme...
File tools/telemetry/telemetry/web_perf/metrics/trace_event_stats_unittest.py
(right):

https://codereview.chromium.org/1238393003/diff/150001/tools/telemetry/teleme...
tools/telemetry/telemetry/web_perf/metrics/trace_event_stats_unittest.py:87: 
On 2015/08/14 at 15:25:45, nednguyen wrote:
> Can you add a test with empty trace model & asserting that results contain no
values?

Done.

[nednguyen, 2015-08-26 17:58:43]

lgtm again

https://codereview.chromium.org/1238393003/diff/170001/tools/telemetry/teleme...
File tools/telemetry/telemetry/web_perf/metrics/trace_event_stats.py (right):

https://codereview.chromium.org/1238393003/diff/170001/tools/telemetry/teleme...
tools/telemetry/telemetry/web_perf/metrics/trace_event_stats.py:72: def
GetEventID(event):
You don't need this method. Call sites can just use
TraceEventStatsInput.GetEventID directly.

[cmumford, 2015-08-26 19:06:19]

https://codereview.chromium.org/1238393003/diff/170001/content/browser/indexe...
File content/browser/indexed_db/indexed_db_backing_store.cc (right):

https://codereview.chromium.org/1238393003/diff/170001/content/browser/indexe...
content/browser/indexed_db/indexed_db_backing_store.cc:4350: this);
You begin with "this", but end with "transaction_". Is that OK?

[dmurph, 2015-08-26 20:24:55]

https://codereview.chromium.org/1238393003/diff/170001/content/browser/indexe...
File content/browser/indexed_db/indexed_db_backing_store.cc (right):

https://codereview.chromium.org/1238393003/diff/170001/content/browser/indexe...
content/browser/indexed_db/indexed_db_backing_store.cc:4350: this);
On 2015/08/26 at 19:06:18, cmumford wrote:
> You begin with "this", but end with "transaction_". Is that OK?

Yes, the BEGIN is in the transaction object, and the END is in the callback
wrapper, and the 'transaction_' variable is the first transaction object.

https://codereview.chromium.org/1238393003/diff/170001/tools/telemetry/teleme...
File tools/telemetry/telemetry/web_perf/metrics/trace_event_stats.py (right):

https://codereview.chromium.org/1238393003/diff/170001/tools/telemetry/teleme...
tools/telemetry/telemetry/web_perf/metrics/trace_event_stats.py:72: def
GetEventID(event):
On 2015/08/26 at 17:58:43, nednguyen wrote:
> You don't need this method. Call sites can just use
TraceEventStatsInput.GetEventID directly.

Done.

[cmumford, 2015-08-26 20:58:02]

lgtm

[dmurph, 2015-08-26 22:04:29]

The CQ bit was checked by dmurph@chromium.org

[dmurph, 2015-08-26 22:04:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
nednguyen@google.com, cmumford@chromium.org
Link to the patchset: https://codereview.chromium.org/1238393003/#ps210001
(title: "Cleanup")

[commit-bot: I haz the power, 2015-08-26 22:04:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1238393003/210001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1238393003/210001

[commit-bot: I haz the power, 2015-08-26 22:21:27]

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

[commit-bot: I haz the power, 2015-08-26 22:21:28]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[dmurph, 2015-09-08 22:13:02]

The CQ bit was checked by dmurph@chromium.org

[dmurph, 2015-09-08 22:13:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
nednguyen@google.com, cmumford@chromium.org
Link to the patchset: https://codereview.chromium.org/1238393003/#ps230001
(title: "test")

[commit-bot: I haz the power, 2015-09-08 22:13:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1238393003/230001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1238393003/230001

[commit-bot: I haz the power, 2015-09-08 22:54:22]

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

[commit-bot: I haz the power, 2015-09-08 22:54:22]

Try jobs failed on following builders:
  android_nexus5_perf_bisect on tryserver.chromium.perf (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.perf/builders/android_nexus5_p...)

[nednguyen, 2015-09-16 15:27:21]

The CQ bit was checked by nednguyen@google.com

[commit-bot: I haz the power, 2015-09-16 15:27:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1238393003/230001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1238393003/230001

[commit-bot: I haz the power, 2015-09-16 15:29:30]

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

[commit-bot: I haz the power, 2015-09-16 15:29:31]

Try jobs failed on following builders:
  mac_perf_bisect on tryserver.chromium.perf (JOB_FAILED, no build URL)

[nednguyen, 2015-09-16 15:29:47]

The CQ bit was checked by nednguyen@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-09-16 15:30:19]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1238393003/230001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1238393003/230001

[commit-bot: I haz the power, 2015-09-16 15:32:14]

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

[commit-bot: I haz the power, 2015-09-16 15:32:15]

Dry run: Try jobs failed on following builders:
  mac_perf_bisect on tryserver.chromium.perf (JOB_FAILED, no build URL)

[dmurph, 2015-09-18 18:43:54]

The CQ bit was checked by dmurph@chromium.org

[dmurph, 2015-09-18 18:43:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
nednguyen@google.com, cmumford@chromium.org
Link to the patchset: https://codereview.chromium.org/1238393003/#ps250001
(title: "forgot to start the old test")

[commit-bot: I haz the power, 2015-09-18 18:44:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1238393003/250001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1238393003/250001

[commit-bot: I haz the power, 2015-09-18 18:48:06]

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

[commit-bot: I haz the power, 2015-09-18 18:48:07]

Try jobs failed on following builders:
  mac_perf_bisect on tryserver.chromium.perf (JOB_FAILED, no build URL)

[dmurph, 2015-09-19 00:42:29]

The CQ bit was checked by dmurph@chromium.org

[commit-bot: I haz the power, 2015-09-19 00:43:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1238393003/250001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1238393003/250001

[commit-bot: I haz the power, 2015-09-19 05:25:34]

Committed patchset #14 (id:250001)

[commit-bot: I haz the power, 2015-09-19 05:26:14]

Patchset 14 (id:??) landed as
https://crrev.com/3723846d9bf63d802a8cae98fbff565a7b7dc526
Cr-Commit-Position: refs/heads/master@{#349853}