Issue 7981004: add classes trace_analyzer::Query and TraceAnalyzer to make it easy to search through trace data

jbates

Split 1/3 from http://codereview.chromium.org/7866026/

9 years, 3 months ago (2011-09-20 17:40:15 UTC) #1

jbates

On 2011/09/20 17:40:15, jbates wrote: > Split 1/3 from http://codereview.chromium.org/7866026/ A few updates in the ...

9 years, 3 months ago (2011-09-21 19:22:25 UTC) #2

scheib

I've skimmed over, looks great! This will be a nice and expressive way to specify ...

9 years, 2 months ago (2011-09-29 23:33:56 UTC) #4

jbates

Thanks! I can't wait to write some tests with it... http://codereview.chromium.org/7981004/diff/1009/base/debug/trace_event.h File base/debug/trace_event.h (right): http://codereview.chromium.org/7981004/diff/1009/base/debug/trace_event.h#newcode462 ...

9 years, 2 months ago (2011-09-30 21:07:37 UTC) #5

jbates

On 2011/09/30 21:07:37, jbates wrote: > Thanks! I can't wait to write some tests with ...

9 years, 2 months ago (2011-10-05 17:13:23 UTC) #6

scheib

:| I'm in a rush for M16, and would need to hold off till at ...

9 years, 2 months ago (2011-10-05 17:18:47 UTC) #7

nduca

I like the query approach, but am bothered by the hard edges around relationships, repeated ...

9 years, 2 months ago (2011-10-11 20:33:37 UTC) #8

jbates

Thanks for the detailed and thorough feedback, Nat. I've updated the documentation and code. I ...

9 years, 2 months ago (2011-10-12 22:35:19 UTC) #9

Thanks for the detailed and thorough feedback, Nat. I've updated the
documentation and code.

I think this is ready for commit. Once we get some tests written, we can revisit
some of the more advanced use cases and add or hide APIs as necessary.

http://codereview.chromium.org/7981004/diff/10001/base/debug/trace_event.cc
File base/debug/trace_event.cc (right):

http://codereview.chromium.org/7981004/diff/10001/base/debug/trace_event.cc#n...
base/debug/trace_event.cc:226: return TRACE_EVENT_PHASE_METADATA;
On 2011/10/11 20:33:37, nduca wrote:
> is this right? 'X'->metdata? X->UNKNOWN and DCHECK(false)??

Done.

http://codereview.chromium.org/7981004/diff/10001/base/debug/trace_event_test...
File base/debug/trace_event_test_utils.h (right):

http://codereview.chromium.org/7981004/diff/10001/base/debug/trace_event_test...
base/debug/trace_event_test_utils.h:1: // Copyright (c) 2011 The Chromium
Authors. All rights reserved.
On 2011/10/11 20:33:37, nduca wrote:
> trace_event_analyzer.h
> 
> move tests of the analyzer into trace_event_analyzer_test.cc

Done.

http://codereview.chromium.org/7981004/diff/10001/base/debug/trace_event_test...
base/debug/trace_event_test_utils.h:5: // Use trace::Query with TraceAnalyzer to
search for specific trace events.
On 2011/10/11 20:33:37, nduca wrote:
> Advertise the namespace that these are all in? I had to go down to code to
> understand that this was in base::trace (a casual trace_event user never sees
> the namespace stuff).

Done.

http://codereview.chromium.org/7981004/diff/10001/base/debug/trace_event_test...
base/debug/trace_event_test_utils.h:9: // TraceAnalyzer analyzer(json_events);
On 2011/10/11 20:33:37, nduca wrote:
> What happens if it fails to parse? Maybe a TraceAnalyzer*
> TraceAnalyzer::create(json_events) would be better?

This should be okay, because the parse would only fail if there's a bug in
trace_event or trace_controller, which will be extremely rare. In the event that
it does happen, the test will fail because there will be no trace events.

http://codereview.chromium.org/7981004/diff/10001/base/debug/trace_event_test...
base/debug/trace_event_test_utils.h:10: // ConstEventPtrVector events;
On 2011/10/11 20:33:37, nduca wrote:
> A blurb explaining the basic idea of trace analysis would be pretty cool, and
> might help shake out some rough edges here... "You analyze traces by parsing
the
> events, then issuing queries against it. Queries are a tree of blahblahblah.
> Events come out as TestTraceEvent objects. Relationships are automatically
> resolved and accessible via other_event."

Done.

http://codereview.chromium.org/7981004/diff/10001/base/debug/trace_event_test...
base/debug/trace_event_test_utils.h:19: // If the test needs to analyze events
that begin and end on different threads,
On 2011/10/11 20:33:37, nduca wrote:
> Recapping our verbal discussion, I think should take this opportunity to fix
B/E
> events and make very precise semantic rules about B-E pairing. THis may clean
up
> this code a bit.

Sounds good. On second look though, the trace_event change can be done
independently since it doesn't change this code. I fixed the example below to
make this compatible with the current assumptions about BEGIN/END events.

http://codereview.chromium.org/7981004/diff/10001/base/debug/trace_event_test...
base/debug/trace_event_test_utils.h:28: //   TRACE_EVENT_END1("test_latency",
"timing1", "id", 3);
On 2011/10/11 20:33:37, nduca wrote:
> This trace wont' even parse in the trace_event_viewer? More reason to fix B/E
> inconsistency.

Fixed the example to use TRACE_EVENT_INSTANT, which is compatible with both the
current about:tracing assumptions about BEGIN/END and the future support for
START/FINISH events.

http://codereview.chromium.org/7981004/diff/10001/base/debug/trace_event_test...
base/debug/trace_event_test_utils.h:34: //   analyzer.AssociateEvents(begin,
end, match);
On 2011/10/11 20:33:37, nduca wrote:
> Based on the first example, I was hoping we'd see call "FindEvents" again, 

You're right, it should have been there... Done.

but
> maybe with some additional arguments. It wasn't clera to me that FindEvents
> associated events, and that AssociateEvents did to but using a custom
operator.

Actually, it's TraceAnalyzer::SetEvents (called during construction) that sets
default associations. I updated the comments to make that more clear.
> 
> Maybe FindEvents(Query)
>       FindEvents(begin, end, matcher)

AssociateEvents can be called multiple times to setup multiple associations.
ClearAssociations resets them. This might not currently be useful, but it's
flexible to future possibilities such as having an event associated with more
than one other event.

> 
> When we fix B/E events (e.g. moving B/E on other threads to be S/F events) do
we
> have use cases for FindEvents with custom matchers?

Probably not, but I'm not sure. I need to implement the input latency test to
get a better idea of what is needed. Since inputs are coalesced, it's going to
be challenging to have a single begin/end pair that represents oldest input and
corresponding DoDeferredUpdate.

http://codereview.chromium.org/7981004/diff/10001/base/debug/trace_event_test...
base/debug/trace_event_test_utils.h:57: struct TestTraceEvent {
On 2011/10/11 20:33:37, nduca wrote:
> I'm confused about base::trace vs base::debug::trace vs base::trace. Can we
> clean this up?
> 
> The name here is hard. I understand that you need an event that is part of the
> analyzer...
> 
> Whatabout
> base::trace::analysis::Query
> base::trace::analysis::Event
> base::trace::analysis::TraceAnalyzer

I've been thinking it would be great to move tracing stuff from base::debug to
base or base::trace since it's no longer debug-only. That would be a great place
for all of trace_event.h and trace_event_analysis.h. I think that's a separate
CL though. This header follows the trace_event.h namespace conventions except
for the Query class, which gets its own namespace for syntactic sugar. I'm open
to changing namespaces up, but I'd prefer to do it in conjunction with
trace_event.h. WDYT?

http://codereview.chromium.org/7981004/diff/10001/base/debug/trace_event_test...
base/debug/trace_event_test_utils.h:77: static bool ParseEventsFromJson(const
std::string& json,
On 2011/10/11 20:33:37, nduca wrote:
> Makes more sense on traceanalyzer?

This is only dependent on TestTraceEvent, so I think it's best to keep it here
to reduce coupling. It also matches the static TraceEvent::AppendEventsAsJSON.

http://codereview.chromium.org/7981004/diff/10001/base/debug/trace_event_test...
base/debug/trace_event_test_utils.h:84: // Returns duration if it's available.
On 2011/10/11 20:33:37, nduca wrote:
> Add a bool has_other_event() const;
> 
> Modify comment to be Returns duration if has_other_event()

Done.

http://codereview.chromium.org/7981004/diff/10001/base/debug/trace_event_test...
base/debug/trace_event_test_utils.h:88: bool IsArg(const std::string& name)
const;
On 2011/10/11 20:33:37, nduca wrote:
> HasArg

Removed it - it wasn't used anywhere :)

http://codereview.chromium.org/7981004/diff/10001/base/debug/trace_event_test...
base/debug/trace_event_test_utils.h:94: // Called by TraceAnalyzer to associate
this event with another event.
On 2011/10/11 20:33:37, nduca wrote:
> Use friending so that this isn't visible?

Also removed, it was unnecessary.

http://codereview.chromium.org/7981004/diff/10001/base/debug/trace_event_test...
base/debug/trace_event_test_utils.h:99: // Process ID and Thread ID.
On 2011/10/11 20:33:37, nduca wrote:
> Are these really supposed to be public? Makes me anxious. :)
> 
> It'd be good to hide stuff that's needed for trace analyzer, and make any
> accesses go through methods.

This is for tests, so I think it's best to keep it simple and public.

http://codereview.chromium.org/7981004/diff/10001/base/debug/trace_event_test...
base/debug/trace_event_test_utils.h:345: void SetEvents(const std::string&
json_events);
On 2011/10/11 20:33:37, nduca wrote:
> Why this rather than creating another analyzer?

The code is about the same either way - may as well allow it.

http://codereview.chromium.org/7981004/diff/10001/base/debug/trace_event_test...
base/debug/trace_event_test_utils.h:354: void SetDefaultAssociations();
On 2011/10/11 20:33:37, nduca wrote:
> Confused what this does. Does an end-user writing tests need to see this?

For now I think it makes sense to keep it available, since it documents the
default association behavior. The new examples in the initial header comments
also help explain how associations work.

http://codereview.chromium.org/7981004/diff/10001/base/debug/trace_event_test...
base/debug/trace_event_test_utils.h:357: void ClearAssociations();
On 2011/10/11 20:33:37, nduca wrote:
> Same here... what does this do? Is the idea here that we the TestTraceEvents
> returned are const so when you do a query using AssociateEvents, it causes the
> other_event fields to change? 

Correct -- AssociateEvents can be called multiple times to setup multiple
associations.

> Can we avoid this complexity, maybe at a
> fractional performance cost?

I went all the way down the functional programming style API that returns a new
copy of the event vector for each call that modifies it, and ran into two
issues:
- there's no use case for filtering multiple times, because Queries can already
be combined to accomplish that. Without this use case, there's little reason
left for the functional style API.
- the other_event pointer is invalidated and would need to be reset to point to
the new vector data for each vector copy. This would be complicated and
hazardous code, not worth the effort. Keeping copies for associations is equally
ugly: the copy's assocations could diverge from other instances in the same
results... no good.

http://codereview.chromium.org/7981004/diff/10001/base/debug/trace_event_test...
base/debug/trace_event_test_utils.h:376: 
On 2011/10/11 20:33:37, nduca wrote:
> If we had B/E only work on threads, always using 
> 
> And, suppose we had S/F that work anywhere, and that matching is done via
name,
> and any argument that starts with id. Eg
>   S A id_x=1 id_y=2
>   F A id_x=2 id_y=2 // no match
>   F A id_x=1 id_y=2 // match
> Where multiple matches are found, break tie via timestamp, e.g.
> Eg
>   S A ts=1 id=1
>   F A ts=2 id=1 // not matched
>   F A ts=1.5 id=1 // matched to the S
> 
> Does this work?

It might, but as we discussed, I'd like to start simple and build up rules as we
discover the need by writing a few tests.

http://codereview.chromium.org/7981004/diff/10001/base/debug/trace_event_unit...
File base/debug/trace_event_unittest.cc (left):

http://codereview.chromium.org/7981004/diff/10001/base/debug/trace_event_unit...
base/debug/trace_event_unittest.cc:409: // Test that categories work.
On 2011/10/11 20:33:37, nduca wrote:
> Rewrite the existing tests to use trace analyzer?

I'll leave that as an exercise for someone else to learn how to use
TraceAnalyzer. I'd like to get some real tests in first to prove out the
TraceAnalyzer design before creating too many more dependencies.

nduca

Going through the discussion stuff here, then will go back and review code. API-wise, I ...

9 years, 2 months ago (2011-10-13 01:00:27 UTC) #10

Going through the discussion stuff here, then will go back and review code.

API-wise, I think there's a lot of improvement required. While this makes sense
to you, I think it'd be hard for an average person to pick up. Specifically, I
think there are some implementation details showing on classes meant to be used
by test authors, and some operations have side effects that one wouldn't expect
to have side effects.

But, I think its also good to enable iteration rather than leave this hanging in
codereview for ages. My suggestion is that we move this to chrome/test, or even
right next to frame_rate_tests. This is a better place for iteration than base.

> This should be okay, because the parse would only fail if there's a bug in
> trace_event or trace_controller, which will be extremely rare. In the event
that
> it does happen, the test will fail because there will be no trace events.
I disagree, sorry. Think about your "is it using the gpu" test --- if for some
reason the data isn't being collected, the test code wouldn't fail --- it'd just
say that the test used the software rendering path. If you don't like a ::create
method, which is quite standard and would fix this, thats fine. But, at minimum,
its got to not let the test continue.

> Sounds good. On second look though, the trace_event change can be done
> independently since it doesn't change this code. I fixed the example below to
> make this compatible with the current assumptions about BEGIN/END events.

new.crbug.com? Its cool to do independently, but worried we're gonna land this
and never fix begin/end...

Can the associateEvents cause association between BEGIN/END across threads? I
think that should never be allowed.

> Actually, it's TraceAnalyzer::SetEvents (called during construction) that sets
> default associations. I updated the comments to make that more clear.

Cool. We should refine this API but can do that at a different step.

> AssociateEvents can be called multiple times to setup multiple associations.
> ClearAssociations resets them. This might not currently be useful, but it's
> flexible to future possibilities such as having an event associated with more
> than one other event.

I'd like to evolve this so its clearer, but we can do that later.

>> do we
> > have use cases for FindEvents with custom matchers?
> 
> Probably not, but I'm not sure. I need to implement the input latency test to
> get a better idea of what is needed. Since inputs are coalesced, it's going to
> be challenging to have a single begin/end pair that represents oldest input
and
> corresponding DoDeferredUpdate.

I hear you. I've still got a mister-yuck face about this, but I totally agree
with you on tabling this until the latency test takes shape!

> I've been thinking it would be great to move tracing stuff from base::debug to
> base or base::trace since it's no longer debug-only. That would be a great
place
> for all of trace_event.h and trace_event_analysis.h. I think that's a separate
> CL though. This header follows the trace_event.h namespace conventions except
> for the Query class, which gets its own namespace for syntactic sugar. I'm
open
> to changing namespaces up, but I'd prefer to do it in conjunction with
> trace_event.h. WDYT?
I get the point of the Query sugar... Can we move the entire analyzer file into
test::tracie_analyzer namespace? Then youd #include it and using namespace
test::trace_analyzer and be happy, right?

http://codereview.chromium.org/7981004/diff/10001/base/debug/trace_event_test...
> base/debug/trace_event_test_utils.h:77: static bool ParseEventsFromJson(const
> std::string& json,
> On 2011/10/11 20:33:37, nduca wrote:
> > Makes more sense on traceanalyzer?
> 
> This is only dependent on TestTraceEvent, so I think it's best to keep it here
> to reduce coupling. It also matches the static TraceEvent::AppendEventsAsJSON.

Yes, but in trace_event, TraceEvent is a class the user of TRACE_EVENT macros
never ever sees. So, we can let implementation details hang out a bit, so to
speak. The reason I suggested this here is because the user interacts with
TestTraceEvent, so 

If this moves to chrome/test, can we just call this TraceEvent? Since its
namespaced spearately, I think its safe to clean it. Plus, TestTraceEvent is
crazy awkward... it sounds like a test. :)

This is me wearing an API hat.

http://codereview.chromium.org/7981004/diff/10001/base/debug/trace_event_test...
> base/debug/trace_event_test_utils.h:99: // Process ID and Thread ID.
> On 2011/10/11 20:33:37, nduca wrote:
> > Are these really supposed to be public? Makes me anxious. :)
> > 
> > It'd be good to hide stuff that's needed for trace analyzer, and make any
> > accesses go through methods.
> 
> This is for tests, so I think it's best to keep it simple and public.
I understand where you're coming from. Do you agree that once we get the API
flushed out, we'll clean this up?

http://codereview.chromium.org/7981004/diff/10001/base/debug/trace_event_test...
> base/debug/trace_event_test_utils.h:345: void SetEvents(const std::string&
> json_events);
> On 2011/10/11 20:33:37, nduca wrote:
> > Why this rather than creating another analyzer?
> 
> The code is about the same either way - may as well allow it.
Yes, for now, but then down the road? We can add this in if you get a use
case.... jamesr has hammered this into me, and I agree with him these days: keep
it simple.



http://codereview.chromium.org/7981004/diff/10001/base/debug/trace_event_test...
> base/debug/trace_event_test_utils.h:354: void SetDefaultAssociations();
> On 2011/10/11 20:33:37, nduca wrote:
> > Confused what this does. Does an end-user writing tests need to see this?
> 
> For now I think it makes sense to keep it available, since it documents the
> default association behavior. The new examples in the initial header comments
> also help explain how associations work.

Then make it private?

http://codereview.chromium.org/7981004/diff/10001/base/debug/trace_event_test...
> base/debug/trace_event_test_utils.h:357: void ClearAssociations();
> On 2011/10/11 20:33:37, nduca wrote:
> > Same here... what does this do? Is the idea here that we the TestTraceEvents
> > returned are const so when you do a query using AssociateEvents, it causes
the
> > other_event fields to change? 
> 
> Correct -- AssociateEvents can be called multiple times to setup multiple
> associations.
> 
> > Can we avoid this complexity, maybe at a
> > fractional performance cost?
> 
> I went all the way down the functional programming style API that returns a
new
> copy of the event vector for each call that modifies it, and ran into two
> issues:
> - there's no use case for filtering multiple times, because Queries can
already
> be combined to accomplish that. Without this use case, there's little reason
> left for the functional style API.
Okay, thats fine.

> - the other_event pointer is invalidated and would need to be reset to point
to
> the new vector data for each vector copy. This would be complicated and
> hazardous code, not worth the effort. Keeping copies for associations is
equally
> ugly: the copy's assocations could diverge from other instances in the same
> results... no good.

I'm not buying this. SetDefaultAssocations/MakeAssociations add associations to
events inside the TraceAnalyzer. That is implemented by setting other_event on
TestTraceEvent classes. BUT, when you get events out of the analyzer, you just
return pointers.

If, when you call FindEvens, you deep copy the TestTraceEvent object [including
its current association state], you'd be fine. 

Right?

http://codereview.chromium.org/7981004/diff/10001/base/debug/trace_event_unit...
> base/debug/trace_event_unittest.cc:409: // Test that categories work.
> On 2011/10/11 20:33:37, nduca wrote:
> > Rewrite the existing tests to use trace analyzer?
> 
> I'll leave that as an exercise for someone else to learn how to use
> TraceAnalyzer. I'd like to get some real tests in first to prove out the
> TraceAnalyzer design before creating too many more dependencies.
Alright.

nduca

http://codereview.chromium.org/7981004/diff/25001/base/debug/trace_event_analyzer.h File base/debug/trace_event_analyzer.h (right): http://codereview.chromium.org/7981004/diff/25001/base/debug/trace_event_analyzer.h#newcode9 base/debug/trace_event_analyzer.h:9: // - Get trace events JSON string from base::debug::TraceLog. ...

9 years, 2 months ago (2011-10-13 01:23:58 UTC) #11

http://codereview.chromium.org/7981004/diff/25001/base/debug/trace_event_anal...
File base/debug/trace_event_analyzer.h (right):

http://codereview.chromium.org/7981004/diff/25001/base/debug/trace_event_anal...
base/debug/trace_event_analyzer.h:9: // - Get trace events JSON string from
base::debug::TraceLog.
The more I think about it, a cleanup here is
- remove clearAssociations. if you want to change associations, make anotehr
analyzer?
- make setDefaultAssociations private
- once you findEvents, you can't call AssociateEvents?

http://codereview.chromium.org/7981004/diff/25001/base/debug/trace_event_anal...
base/debug/trace_event_analyzer.h:10: // - Pass the events string to
TraceAnalyzer.
Isn't there an optional step here before FindEvents where you can call
AssociateEvents to add additional associations?

http://codereview.chromium.org/7981004/diff/25001/base/debug/trace_event_anal...
base/debug/trace_event_analyzer.h:28: // ConstEventPtrVector events;
When I see this sort of thing, its a sign something is a little odd with the
API. Here, I get that you're trying to return a vector to pointers that don't
have to be managed.

You could call this a TraceEventVector.

Or, since this isn't perf code, you can make this a lot cleaner:
    vector<TraceEvent> FindEvents(...);

Yes, yes, you'd get in trouble currently because other_event.
BUT, you could do:
  struct TraceEvent {
    const TraceEventInternal* ptr;
    // accessors that delegate to ptr; 
  }

You're welcome to argue "API later" if you want.

http://codereview.chromium.org/7981004/diff/25001/base/debug/trace_event_anal...
base/debug/trace_event_analyzer.h:47: // By default, only begin/end events on
the same thread will ever be associated.
I dont think begin or end events should *ever* be allowed to get an other_event
on another thread.

+// Begin/end events on the same thread will ever be associated.
+// If the test needs to analyze something that starts and ends on different
threads, the test needs to use INSTANT events.
+// The typical procedure is to include a
+// unique ID as one of the TRACE_EVENT arguments that only matches a single
+// INSTANT event across all Chrome processes and threads.
+//

http://codereview.chromium.org/7981004/diff/25001/base/debug/trace_event_anal...
base/debug/trace_event_analyzer.h:96: struct TestTraceEvent {
I think I mentioned this somewhere else but just call this TraceEvent?

http://codereview.chromium.org/7981004/diff/25001/base/debug/trace_event_anal...
base/debug/trace_event_analyzer.h:125: // Returns duration if has_other_event()
is true.
Might make a note about the unit of the duration.

http://codereview.chromium.org/7981004/diff/25001/base/debug/trace_event_anal...
base/debug/trace_event_analyzer.h:149: // By using the trace namespace, tests
can use short terms like "Query".
Presumably this will change with the new namespace plans...

http://codereview.chromium.org/7981004/diff/25001/base/debug/trace_event_anal...
base/debug/trace_event_analyzer.h:378: TraceAnalyzer(const EventVector& events);
what uses this?

http://codereview.chromium.org/7981004/diff/25001/base/debug/trace_event_anal...
base/debug/trace_event_analyzer.h:416: const EventVector& events() { return
raw_events_; }
Why expose this? Couldn't one do FindEvents(Query(1))?

jbates

On 2011/10/13 01:00:27, nduca wrote: > Going through the discussion stuff here, then will go ...

9 years, 2 months ago (2011-10-13 03:05:50 UTC) #12

On 2011/10/13 01:00:27, nduca wrote:
> Going through the discussion stuff here, then will go back and review code.
> 
> API-wise, I think there's a lot of improvement required. While this makes
sense
> to you, I think it'd be hard for an average person to pick up. Specifically, I
> think there are some implementation details showing on classes meant to be
used
> by test authors, and some operations have side effects that one wouldn't
expect
> to have side effects.
> 
> But, I think its also good to enable iteration rather than leave this hanging
in
> codereview for ages. My suggestion is that we move this to chrome/test, or
even
> right next to frame_rate_tests. This is a better place for iteration than
base.
> 
> > This should be okay, because the parse would only fail if there's a bug in
> > trace_event or trace_controller, which will be extremely rare. In the event
> that
> > it does happen, the test will fail because there will be no trace events.
> I disagree, sorry. Think about your "is it using the gpu" test --- if for some
> reason the data isn't being collected, the test code wouldn't fail --- it'd
just
> say that the test used the software rendering path. If you don't like a
::create
> method, which is quite standard and would fix this, thats fine. But, at
minimum,
> its got to not let the test continue.

Fair enough, will add an assertion.

> 
> > Sounds good. On second look though, the trace_event change can be done
> > independently since it doesn't change this code. I fixed the example below
to
> > make this compatible with the current assumptions about BEGIN/END events.
> 
> new.crbug.com? Its cool to do independently, but worried we're gonna land this
> and never fix begin/end...

filed crbug.com/100131

> 
> Can the associateEvents cause association between BEGIN/END across threads? I
> think that should never be allowed.
> 
> > Actually, it's TraceAnalyzer::SetEvents (called during construction) that
sets
> > default associations. I updated the comments to make that more clear.
> 
> Cool. We should refine this API but can do that at a different step.
> 
> > AssociateEvents can be called multiple times to setup multiple associations.
> > ClearAssociations resets them. This might not currently be useful, but it's
> > flexible to future possibilities such as having an event associated with
more
> > than one other event.
> 
> I'd like to evolve this so its clearer, but we can do that later.
> 
> >> do we
> > > have use cases for FindEvents with custom matchers?
> > 
> > Probably not, but I'm not sure. I need to implement the input latency test
to
> > get a better idea of what is needed. Since inputs are coalesced, it's going
to
> > be challenging to have a single begin/end pair that represents oldest input
> and
> > corresponding DoDeferredUpdate.
> 
> I hear you. I've still got a mister-yuck face about this, but I totally agree
> with you on tabling this until the latency test takes shape!
> 
> > I've been thinking it would be great to move tracing stuff from base::debug
to
> > base or base::trace since it's no longer debug-only. That would be a great
> place
> > for all of trace_event.h and trace_event_analysis.h. I think that's a
separate
> > CL though. This header follows the trace_event.h namespace conventions
except
> > for the Query class, which gets its own namespace for syntactic sugar. I'm
> open
> > to changing namespaces up, but I'd prefer to do it in conjunction with
> > trace_event.h. WDYT?
> I get the point of the Query sugar... Can we move the entire analyzer file
into
> test::tracie_analyzer namespace? Then youd #include it and using namespace
> test::trace_analyzer and be happy, right?

Done.

jbates

> http://codereview.chromium.org/7981004/diff/10001/base/debug/trace_event_test_utils.h#newcode77 > > base/debug/trace_event_test_utils.h:77: static bool ParseEventsFromJson(const > > std::string& json, > > On ...

9 years, 2 months ago (2011-10-13 03:36:15 UTC) #13

>
http://codereview.chromium.org/7981004/diff/10001/base/debug/trace_event_test...
> > base/debug/trace_event_test_utils.h:77: static bool
ParseEventsFromJson(const
> > std::string& json,
> > On 2011/10/11 20:33:37, nduca wrote:
> > > Makes more sense on traceanalyzer?
> > 
> > This is only dependent on TestTraceEvent, so I think it's best to keep it
here
> > to reduce coupling. It also matches the static
TraceEvent::AppendEventsAsJSON.
> 
> Yes, but in trace_event, TraceEvent is a class the user of TRACE_EVENT macros
> never ever sees. So, we can let implementation details hang out a bit, so to
> speak. The reason I suggested this here is because the user interacts with
> TestTraceEvent, so 

Done.

> 
> If this moves to chrome/test, can we just call this TraceEvent? Since its
> namespaced spearately, I think its safe to clean it. Plus, TestTraceEvent is
> crazy awkward... it sounds like a test. :)
> 

Done.

> > This is me wearing an API hat.
> 
>
http://codereview.chromium.org/7981004/diff/10001/base/debug/trace_event_test...
> > base/debug/trace_event_test_utils.h:99: // Process ID and Thread ID.
> > On 2011/10/11 20:33:37, nduca wrote:
> > > Are these really supposed to be public? Makes me anxious. :)
> > > 
> > > It'd be good to hide stuff that's needed for trace analyzer, and make any
> > > accesses go through methods.
> > 
> > This is for tests, so I think it's best to keep it simple and public.
> I understand where you're coming from. Do you agree that once we get the API
> flushed out, we'll clean this up?

SGTM.

> 
>
http://codereview.chromium.org/7981004/diff/10001/base/debug/trace_event_test...
> > base/debug/trace_event_test_utils.h:345: void SetEvents(const std::string&
> > json_events);
> > On 2011/10/11 20:33:37, nduca wrote:
> > > Why this rather than creating another analyzer?
> > 
> > The code is about the same either way - may as well allow it.
> Yes, for now, but then down the road? We can add this in if you get a use
> case.... jamesr has hammered this into me, and I agree with him these days:
keep
> it simple.
> 

Done.

> 
>
http://codereview.chromium.org/7981004/diff/10001/base/debug/trace_event_test...
> > base/debug/trace_event_test_utils.h:354: void SetDefaultAssociations();
> > On 2011/10/11 20:33:37, nduca wrote:
> > > Confused what this does. Does an end-user writing tests need to see this?
> > 
> > For now I think it makes sense to keep it available, since it documents the
> > default association behavior. The new examples in the initial header
comments
> > also help explain how associations work.
> 
> Then make it private?

Removed this method and replaced SetDefaultAssociations with
AssociateBeginEndEvents, which is no longer called in the constructor.

> 
>
http://codereview.chromium.org/7981004/diff/10001/base/debug/trace_event_test...
> > base/debug/trace_event_test_utils.h:357: void ClearAssociations();
> > On 2011/10/11 20:33:37, nduca wrote:
> > > Same here... what does this do? Is the idea here that we the
TestTraceEvents
> > > returned are const so when you do a query using AssociateEvents, it causes
> the
> > > other_event fields to change? 
> > 
> > Correct -- AssociateEvents can be called multiple times to setup multiple
> > associations.
> > 
> > > Can we avoid this complexity, maybe at a
> > > fractional performance cost?
> > 
> > I went all the way down the functional programming style API that returns a
> new
> > copy of the event vector for each call that modifies it, and ran into two
> > issues:
> > - there's no use case for filtering multiple times, because Queries can
> already
> > be combined to accomplish that. Without this use case, there's little reason
> > left for the functional style API.
> Okay, thats fine.
> 
> > - the other_event pointer is invalidated and would need to be reset to point
> to
> > the new vector data for each vector copy. This would be complicated and
> > hazardous code, not worth the effort. Keeping copies for associations is
> equally
> > ugly: the copy's assocations could diverge from other instances in the same
> > results... no good.
> 
> I'm not buying this. SetDefaultAssocations/MakeAssociations add associations
to
> events inside the TraceAnalyzer. That is implemented by setting other_event on
> TestTraceEvent classes. BUT, when you get events out of the analyzer, you just
> return pointers.
> 
> If, when you call FindEvens, you deep copy the TestTraceEvent object
[including
> its current association state], you'd be fine. 
> 
> Right?

Yes, but at the cost of quite a bit more code. If there's no use case, let's
just keep it simple and document the side-effects of AssociateEvents.

> 
>
http://codereview.chromium.org/7981004/diff/10001/base/debug/trace_event_unit...
> > base/debug/trace_event_unittest.cc:409: // Test that categories work.
> > On 2011/10/11 20:33:37, nduca wrote:
> > > Rewrite the existing tests to use trace analyzer?
> > 
> > I'll leave that as an exercise for someone else to learn how to use
> > TraceAnalyzer. I'd like to get some real tests in first to prove out the
> > TraceAnalyzer design before creating too many more dependencies.
> Alright.

jbates

On 2011/10/13 01:23:58, nduca wrote: > http://codereview.chromium.org/7981004/diff/25001/base/debug/trace_event_analyzer.h > File base/debug/trace_event_analyzer.h (right): > > http://codereview.chromium.org/7981004/diff/25001/base/debug/trace_event_analyzer.h#newcode9 > ...

9 years, 2 months ago (2011-10-13 04:02:51 UTC) #14

On 2011/10/13 01:23:58, nduca wrote:
>
http://codereview.chromium.org/7981004/diff/25001/base/debug/trace_event_anal...
> File base/debug/trace_event_analyzer.h (right):
> 
>
http://codereview.chromium.org/7981004/diff/25001/base/debug/trace_event_anal...
> base/debug/trace_event_analyzer.h:9: // - Get trace events JSON string from
> base::debug::TraceLog.
> The more I think about it, a cleanup here is
> - remove clearAssociations. if you want to change associations, make anotehr
> analyzer?
> - make setDefaultAssociations private

If both of these are done, then you either can't ever get default associations
or you always get default associations. Instead, I removed clearAssociations and
left setDefaultAssociations as AssociateBeginEndEvents.

> - once you findEvents, you can't call AssociateEvents?

Done.

> 
>
http://codereview.chromium.org/7981004/diff/25001/base/debug/trace_event_anal...
> base/debug/trace_event_analyzer.h:10: // - Pass the events string to
> TraceAnalyzer.
> Isn't there an optional step here before FindEvents where you can call
> AssociateEvents to add additional associations?

Done.

> 
>
http://codereview.chromium.org/7981004/diff/25001/base/debug/trace_event_anal...
> base/debug/trace_event_analyzer.h:28: // ConstEventPtrVector events;
> When I see this sort of thing, its a sign something is a little odd with the
> API. Here, I get that you're trying to return a vector to pointers that don't
> have to be managed.
> 
> You could call this a TraceEventVector.

Done.

> 
> Or, since this isn't perf code, you can make this a lot cleaner:
>     vector<TraceEvent> FindEvents(...);
> 
> Yes, yes, you'd get in trouble currently because other_event.
> BUT, you could do:
>   struct TraceEvent {
>     const TraceEventInternal* ptr;
>     // accessors that delegate to ptr; 
>   }

The devil is in the details with safe memory management, new types, conversion
to this type upon return, etc. It's a lot more code without any use cases. I
think we're fully clean now that AssociateEvents can't be called after
FindEvents.

> 
> You're welcome to argue "API later" if you want.
> 
>
http://codereview.chromium.org/7981004/diff/25001/base/debug/trace_event_anal...
> base/debug/trace_event_analyzer.h:47: // By default, only begin/end events on
> the same thread will ever be associated.
> I dont think begin or end events should *ever* be allowed to get an
other_event
> on another thread.
> 
> +// Begin/end events on the same thread will ever be associated.
> +// If the test needs to analyze something that starts and ends on different
> threads, the test needs to use INSTANT events.
> +// The typical procedure is to include a
> +// unique ID as one of the TRACE_EVENT arguments that only matches a single
> +// INSTANT event across all Chrome processes and threads.
> +//

Cleaned up the comment.

> 
>
http://codereview.chromium.org/7981004/diff/25001/base/debug/trace_event_anal...
> base/debug/trace_event_analyzer.h:96: struct TestTraceEvent {
> I think I mentioned this somewhere else but just call this TraceEvent?

Done.

> 
>
http://codereview.chromium.org/7981004/diff/25001/base/debug/trace_event_anal...
> base/debug/trace_event_analyzer.h:125: // Returns duration if
has_other_event()
> is true.
> Might make a note about the unit of the duration.

Done.

> 
>
http://codereview.chromium.org/7981004/diff/25001/base/debug/trace_event_anal...
> base/debug/trace_event_analyzer.h:149: // By using the trace namespace, tests
> can use short terms like "Query".
> Presumably this will change with the new namespace plans...

Done.

> 
>
http://codereview.chromium.org/7981004/diff/25001/base/debug/trace_event_anal...
> base/debug/trace_event_analyzer.h:378: TraceAnalyzer(const EventVector&
events);
> what uses this?

Done.

> 
>
http://codereview.chromium.org/7981004/diff/25001/base/debug/trace_event_anal...
> base/debug/trace_event_analyzer.h:416: const EventVector& events() { return
> raw_events_; }
> Why expose this? Couldn't one do FindEvents(Query(1))?

Done.

Paweł Hajdan Jr.

http://codereview.chromium.org/7981004/diff/32002/chrome/test/base/trace_event_analyzer.cc File chrome/test/base/trace_event_analyzer.cc (right): http://codereview.chromium.org/7981004/diff/32002/chrome/test/base/trace_event_analyzer.cc#newcode8 chrome/test/base/trace_event_analyzer.cc:8: #include "chrome/test/base/trace_event_analyzer.h" Why is this in chrome and not ...

9 years, 2 months ago (2011-10-13 19:50:42 UTC) #16

jbates

http://codereview.chromium.org/7981004/diff/32002/chrome/test/base/trace_event_analyzer.cc File chrome/test/base/trace_event_analyzer.cc (right): http://codereview.chromium.org/7981004/diff/32002/chrome/test/base/trace_event_analyzer.cc#newcode8 chrome/test/base/trace_event_analyzer.cc:8: #include "chrome/test/base/trace_event_analyzer.h" On 2011/10/13 19:50:43, Paweł Hajdan Jr. wrote: ...

9 years, 2 months ago (2011-10-13 19:56:01 UTC) #17

nduca

jbates: I'll do a nitpick pass tonight, but my grumpiness is replaced by that nice ...

9 years, 2 months ago (2011-10-13 21:59:36 UTC) #18

Peter Kasting

jbates asked me about operator overloading and my take was that it is (unfortunately) banned ...

9 years, 2 months ago (2011-10-13 22:10:32 UTC) #19

jbates

http://codereview.chromium.org/7981004/diff/32002/chrome/test/base/trace_event_analyzer.cc File chrome/test/base/trace_event_analyzer.cc (right): http://codereview.chromium.org/7981004/diff/32002/chrome/test/base/trace_event_analyzer.cc#newcode8 chrome/test/base/trace_event_analyzer.cc:8: #include "chrome/test/base/trace_event_analyzer.h" On 2011/10/13 19:56:01, jbates wrote: > On ...

9 years, 2 months ago (2011-10-14 03:41:20 UTC) #20

jbates

On 2011/10/13 22:10:32, Peter Kasting wrote: > If you guys want to try and make ...

9 years, 2 months ago (2011-10-14 17:30:21 UTC) #21

Zhenyao Mo

Any updates on this CL? I have a few tests pending this CL.

9 years, 2 months ago (2011-10-14 17:34:23 UTC) #22

nduca

http://codereview.chromium.org/7981004/diff/35002/base/test/trace_event_analyzer.cc File base/test/trace_event_analyzer.cc (right): http://codereview.chromium.org/7981004/diff/35002/base/test/trace_event_analyzer.cc#newcode30 base/test/trace_event_analyzer.cc:30: return; Error handling. http://codereview.chromium.org/7981004/diff/35002/base/test/trace_event_analyzer.cc#newcode43 base/test/trace_event_analyzer.cc:43: dictionary->GetDictionary("args", &args)) { I'm ...

9 years, 2 months ago (2011-10-14 23:14:15 UTC) #23

nduca

Recapping our verbal discussion, I think the core issue here is that this API lets ...

9 years, 2 months ago (2011-10-15 00:04:10 UTC) #24

jbates

On 2011/10/15 00:04:10, nduca wrote: > Recapping our verbal discussion, I think the core issue ...

9 years, 2 months ago (2011-10-15 00:34:04 UTC) #25

Paweł Hajdan Jr.

http://codereview.chromium.org/7981004/diff/35002/base/test/trace_event_analyzer.h File base/test/trace_event_analyzer.h (right): http://codereview.chromium.org/7981004/diff/35002/base/test/trace_event_analyzer.h#newcode241 base/test/trace_event_analyzer.h:241: Query operator==(const Query& rhs) const; I think you get ...

9 years, 2 months ago (2011-10-18 10:07:15 UTC) #27

Paweł Hajdan Jr.

http://codereview.chromium.org/7981004/diff/35002/base/test/trace_event_analyzer.h File base/test/trace_event_analyzer.h (right): http://codereview.chromium.org/7981004/diff/35002/base/test/trace_event_analyzer.h#newcode190 base/test/trace_event_analyzer.h:190: Query(double num); Having an override for both double and ...

9 years, 2 months ago (2011-10-18 10:10:49 UTC) #28

jbates

http://codereview.chromium.org/7981004/diff/35002/base/test/trace_event_analyzer.cc File base/test/trace_event_analyzer.cc (right): http://codereview.chromium.org/7981004/diff/35002/base/test/trace_event_analyzer.cc#newcode30 base/test/trace_event_analyzer.cc:30: return; On 2011/10/14 23:14:16, nduca wrote: > Error handling. ...

9 years, 2 months ago (2011-10-18 18:14:21 UTC) #29

http://codereview.chromium.org/7981004/diff/35002/base/test/trace_event_analy...
File base/test/trace_event_analyzer.cc (right):

http://codereview.chromium.org/7981004/diff/35002/base/test/trace_event_analy...
base/test/trace_event_analyzer.cc:30: return;
On 2011/10/14 23:14:16, nduca wrote:
> Error handling.

Done.

http://codereview.chromium.org/7981004/diff/35002/base/test/trace_event_analy...
base/test/trace_event_analyzer.cc:43: dictionary->GetDictionary("args", &args))
{
On 2011/10/14 23:14:16, nduca wrote:
> I'm concerned by the lack of failure handling here. We should dcheck or
> something...
> 
> Also, metadata events dont have to have ts, I think. They do now, but its sort
> of optional.

Done.

http://codereview.chromium.org/7981004/diff/35002/base/test/trace_event_analy...
base/test/trace_event_analyzer.cc:614: // Search for matching begin/end event
pairs. When a matching end is found,
On 2011/10/14 23:14:16, nduca wrote:
> I think you shouldn't be allowed to associate begin_end pairs from the
external
> API.

Filed http://code.google.com/p/chromium/issues/detail?id=100639 to track the
multiple association feature that gets around this problem.

http://codereview.chromium.org/7981004/diff/35002/base/test/trace_event_analy...
File base/test/trace_event_analyzer.h (right):

http://codereview.chromium.org/7981004/diff/35002/base/test/trace_event_analy...
base/test/trace_event_analyzer.h:123: bool GetDuration(double* duration) const;
On 2011/10/14 23:14:16, nduca wrote:
> duration makes sense on begin events, but for two manually associated instant
> events, its odd. Similarly, what is the duration of an end event?
> 
> TimeToOtherEvent might be more precise here. e.TimeToOtherEvent() for end
event
> "e" is -begin.TimeToOtherEvent(a)

Renamed to GetAbsTimeToOtherEvent and documented the behavior better.

http://codereview.chromium.org/7981004/diff/35002/base/test/trace_event_analy...
base/test/trace_event_analyzer.h:131: PidTid pid_tid;
On 2011/10/14 23:14:16, nduca wrote:
> Do we typically add spaces between fields? I forget... personally, this feels
> dense.

Done.

http://codereview.chromium.org/7981004/diff/35002/base/test/trace_event_analy...
base/test/trace_event_analyzer.h:190: Query(double num);
On 2011/10/18 10:10:50, Paweł Hajdan Jr. wrote:
> Having an override for both double and float seems dangerous. This is an
> invitation for trouble.

Done. FYI, this was safe because the float constructor passes through to the
double constructor. I removed the float constructor though because C++ is happy
to implicitly cast float to double.

http://codereview.chromium.org/7981004/diff/35002/base/test/trace_event_analy...
base/test/trace_event_analyzer.h:192: Query(int num);
On 2011/10/18 10:10:50, Paweł Hajdan Jr. wrote:
> Same here for int and uint, and bool... when literals are used in the client
> code I'm not so confident the expected ctor will be used. And if they're
> essentially equivalent, they shouldn't be duplicated.

These three are all distinct types, so literals should go through to the
expected constructor. I added some tests to verify the expected behavior of
literals.

http://codereview.chromium.org/7981004/diff/35002/base/test/trace_event_analy...
base/test/trace_event_analyzer.h:372: bool SetEvents(const std::string&
json_events) WARN_UNUSED_RESULT;
On 2011/10/14 23:14:16, nduca wrote:
> What happens if you call this twice?

Done.

http://codereview.chromium.org/7981004/diff/35002/base/test/trace_event_analy...
base/test/trace_event_analyzer.h:381: // AssociateEvents can be used to
customize begin/end event associations.
On 2011/10/14 23:14:16, nduca wrote:
> does this work on begin/end events? I thought you said we were going to only
> allow custom association on immediate events?

Filed bug, improved comments. See related comment in .cc.

Paweł Hajdan Jr.

http://codereview.chromium.org/7981004/diff/52001/base/test/trace_event_analyzer.cc File base/test/trace_event_analyzer.cc (right): http://codereview.chromium.org/7981004/diff/52001/base/test/trace_event_analyzer.cc#newcode62 base/test/trace_event_analyzer.cc:62: arg_numbers[*keyi] = double_num; nit: else NOTREACHED. http://codereview.chromium.org/7981004/diff/52001/base/test/trace_event_analyzer.cc#newcode157 base/test/trace_event_analyzer.cc:157: *this ...

9 years, 2 months ago (2011-10-19 08:59:23 UTC) #30

jbates

http://codereview.chromium.org/7981004/diff/52001/base/test/trace_event_analyzer.cc File base/test/trace_event_analyzer.cc (right): http://codereview.chromium.org/7981004/diff/52001/base/test/trace_event_analyzer.cc#newcode62 base/test/trace_event_analyzer.cc:62: arg_numbers[*keyi] = double_num; On 2011/10/19 08:59:23, Paweł Hajdan Jr. ...

9 years, 2 months ago (2011-10-19 19:35:54 UTC) #31

http://codereview.chromium.org/7981004/diff/52001/base/test/trace_event_analy...
File base/test/trace_event_analyzer.cc (right):

http://codereview.chromium.org/7981004/diff/52001/base/test/trace_event_analy...
base/test/trace_event_analyzer.cc:62: arg_numbers[*keyi] = double_num;
On 2011/10/19 08:59:23, Paweł Hajdan Jr. wrote:
> nit: else NOTREACHED.

Done.

http://codereview.chromium.org/7981004/diff/52001/base/test/trace_event_analy...
base/test/trace_event_analyzer.cc:157: *this = Query(static_cast<double>(boolean
? 1 : 0));
On 2011/10/19 08:59:23, Paweł Hajdan Jr. wrote:
> If we cast those things to double anyway, I'd really prefer the client code to
> do that.

The interpretation of bool is consistently handled internally in this API.
Allowing client code to decide how to convert bool to double here would make
client-specified bools potentially different from bools extracted from the trace
data. See TraceEvent::SetFromJSON.

http://codereview.chromium.org/7981004/diff/52001/base/test/trace_event_analy...
File base/test/trace_event_analyzer.h (right):

http://codereview.chromium.org/7981004/diff/52001/base/test/trace_event_analy...
base/test/trace_event_analyzer.h:196: Query(const char* str);
On 2011/10/19 08:59:23, Paweł Hajdan Jr. wrote:
> Why yet another duplicate ctor? There is an implicit conversion of char* to
> string, right?

char* can be implicitly converted to bool or std::string. This constructor is
required to prevent the wrong conversion.

http://codereview.chromium.org/7981004/diff/52001/base/test/trace_event_analy...
base/test/trace_event_analyzer.h:201: Query(uint32 num);
On 2011/10/19 08:59:23, Paweł Hajdan Jr. wrote:
> I don't really like it. Maybe Query should be distinct from things we compare
it
> with? We could have operator==(Query, double), and so on. It seems better
> solution to me than 150 ctors.

That's a good idea, but it doesn't map with how queries are evaluated as an
expression tree. Take a look at Query::CompareAsDouble. The lhs and rhs
parameters are calculated by attempting to recursively evaluate the left and
right expression trees down to a double. The double could come from any one of
the following expressions: a literal Query(double) or Query(int); a numerical
event member Query(EVENT_TIME); or an arithmetic operation (recursive). For that
to work elegantly in code, it requires all those options to be expressed by a
Query object.

Paweł Hajdan Jr.

Sorry to drag the review further on, but... have you sent a design doc for ...

9 years, 2 months ago (2011-10-21 08:31:46 UTC) #33

Sorry to drag the review further on, but... have you sent a design doc for this
earlier to chromium-dev? If yes, I'm sorry that I missed it, and if not, you may
really need to do a lot of design changes.

http://codereview.chromium.org/7981004/diff/52001/base/test/trace_event_analy...
File base/test/trace_event_analyzer.cc (right):

http://codereview.chromium.org/7981004/diff/52001/base/test/trace_event_analy...
base/test/trace_event_analyzer.cc:157: *this = Query(static_cast<double>(boolean
? 1 : 0));
On 2011/10/19 19:35:54, jbates wrote:
> On 2011/10/19 08:59:23, Paweł Hajdan Jr. wrote:
> > If we cast those things to double anyway, I'd really prefer the client code
to
> > do that.
> 
> The interpretation of bool is consistently handled internally in this API.
> Allowing client code to decide how to convert bool to double here would make
> client-specified bools potentially different from bools extracted from the
trace
> data. See TraceEvent::SetFromJSON.

Okay, just please find another solution then that satisfies the above
requirement (not pushing the conversion to the client), but doesn't need so many
ctors and implicit conversions.

http://codereview.chromium.org/7981004/diff/52001/base/test/trace_event_analy...
File base/test/trace_event_analyzer.h (right):

http://codereview.chromium.org/7981004/diff/52001/base/test/trace_event_analy...
base/test/trace_event_analyzer.h:196: Query(const char* str);
On 2011/10/19 19:35:54, jbates wrote:
> On 2011/10/19 08:59:23, Paweł Hajdan Jr. wrote:
> > Why yet another duplicate ctor? There is an implicit conversion of char* to
> > string, right?
> 
> char* can be implicitly converted to bool or std::string. This constructor is
> required to prevent the wrong conversion.

This is scary. I didn't know that, so I expect many other people to also be
surprised. Please just say no to those implicit conversions and ctor, and make
those things explicit (i.e. mention the type of the thing when creating; maybe
something like CreateFromBool, CreateFromDouble, etc).

jbates

There was no design doc, I'm afraid. I had a direct need for these features ...

9 years, 2 months ago (2011-10-21 17:54:44 UTC) #34

There was no design doc, I'm afraid. I had a direct need for these features in
GPU tests that I need to write. Test framework code is not my group's
responsibility, so I can't justify spending much more time on this. I think this
is an extremely useful API, and with nduca's and phadran's feedback it is
already quite refined.

I thought this would be useful to other teams so I am trying to put it in base.
But I was not expecting it to take over a month to review. This delay is
directly impacting GPU OKRs.

At this point if we can't move forward, I think I will pull this over into a GPU
test directory so that we can make progress.

http://codereview.chromium.org/7981004/diff/52001/base/test/trace_event_analy...
File base/test/trace_event_analyzer.cc (right):

http://codereview.chromium.org/7981004/diff/52001/base/test/trace_event_analy...
base/test/trace_event_analyzer.cc:157: *this = Query(static_cast<double>(boolean
? 1 : 0));
On 2011/10/21 08:31:46, Paweł Hajdan Jr. wrote:
> On 2011/10/19 19:35:54, jbates wrote:
> > On 2011/10/19 08:59:23, Paweł Hajdan Jr. wrote:
> > > If we cast those things to double anyway, I'd really prefer the client
code
> to
> > > do that.
> > 
> > The interpretation of bool is consistently handled internally in this API.
> > Allowing client code to decide how to convert bool to double here would make
> > client-specified bools potentially different from bools extracted from the
> trace
> > data. See TraceEvent::SetFromJSON.
> 
> Okay, just please find another solution then that satisfies the above
> requirement (not pushing the conversion to the client), but doesn't need so
many
> ctors and implicit conversions.

This code follows the same practice used in TraceValue base/debug/trace_event.h
(approved by siggi http://codereview.chromium.org/6862002/). The point of this
design is to provide a clean coding experience for client code. If you can
provide a problematic client code example, I am willing to revisit this design.
But again, this is an approved style used in other trace code.

http://codereview.chromium.org/7981004/diff/52001/base/test/trace_event_analy...
File base/test/trace_event_analyzer.h (right):

http://codereview.chromium.org/7981004/diff/52001/base/test/trace_event_analy...
base/test/trace_event_analyzer.h:196: Query(const char* str);
On 2011/10/21 08:31:46, Paweł Hajdan Jr. wrote:
> On 2011/10/19 19:35:54, jbates wrote:
> > On 2011/10/19 08:59:23, Paweł Hajdan Jr. wrote:
> > > Why yet another duplicate ctor? There is an implicit conversion of char*
to
> > > string, right?
> > 
> > char* can be implicitly converted to bool or std::string. This constructor
is
> > required to prevent the wrong conversion.
> 
> This is scary. I didn't know that, 

What did you think happens with this code?
char* ptr = 0;
if (ptr) {...}

> so I expect many other people to also be
> surprised. Please just say no to those implicit conversions and ctor, and make
> those things explicit (i.e. mention the type of the thing when creating; maybe
> something like CreateFromBool, CreateFromDouble, etc).

C++ is strongly typed. Providing constructors of multiple types has well defined
behavior. And again, this is following the same approved design as TraceValue.
The problem with your proposal is that it makes the API more difficult to use
from client code. The whole purpose of this API is to simplify client code.

Paweł Hajdan Jr.

http://codereview.chromium.org/7981004/diff/52001/base/test/trace_event_analyzer.h File base/test/trace_event_analyzer.h (right): http://codereview.chromium.org/7981004/diff/52001/base/test/trace_event_analyzer.h#newcode196 base/test/trace_event_analyzer.h:196: Query(const char* str); On 2011/10/21 17:54:44, jbates wrote: > ...

9 years, 2 months ago (2011-10-24 07:19:28 UTC) #35

jbates

On 2011/10/24 07:19:28, Paweł Hajdan Jr. wrote: > http://codereview.chromium.org/7981004/diff/52001/base/test/trace_event_analyzer.h > File base/test/trace_event_analyzer.h (right): > > ...

9 years, 2 months ago (2011-10-24 23:48:16 UTC) #36

On 2011/10/24 07:19:28, Paweł Hajdan Jr. wrote:
>
http://codereview.chromium.org/7981004/diff/52001/base/test/trace_event_analy...
> File base/test/trace_event_analyzer.h (right):
> 
>
http://codereview.chromium.org/7981004/diff/52001/base/test/trace_event_analy...
> base/test/trace_event_analyzer.h:196: Query(const char* str);
> On 2011/10/21 17:54:44, jbates wrote:
> > On 2011/10/21 08:31:46, Paweł Hajdan Jr. wrote:
> > > On 2011/10/19 19:35:54, jbates wrote:
> > > > On 2011/10/19 08:59:23, Paweł Hajdan Jr. wrote:
> > > > > Why yet another duplicate ctor? There is an implicit conversion of
char*
> > to
> > > > > string, right?
> > > > 
> > > > char* can be implicitly converted to bool or std::string. This
constructor
> > is
> > > > required to prevent the wrong conversion.
> > > 
> > > This is scary. I didn't know that, 
> > 
> > What did you think happens with this code?
> > char* ptr = 0;
> > if (ptr) {...}
> 
> This is obvious, but the interaction of this and std::string implicit
conversion
> is very confusing. That leads me to conclusion that having many ctors here
that
> are for types that otherwise would be implicitly converted to each other is
> dangerous. We could easily miss some type, and in general it's likely to do a
> different thing that people may thing by looking at the code.

Please give an example of how this might be misused in a way that will surprise
users. I am honestly open to redesign if there is a confusing use case.

> 
> > C++ is strongly typed. Providing constructors of multiple types has well
> defined
> > behavior. And again, this is following the same approved design as
TraceValue.
> > The problem with your proposal is that it makes the API more difficult to
use
> > from client code. The whole purpose of this API is to simplify client code.
> 
> Hmm, could you please post links to TraceValue reviews? I'd like to learn more
> about the history of this and any possible discussions.

Here's the link I sent previously: http://codereview.chromium.org/6862002/
That has LGTMs from siggi and brettw.

Paweł Hajdan Jr.

On 2011/10/24 23:48:16, jbates wrote: > Please give an example of how this might be ...

9 years, 2 months ago (2011-10-25 08:58:38 UTC) #37

nduca

On 2011/10/25 08:58:38, Paweł Hajdan Jr. wrote: > brett's review has a disclaimer that he ...

9 years, 2 months ago (2011-10-25 09:37:35 UTC) #38

jbates

On 2011/10/25 08:58:38, Paweł Hajdan Jr. wrote: > On 2011/10/24 23:48:16, jbates wrote: > > ...

9 years, 2 months ago (2011-10-25 17:12:41 UTC) #39

Paweł Hajdan Jr.

On 2011/10/25 09:37:35, nduca wrote: > John has done ample due dillgence on the operator ...

9 years, 2 months ago (2011-10-25 19:03:31 UTC) #40

On 2011/10/25 09:37:35, nduca wrote:
> John has done ample due dillgence on the operator overloading portion of this
> codereview. He's socialized the technique with stakeholders here, and has
gotten
> AMPLE feedback (especially from me) that the operator overloading and
> constructor-based design is problematic.

So now we have at least two people independently concerned this can be
dangerous, and yet continue pushing to check this in?

> John has made a compelling case that we should try it out and see what
happens.

I can tell you what usually happens: people start using the code and the
interface becomes much harder to change (i.e. requires a rewrite of all client
code). I don't see the "new fancy" interface as compelling enough to take that
risk.

> If you read back in my comments above, you will see my rationale for this, and
I
> stand by it today: in short, this is an experimental way of writing tests and
we
> should let the experiment begin. If it proves to be an effective way to write
> tests, THEN is the time to make the API clean. This is not a clean API, but it
> will suffice to see whether tests can be written in this way.

Sorry, the previous points are still quite arguable, but I really don't think
this is true. If you know the API has problems, they should be fixed before
people start using it, not after (that of course can't apply to problems you
don't know about). Feel free to experiment more locally. Checking in code that
is not used has its own problems anyway, no matter how clean.

> Can you provide concerete steps required in order to meet your approval? You
are
> not clear what your request is above --- you state an opinion but no action
> required.

I stated actions, but here is the summary: either reduce the number of ctors so
that it's super-obvious which one gets chosen in each case, or make the whole
thing completely explicit, i.e. FromDouble, FromString, and so on. There might
be some other, minor comments too.

On 2011/10/25 17:12:41, jbates wrote:
> I understand this is your concern. I would like to see a concrete example of
> this that we might see in test code. I can't think of any, which is why I
don't
> agree that we should disallow overloading Query constructors.

If a reviewer points out a buffer overflow in your code, do you ask him to write
an exploit before you agree to fix it?

The thing here is not about code. I guess this works correctly, at least from
the compiler's POV. However, it's about confusion in the API, that there are
ctors for things otherwise implicitly convertible. It's not easy give a code
example, it's more a confusing situation for someone writing, reading, or
debugging the code, that what a developer thinks is not necessarily what the
compiler does. Another thing is that it can easily break if someone removes one
of the ctor and now the client code starts to do implicit conversions instead of
using that ctor.

> Current API allows:
> Query q = Query(EVENT_TIME) > 1000.0
> 
> Without allowing constructors, this would be:
> Query q = Query::CreateEventMember(EVENT_TIME) > Query::CreateDouble(1000.0)
> 
> For me, this is an unacceptable tradeoff when there are no specific examples
of
> problems with the current API. The coding guidelines allow overloading within
> reason, and this appears reasonable to me considering the tradeoff.

I don't think the difference is that big, and certainly far from unacceptable
(hey, how about base::Value?). I think that explicit is better than implicit,
especially with tricky code, and I like the second example better (CreateDouble,
CreateEventMember).

> base/debug is used in shipping code. I thought base/test was only for test
code.

Does this somehow assume test code should have lower standards?

> Same reason as Query. It simplifies client code: TraceValue simplifies
> TRACE_EVENT macros.

Given the example above, I don't think the benefits outweigh the problems.

nduca

On 2011/10/25 19:03:31, Paweł Hajdan Jr. wrote: > There might be some other, minor comments ...

9 years, 2 months ago (2011-10-25 19:46:18 UTC) #41

jbates

Rather than drag this on, I went ahead and made the final change that Pawel ...

9 years, 2 months ago (2011-10-25 23:47:06 UTC) #42

Paweł Hajdan Jr.

LGTM with comments, thank you for the fixes. http://codereview.chromium.org/7981004/diff/73001/base/test/trace_event_analyzer.h File base/test/trace_event_analyzer.h (right): http://codereview.chromium.org/7981004/diff/73001/base/test/trace_event_analyzer.h#newcode44 base/test/trace_event_analyzer.h:44: // ...

9 years, 2 months ago (2011-10-26 08:38:39 UTC) #43

joth

Sorry I've been very busy with other work so unable to follow this in detail. ...

9 years, 2 months ago (2011-10-26 09:04:26 UTC) #44

Sorry I've been very busy with other work so unable to follow this in
detail. However on the std::string vs const char* c'tor overloading, I
normally favor just having one function that takes const StringPiece&. This
automatically allows for the correct promotion from char* or string. And, it
is more flexible if the caller is actually storing the string another way
where they happen to already know the length. (probably irrelevant here, but
another argument for the consistent use of it as input parameter)

Any reason not to use StringPiece here?



On 26 October 2011 00:47, <jbates@chromium.org> wrote:

> Rather than drag this on, I went ahead and made the final change that Pawel
> requested even though I disagree with it. We can revisit it later. Right
> now
> there are multiple dependencies on this CL that have been delayed for
> weeks, so
> we need to move on.
>
>
>
> http://codereview.chromium.**org/7981004/diff/52001/base/**
>
test/trace_event_analyzer.h<http://codereview.chromium.org/7981004/diff/52001/base/test/trace_event_analyzer.h>
> File base/test/trace_event_**analyzer.h (right):
>
> http://codereview.chromium.**org/7981004/diff/52001/base/**
>
test/trace_event_analyzer.h#**newcode196<http://codereview.chromium.org/7981004/diff/52001/base/test/trace_event_analyzer.h#newcode196>
> base/test/trace_event_**analyzer.h:196: Query(const char* str);
> On 2011/10/21 08:31:46, Paweł Hajdan Jr. wrote:
>
>> On 2011/10/19 19:35:54, jbates wrote:
>> > On 2011/10/19 08:59:23, Paweł Hajdan Jr. wrote:
>> > > Why yet another duplicate ctor? There is an implicit conversion of
>>
> char* to
>
>> > > string, right?
>> >
>> > char* can be implicitly converted to bool or std::string. This
>>
> constructor is
>
>> > required to prevent the wrong conversion.
>>
>
>  This is scary. I didn't know that, so I expect many other people to
>>
> also be
>
>> surprised. Please just say no to those implicit conversions and ctor,
>>
> and make
>
>> those things explicit (i.e. mention the type of the thing when
>>
> creating; maybe
>
>> something like CreateFromBool, CreateFromDouble, etc).
>>
>
> Done.
>
>
http://codereview.chromium.**org/7981004/<http://codereview.chromium.org/7981...
>

jbates

On 2011/10/26 09:04:26, joth wrote: > Sorry I've been very busy with other work so ...

9 years, 1 month ago (2011-10-26 16:31:31 UTC) #45

On 2011/10/26 09:04:26, joth wrote:
> Sorry I've been very busy with other work so unable to follow this in
> detail. However on the std::string vs const char* c'tor overloading, I
> normally favor just having one function that takes const StringPiece&. This
> automatically allows for the correct promotion from char* or string. And, it
> is more flexible if the caller is actually storing the string another way
> where they happen to already know the length. (probably irrelevant here, but
> another argument for the consistent use of it as input parameter)
> 
> Any reason not to use StringPiece here?

The problem was that const char* can implicitly cast to both bool and
std::string, so the const char* constructor is required whether we use
std::string or StringPiece. I'm generally a fan of StringPiece, but in this case
there's no need for it since this code is not performance oriented.

> 
> 
> 
> On 26 October 2011 00:47, <mailto:jbates@chromium.org> wrote:
> 
> > Rather than drag this on, I went ahead and made the final change that Pawel
> > requested even though I disagree with it. We can revisit it later. Right
> > now
> > there are multiple dependencies on this CL that have been delayed for
> > weeks, so
> > we need to move on.
> >
> >
> >
> > http://codereview.chromium.**org/7981004/diff/52001/base/**
> >
>
test/trace_event_analyzer.h<http://codereview.chromium.org/7981004/diff/52001/base/test/trace_event_analyzer.h>
> > File base/test/trace_event_**analyzer.h (right):
> >
> > http://codereview.chromium.**org/7981004/diff/52001/base/**
> >
>
test/trace_event_analyzer.h#**newcode196<http://codereview.chromium.org/7981004/diff/52001/base/test/trace_event_analyzer.h#newcode196>
> > base/test/trace_event_**analyzer.h:196: Query(const char* str);
> > On 2011/10/21 08:31:46, Paweł Hajdan Jr. wrote:
> >
> >> On 2011/10/19 19:35:54, jbates wrote:
> >> > On 2011/10/19 08:59:23, Paweł Hajdan Jr. wrote:
> >> > > Why yet another duplicate ctor? There is an implicit conversion of
> >>
> > char* to
> >
> >> > > string, right?
> >> >
> >> > char* can be implicitly converted to bool or std::string. This
> >>
> > constructor is
> >
> >> > required to prevent the wrong conversion.
> >>
> >
> >  This is scary. I didn't know that, so I expect many other people to
> >>
> > also be
> >
> >> surprised. Please just say no to those implicit conversions and ctor,
> >>
> > and make
> >
> >> those things explicit (i.e. mention the type of the thing when
> >>
> > creating; maybe
> >
> >> something like CreateFromBool, CreateFromDouble, etc).
> >>
> >
> > Done.
> >
> >
>
http://codereview.chromium.**org/7981004/%3Chttp://codereview.chromium.org/79...>
> >

jbates

willchan: base/ owners stamp please (trace_event.h/cc, base.gyp) http://codereview.chromium.org/7981004/diff/73001/base/test/trace_event_analyzer.h File base/test/trace_event_analyzer.h (right): http://codereview.chromium.org/7981004/diff/73001/base/test/trace_event_analyzer.h#newcode44 base/test/trace_event_analyzer.h:44: // Query(EVENT_DURATION) ...

9 years, 1 month ago (2011-10-26 16:55:49 UTC) #46

willchan no longer on Chromium

Jim, I'm passing onto you since you're doing some more work in this area. Pass ...

9 years, 1 month ago (2011-10-26 17:01:33 UTC) #47

jar (doing other things)

I haven't read much of this code before... so I'm being educated. Perhaps I need ...

9 years, 1 month ago (2011-10-26 19:35:20 UTC) #48

I haven't read much of this code before... so I'm being educated.  Perhaps I
need to see more high level motivation for this work.  Below are a list of nits,
as well as some more focused comments and questions.

Thanks,

Jim

http://codereview.chromium.org/7981004/diff/72003/base/debug/trace_event.cc
File base/debug/trace_event.cc (right):

http://codereview.chromium.org/7981004/diff/72003/base/debug/trace_event.cc#n...
base/debug/trace_event.cc:207: const char*
TraceEvent::GetPhaseStr(TraceEventPhase phase) {
nit: Although this is just moving... names should not use abbreviations, such as
"Str" despite the fact that C used stuff like strlen and strcpy etc.

http://codereview.chromium.org/7981004/diff/72003/base/test/trace_event_analy...
File base/test/trace_event_analyzer.cc (right):

http://codereview.chromium.org/7981004/diff/72003/base/test/trace_event_analy...
base/test/trace_event_analyzer.cc:8: #include "base/test/trace_event_analyzer.h"
nit: alphabetize

http://codereview.chromium.org/7981004/diff/72003/base/test/trace_event_analy...
base/test/trace_event_analyzer.cc:188: CHECK(type_ == QUERY_BOOLEAN_OPERATOR)
Checks in production code like this are bad. Brett is working to trim down the
500K of comments currently affiliated with CHECKs.  Please use DCHECKs, and
handle the cases (rather than crashing).

http://codereview.chromium.org/7981004/diff/72003/base/test/trace_event_analy...
base/test/trace_event_analyzer.cc:192: if (operator_ < OP_AND) {
This was hard to read, as I had to go back to the header file to see the list. 
Perhaps it would be nice to have:
IsUnaryLogicalOperator(operator)
This way I'd at least see the intent here.

http://codereview.chromium.org/7981004/diff/72003/base/test/trace_event_analy...
base/test/trace_event_analyzer.cc:303: break;
personal style nit: Early return is nicer, so that the reader doesn't have to
look down and see what else you are doing.

Suggest replace:
break;
with
return true;

http://codereview.chromium.org/7981004/diff/72003/base/test/trace_event_analy...
base/test/trace_event_analyzer.cc:632: for (size_t input_i = 0; input_i <
raw_events_.size(); ++input_i) {
nit: please use either i and j for integer index, or a full blown name such as
"index" or "input_index".
When I read "input_i" I misunderstood it to be the i'th input, when it is really
an index.

http://codereview.chromium.org/7981004/diff/72003/base/test/trace_event_analy...
base/test/trace_event_analyzer.cc:640: for (int si =
static_cast<int>(begin_stack.size()) - 1; si >= 0; --si) {
nit: variable name |si| had me looking all over.  Better is to use things like i
and j for integer iteration, or else use a full blows name.

http://codereview.chromium.org/7981004/diff/72003/base/test/trace_event_analy...
base/test/trace_event_analyzer.cc:684: if (this_event.phase ==
base::debug::TRACE_EVENT_PHASE_METADATA) {
nit: for readability, nested ifs are hard to grok.  This is a case where use of
"continue" can make things much easier to read (no curlies, no indent, and less
wrapping).
Example:
if  (this_event.phase != base::debug::TRACE_EVENT_PHASE_METADATA)
  continue;
etc.

http://codereview.chromium.org/7981004/diff/72003/base/test/trace_event_analy...
base/test/trace_event_analyzer.cc:689: if (string_i !=
this_event.arg_strings.end())
How do you deal with naming of worker threads?

http://codereview.chromium.org/7981004/diff/72003/base/test/trace_event_analy...
File base/test/trace_event_analyzer.h (right):

http://codereview.chromium.org/7981004/diff/72003/base/test/trace_event_analy...
base/test/trace_event_analyzer.h:101: struct PidTid {
nit: Avoid Abreviations.  Suggest: 
ProcessIdThreadId
or
ProcessThreadIdPair
or
ProcessAndThreadId

http://codereview.chromium.org/7981004/diff/72003/base/test/trace_event_analy...
base/test/trace_event_analyzer.h:138: double timestamp;
I was surprised to see double here (and throughout).  With all the C++ code
handling Time(), or TimeTicks(), or TimeDelta(), it seems like you'd want this
higher performance (C++) code to use this more native representation.  When you
pass it in/out of JSON, that might be a good time to translate, as that would be
off the (recording) critical path.

http://codereview.chromium.org/7981004/diff/72003/base/test/trace_event_analy...
base/test/trace_event_analyzer.h:148: std::map<std::string, double> arg_numbers;
If this is indeed a TraceEvent (as its name suggests), I'm wary that the
wastefulness here will be hard to recover from. 

You're using double precision (64 bit = 8 bytes) to hold bools here.  I assume
that line 142 category is a small enumerated set, but you a string is being
used, which probably (including overhead) is much larger than 8 bytes (it has 32
bit length, 32 bit pointer, and commonly a small built in buffer to hold a small
string).

http://codereview.chromium.org/7981004/diff/72003/base/test/trace_event_analy...
File base/test/trace_event_analyzer_unittest.cc (right):

http://codereview.chromium.org/7981004/diff/72003/base/test/trace_event_analy...
base/test/trace_event_analyzer_unittest.cc:54: // Test that the TraceAnalyzer
works.
It is much nicer to test components, and verify that they work, starting with
small items, and working your way up. 

For example:
Test1:  see if you can do a BeginTracing and EndTracing and have no data, and
handle this case well.

Test2: See if you can do tracing with just one event.  Perhaps you can JSON-ify
the results, and see if they are as expected.

Test3: Check to see that the accessors can identify elements.

etc.

As it stands now, you mostly just run a big test, and everything supposedly
works.  When this test fails, it will be much harder (in a year) to figure out
what happened.

In addition, I doubt that you get very complete coverage... but I don't know.  I
saw a lot of discussion of various binary and unary operators... and it is not
clear how or why or where this was tested.

bottom line: A series of small, well labeled tests are much easier to read, and
easier to respond to when there is a failure.

http://codereview.chromium.org/7981004/diff/72003/base/test/trace_event_analy...
base/test/trace_event_analyzer_unittest.cc:61: TRACE_EVENT_END0("cat3",
"name7");
nit: for readability, it is much nicer to use named values (put in temporaries)
than simply have embedded constants, where the user must decipher the intent. 
For example:
const char* kThreadName = "cat3";
const char* kFunction = "name7"
TRACE_EVENT_END0(kThreadName, kFunction);

Also note that you can re-use these constats later in your test, when doing the
EXPECT.. calls.

jbates

http://codereview.chromium.org/7981004/diff/72003/base/debug/trace_event.cc File base/debug/trace_event.cc (right): http://codereview.chromium.org/7981004/diff/72003/base/debug/trace_event.cc#newcode207 base/debug/trace_event.cc:207: const char* TraceEvent::GetPhaseStr(TraceEventPhase phase) { On 2011/10/26 19:35:20, jar ...

9 years, 1 month ago (2011-10-26 21:51:54 UTC) #49

http://codereview.chromium.org/7981004/diff/72003/base/debug/trace_event.cc
File base/debug/trace_event.cc (right):

http://codereview.chromium.org/7981004/diff/72003/base/debug/trace_event.cc#n...
base/debug/trace_event.cc:207: const char*
TraceEvent::GetPhaseStr(TraceEventPhase phase) {
On 2011/10/26 19:35:20, jar wrote:
> nit: Although this is just moving... names should not use abbreviations, such
as
> "Str" despite the fact that C used stuff like strlen and strcpy etc.

Done.

http://codereview.chromium.org/7981004/diff/72003/base/test/trace_event_analy...
File base/test/trace_event_analyzer.cc (right):

http://codereview.chromium.org/7981004/diff/72003/base/test/trace_event_analy...
base/test/trace_event_analyzer.cc:8: #include "base/test/trace_event_analyzer.h"
On 2011/10/26 19:35:20, jar wrote:
> nit: alphabetize

Done.

http://codereview.chromium.org/7981004/diff/72003/base/test/trace_event_analy...
base/test/trace_event_analyzer.cc:188: CHECK(type_ == QUERY_BOOLEAN_OPERATOR)
On 2011/10/26 19:35:20, jar wrote:
> Checks in production code like this are bad. Brett is working to trim down the
> 500K of comments currently affiliated with CHECKs.  Please use DCHECKs, and
> handle the cases (rather than crashing).

Done. This is base/test, so shouldn't end up in prod code. My main goal is to
assert here for both debug and release test builds. Since DCHECK also works for
Release, it works also.

http://codereview.chromium.org/7981004/diff/72003/base/test/trace_event_analy...
base/test/trace_event_analyzer.cc:192: if (operator_ < OP_AND) {
On 2011/10/26 19:35:20, jar wrote:
> This was hard to read, as I had to go back to the header file to see the list.

> Perhaps it would be nice to have:
> IsUnaryLogicalOperator(operator)
> This way I'd at least see the intent here.

Done.

http://codereview.chromium.org/7981004/diff/72003/base/test/trace_event_analy...
base/test/trace_event_analyzer.cc:303: break;
On 2011/10/26 19:35:20, jar wrote:
> personal style nit: Early return is nicer, so that the reader doesn't have to
> look down and see what else you are doing.
> 
> Suggest replace:
> break;
> with
> return true;

Done.

http://codereview.chromium.org/7981004/diff/72003/base/test/trace_event_analy...
base/test/trace_event_analyzer.cc:632: for (size_t input_i = 0; input_i <
raw_events_.size(); ++input_i) {
On 2011/10/26 19:35:20, jar wrote:
> nit: please use either i and j for integer index, or a full blown name such as
> "index" or "input_index".
> When I read "input_i" I misunderstood it to be the i'th input, when it is
really
> an index.

Done.

http://codereview.chromium.org/7981004/diff/72003/base/test/trace_event_analy...
base/test/trace_event_analyzer.cc:640: for (int si =
static_cast<int>(begin_stack.size()) - 1; si >= 0; --si) {
On 2011/10/26 19:35:20, jar wrote:
> nit: variable name |si| had me looking all over.  Better is to use things like
i
> and j for integer iteration, or else use a full blows name.

Done.

http://codereview.chromium.org/7981004/diff/72003/base/test/trace_event_analy...
base/test/trace_event_analyzer.cc:684: if (this_event.phase ==
base::debug::TRACE_EVENT_PHASE_METADATA) {
On 2011/10/26 19:35:20, jar wrote:
> nit: for readability, nested ifs are hard to grok.  This is a case where use
of
> "continue" can make things much easier to read (no curlies, no indent, and
less
> wrapping).
> Example:
> if  (this_event.phase != base::debug::TRACE_EVENT_PHASE_METADATA)
>   continue;
> etc.

Done.

http://codereview.chromium.org/7981004/diff/72003/base/test/trace_event_analy...
base/test/trace_event_analyzer.cc:689: if (string_i !=
this_event.arg_strings.end())
On 2011/10/26 19:35:20, jar wrote:
> How do you deal with naming of worker threads?

They show as "WorkerPool/3478" in about:tracing with unique numbers. The thread
names are set in base/debug/trace_event.cc.

http://codereview.chromium.org/7981004/diff/72003/base/test/trace_event_analy...
File base/test/trace_event_analyzer.h (right):

http://codereview.chromium.org/7981004/diff/72003/base/test/trace_event_analy...
base/test/trace_event_analyzer.h:138: double timestamp;
On 2011/10/26 19:35:20, jar wrote:
> I was surprised to see double here (and throughout).  With all the C++ code
> handling Time(), or TimeTicks(), or TimeDelta(), it seems like you'd want this
> higher performance (C++) code to use this more native representation.  When
you
> pass it in/out of JSON, that might be a good time to translate, as that would
be
> off the (recording) critical path.

This starts as TimeTicks in the original trace data, gets converted to double in
JSON, then gets stored here straight from JSON. In order to keep the Query
language simple, it only supports double for numbers and bools, so I didn't
bother converting this back to TimeTicks. It may be the right thing to do long
term, though.

http://codereview.chromium.org/7981004/diff/72003/base/test/trace_event_analy...
base/test/trace_event_analyzer.h:148: std::map<std::string, double> arg_numbers;
On 2011/10/26 19:35:20, jar wrote:
> If this is indeed a TraceEvent (as its name suggests), I'm wary that the
> wastefulness here will be hard to recover from. 

Sorry if it wasn't clear form the description. This is only for use in test code
(base/test), so it heavily leans towards ease-of-use over memory footprint and
performance. Incidentally, this used to be named TestTraceEvent to make it more
clear, but earlier reviewers had me change it.

> 
> You're using double precision (64 bit = 8 bytes) to hold bools here.  I assume
> that line 142 category is a small enumerated set, but you a string is being
> used, which probably (including overhead) is much larger than 8 bytes (it has
32
> bit length, 32 bit pointer, and commonly a small built in buffer to hold a
small
> string).

http://codereview.chromium.org/7981004/diff/72003/base/test/trace_event_analy...
File base/test/trace_event_analyzer_unittest.cc (right):

http://codereview.chromium.org/7981004/diff/72003/base/test/trace_event_analy...
base/test/trace_event_analyzer_unittest.cc:54: // Test that the TraceAnalyzer
works.
On 2011/10/26 19:35:20, jar wrote:
> It is much nicer to test components, and verify that they work, starting with
> small items, and working your way up. 
> 
> For example:
> Test1:  see if you can do a BeginTracing and EndTracing and have no data, and
> handle this case well.
> 
> Test2: See if you can do tracing with just one event.  Perhaps you can
JSON-ify
> the results, and see if they are as expected.
> 
> Test3: Check to see that the accessors can identify elements.
> 
> etc.
> 
> As it stands now, you mostly just run a big test, and everything supposedly
> works.  When this test fails, it will be much harder (in a year) to figure out
> what happened.
> 
> In addition, I doubt that you get very complete coverage... but I don't know. 
I
> saw a lot of discussion of various binary and unary operators... and it is not
> clear how or why or where this was tested.
> 
> bottom line: A series of small, well labeled tests are much easier to read,
and
> easier to respond to when there is a failure.

In hindsight, I agree that would be easier to maintain. Is this a blocker for
you or can that be done as a followup CL? I ask because there are some
dependencies waiting on this API.

http://codereview.chromium.org/7981004/diff/72003/base/test/trace_event_analy...
base/test/trace_event_analyzer_unittest.cc:61: TRACE_EVENT_END0("cat3",
"name7");
On 2011/10/26 19:35:20, jar wrote:
> nit: for readability, it is much nicer to use named values (put in
temporaries)
> than simply have embedded constants, where the user must decipher the intent. 
> For example:
> const char* kThreadName = "cat3";
> const char* kFunction = "name7"
> TRACE_EVENT_END0(kThreadName, kFunction);
> 
> Also note that you can re-use these constats later in your test, when doing
the
> EXPECT.. calls.

I agree that would be clearer. These trace events follow the style of
base/debug/trace_event_unittests.cc.

jbates

Blackjack! What do I win for 21 patches? (How about a final LGTM? :-) http://codereview.chromium.org/7981004/diff/72003/base/test/trace_event_analyzer_unittest.cc ...

9 years, 1 month ago (2011-10-27 20:36:34 UTC) #50

jar (doing other things)

Thanks for making the changes! Two comments below to consider. If you're ok with them, ...

9 years, 1 month ago (2011-10-28 20:47:53 UTC) #51

jbates

9 years, 1 month ago (2011-10-28 21:54:28 UTC) #52

http://codereview.chromium.org/7981004/diff/72003/base/test/trace_event_analy...
File base/test/trace_event_analyzer.cc (right):

http://codereview.chromium.org/7981004/diff/72003/base/test/trace_event_analy...
base/test/trace_event_analyzer.cc:188: CHECK(type_ == QUERY_BOOLEAN_OPERATOR)
On 2011/10/28 20:47:53, jar wrote:
> DCHECK is debug only (I think)
> CHECK is Release and Official (as well as DEBUG).
> 
> Do you need something different here?

Since --enable-dcheck works for Release builds, DCHECK should be OK.

> 
> On 2011/10/26 21:51:54, jbates wrote:
> > On 2011/10/26 19:35:20, jar wrote:
> > > Checks in production code like this are bad. Brett is working to trim down
> the
> > > 500K of comments currently affiliated with CHECKs.  Please use DCHECKs,
and
> > > handle the cases (rather than crashing).
> > 
> > Done. This is base/test, so shouldn't end up in prod code. My main goal is
to
> > assert here for both debug and release test builds. Since DCHECK also works
> for
> > Release, it works also.
>

http://codereview.chromium.org/7981004/diff/72003/base/test/trace_event_analy...
File base/test/trace_event_analyzer.h (right):

http://codereview.chromium.org/7981004/diff/72003/base/test/trace_event_analy...
base/test/trace_event_analyzer.h:101: struct PidTid {
On 2011/10/28 20:47:53, jar wrote:
> nit: You didn't change this.... did you feel this name was better for folks
that
> are savvy in this file?  ...or was it an oversight?

Oversight, oops! Done.

> 
> On 2011/10/26 19:35:20, jar wrote:
> > nit: Avoid Abreviations.  Suggest: 
> > ProcessIdThreadId
> > or
> > ProcessThreadIdPair
> > or
> > ProcessAndThreadId
>

Issue 7981004: add classes trace_analyzer::Query and TraceAnalyzer to make it easy to search through trace data (Closed)

Description

Patch Set 1 #

Patch Set 2 : . #

Patch Set 3 : test/compile #

Patch Set 4 : comments, style, arithmetic operators #

Patch Set 5 : generalized begin/end match finding #

Patch Set 6 : more comments #

Patch Set 7 : moved TestTraceEvent #

Patch Set 8 : . #

Patch Set 9 : cleanup #

Patch Set 10 : . #

Patch Set 11 : . #

Patch Set 12 : . #

Patch Set 13 : . #

Patch Set 14 : base export on TraceEvent #

Patch Set 15 : includes #

Patch Set 16 : TraceResultBuffer merge #

Patch Set 17 : reduced constructor overloading #

Patch Set 18 : comments updated #

Patch Set 19 : cleanup #

Patch Set 20 : dcheck #

Patch Set 21 : cleaned up tests #

Patch Set 22 : pid tid name fixed #

Messages