Add searching features to new TraceEventVector class for tests.

Add searching features to new TraceEventVector class for tests.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=114504

[jbates, 2011-12-09 22:41:51]

PTAL.

The new features are used by a new input latency test:
http://codereview.chromium.org/8883005/

[Paweł Hajdan Jr., 2011-12-12 17:21:51]

Please also add someone more familiar with trace events to the reviewers list.

http://codereview.chromium.org/8900001/diff/1/base/test/trace_event_analyzer.cc
File base/test/trace_event_analyzer.cc (right):

http://codereview.chromium.org/8900001/diff/1/base/test/trace_event_analyzer....
base/test/trace_event_analyzer.cc:682: stats->standard_deviation_us =
Isn't this standard deviation code being duplicated? If so, please extract the
common parts.

http://codereview.chromium.org/8900001/diff/1/base/test/trace_event_analyzer....
base/test/trace_event_analyzer.cc:706: for (;;) {
nit: Why not "while (true)"? It's generally more descriptive.

http://codereview.chromium.org/8900001/diff/1/base/test/trace_event_analyzer.h
File base/test/trace_event_analyzer.h (right):

http://codereview.chromium.org/8900001/diff/1/base/test/trace_event_analyzer....
base/test/trace_event_analyzer.h:418: class TraceEventVector : public
std::vector<const TraceEvent*> {
Inheritance from std::vector? I'm suspicious. Could you explain more why you're
doing that?

http://codereview.chromium.org/8900001/diff/1/base/test/trace_event_analyzer....
base/test/trace_event_analyzer.h:451: size_t* return_second_closest) const;
nit: If second_closest is there, why not third_closest and so on? Would it help
to have a method like that to only return the first value, and then another one
to return a sorted vector?

http://codereview.chromium.org/8900001/diff/1/base/test/trace_event_analyzer....
base/test/trace_event_analyzer.h:454: int CountMatches(Query query,
nit: If size_t is generally used, why this and method below return int?

[jbates, 2011-12-12 22:00:30]

(sorry pawel - mistyped your name in the cl upload comment!)

Adding zmo for functional review, since he has used the API before.

http://codereview.chromium.org/8900001/diff/1/base/test/trace_event_analyzer.cc
File base/test/trace_event_analyzer.cc (right):

http://codereview.chromium.org/8900001/diff/1/base/test/trace_event_analyzer....
base/test/trace_event_analyzer.cc:682: stats->standard_deviation_us =
On 2011/12/12 17:21:51, Paweł Hajdan Jr. wrote:
> Isn't this standard deviation code being duplicated? If so, please extract the
> common parts.

This is just a code move, so no duplication.

http://codereview.chromium.org/8900001/diff/1/base/test/trace_event_analyzer....
base/test/trace_event_analyzer.cc:706: for (;;) {
On 2011/12/12 17:21:51, Paweł Hajdan Jr. wrote:
> nit: Why not "while (true)"? It's generally more descriptive.

Grepping base/ for 'for (;;)' turns up slightly more occurrences than 'while
(true)' although both are used throughout our code. I guess this is left to
personal preference. I've read that MSVC can generate better code with for (;;).
It also is possible to get compiler warnings with while (true) (see
http://msdn.microsoft.com/en-us/library/6t66728h(v=vs.80).aspx), but I guess we
disable that warning.

http://codereview.chromium.org/8900001/diff/1/base/test/trace_event_analyzer.h
File base/test/trace_event_analyzer.h (right):

http://codereview.chromium.org/8900001/diff/1/base/test/trace_event_analyzer....
base/test/trace_event_analyzer.h:418: class TraceEventVector : public
std::vector<const TraceEvent*> {
On 2011/12/12 17:21:51, Paweł Hajdan Jr. wrote:
> Inheritance from std::vector? I'm suspicious. Could you explain more why
you're
> doing that?

Done.

http://codereview.chromium.org/8900001/diff/1/base/test/trace_event_analyzer....
base/test/trace_event_analyzer.h:451: size_t* return_second_closest) const;
On 2011/12/12 17:21:51, Paweł Hajdan Jr. wrote:
> nit: If second_closest is there, why not third_closest and so on? Would it
help
> to have a method like that to only return the first value, and then another
one
> to return a sorted vector?

Returning a sorted vector did occur to me, but one or two satisfies all the use
cases I can think of. Basically, the second one is just to find out if the event
was very close to the closest, or if it was closer to the mid-point between two
events. Going from 2 to n adds some complexity, so I didn't think it was worth
it without some use cases.

http://codereview.chromium.org/8900001/diff/1/base/test/trace_event_analyzer....
base/test/trace_event_analyzer.h:454: int CountMatches(Query query,
On 2011/12/12 17:21:51, Paweł Hajdan Jr. wrote:
> nit: If size_t is generally used, why this and method below return int?

Done.

[Zhenyao Mo, 2011-12-12 22:30:07]

LGTM with a few nits.

[Zhenyao Mo, 2011-12-12 22:30:13]

http://codereview.chromium.org/8900001/diff/6001/base/test/trace_event_analyz...
File base/test/trace_event_analyzer.cc (right):

http://codereview.chromium.org/8900001/diff/6001/base/test/trace_event_analyz...
base/test/trace_event_analyzer.cc:791: // Need at least 3 events to calculate
rate stats.
CHECK(stats);

http://codereview.chromium.org/8900001/diff/6001/base/test/trace_event_analyz...
base/test/trace_event_analyzer.cc:831: *return_index = i;
You should CHECK(return_index) before using it.

http://codereview.chromium.org/8900001/diff/6001/base/test/trace_event_analyz...
base/test/trace_event_analyzer.cc:847: *return_index = position;
CHECK(return_index)

http://codereview.chromium.org/8900001/diff/6001/base/test/trace_event_analyz...
base/test/trace_event_analyzer.cc:884: *return_closest = closest;
CHECK(return_closest);

http://codereview.chromium.org/8900001/diff/6001/base/test/trace_event_analyz...
File base/test/trace_event_analyzer_unittest.cc (right):

http://codereview.chromium.org/8900001/diff/6001/base/test/trace_event_analyz...
base/test/trace_event_analyzer_unittest.cc:695: &index_1, &index_2));
format: too many whitespaces here.

http://codereview.chromium.org/8900001/diff/6001/base/test/trace_event_analyz...
base/test/trace_event_analyzer_unittest.cc:731: // Test FindClosest.
FindClosest -> CountMatches

[jbates, 2011-12-12 22:54:41]

http://codereview.chromium.org/8900001/diff/6001/base/test/trace_event_analyz...
File base/test/trace_event_analyzer.cc (right):

http://codereview.chromium.org/8900001/diff/6001/base/test/trace_event_analyz...
base/test/trace_event_analyzer.cc:791: // Need at least 3 events to calculate
rate stats.
On 2011/12/12 22:30:13, Zhenyao Mo wrote:
> CHECK(stats);

Done.

http://codereview.chromium.org/8900001/diff/6001/base/test/trace_event_analyz...
base/test/trace_event_analyzer.cc:831: *return_index = i;
On 2011/12/12 22:30:13, Zhenyao Mo wrote:
> You should CHECK(return_index) before using it.

Done.

http://codereview.chromium.org/8900001/diff/6001/base/test/trace_event_analyz...
base/test/trace_event_analyzer.cc:847: *return_index = position;
On 2011/12/12 22:30:13, Zhenyao Mo wrote:
> CHECK(return_index)

Done.

http://codereview.chromium.org/8900001/diff/6001/base/test/trace_event_analyz...
base/test/trace_event_analyzer.cc:884: *return_closest = closest;
On 2011/12/12 22:30:13, Zhenyao Mo wrote:
> CHECK(return_closest);

Done.

http://codereview.chromium.org/8900001/diff/6001/base/test/trace_event_analyz...
File base/test/trace_event_analyzer_unittest.cc (right):

http://codereview.chromium.org/8900001/diff/6001/base/test/trace_event_analyz...
base/test/trace_event_analyzer_unittest.cc:695: &index_1, &index_2));
On 2011/12/12 22:30:13, Zhenyao Mo wrote:
> format: too many whitespaces here.

Done.

http://codereview.chromium.org/8900001/diff/6001/base/test/trace_event_analyz...
base/test/trace_event_analyzer_unittest.cc:731: // Test FindClosest.
On 2011/12/12 22:30:13, Zhenyao Mo wrote:
> FindClosest -> CountMatches

Done.

[jbates, 2011-12-13 20:42:44]

pawel: ptal

[Paweł Hajdan Jr., 2011-12-14 09:03:09]

LGTM (I consider Zhenyao's review as primary, and only took a brief look).