Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1923533004:
Tracing pre-filtering (Closed)
|
Created:
4 years, 8 months ago by oystein (OOO til 10th of July) Modified:
4 years, 4 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionTracing pre-filtering
This adds functionality to insert filtering predicates in-between calls to add trace events, and the actual trace buffer. These predicates can be used to implement custom logic for whether or not events should actually be added or not, in addition to the normal category checks. Events can also be modified, if need be.
The initial added usecase for this is to be able to add an additional level of filtering to reduce sizes of traces to the bare minimum, for use with perf waterfall TBMv2 metrics, and Slow Reports.
Committed: https://crrev.com/ca609aea2cd3f4c25766f1f7d2715f4c1e54e296
Cr-Commit-Position: refs/heads/master@{#412594}
Patch Set 1 : #
Total comments: 1
Patch Set 2 : #
Total comments: 21
Patch Set 3 : Review fixes #
Total comments: 2
Patch Set 4 : #Patch Set 5 : Added test #
Total comments: 26
Patch Set 6 : Rebase #Patch Set 7 : Review fixes #
Total comments: 8
Patch Set 8 : Review fixes #
Dependent Patchsets: Messages
Total messages: 58 (30 generated)
simonhatch@chromium.org changed reviewers: + simonhatch@chromium.org
https://codereview.chromium.org/1923533004/diff/100001/base/trace_event/trace... File base/trace_event/trace_log.cc (right): https://codereview.chromium.org/1923533004/diff/100001/base/trace_event/trace... base/trace_event/trace_log.cc:110: virtual bool FilterTraceEvent(const TraceEvent& trace_event) const = 0; Did we come to a decision on whether to allow the predicates to mutate the events?
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
Patchset #2 (id:120001) has been deleted
Patchset #2 (id:140001) has been deleted
Patchset #2 (id:160001) has been deleted
On 2016/05/04 19:57:38, shatch wrote: > https://codereview.chromium.org/1923533004/diff/100001/base/trace_event/trace... > File base/trace_event/trace_log.cc (right): > > https://codereview.chromium.org/1923533004/diff/100001/base/trace_event/trace... > base/trace_event/trace_log.cc:110: virtual bool FilterTraceEvent(const > TraceEvent& trace_event) const = 0; > Did we come to a decision on whether to allow the predicates to mutate the > events? I think the current decision is "keep const until we need to introduce a predicate which mutates, and then figure out any needed precautions then".
Patchset #2 (id:180001) has been deleted
Description was changed from ========== WIP NOCOMMIT: Tracing pre-filtering ========== to ========== Tracing pre-filtering ==========
oysteine@chromium.org changed reviewers: + primiano@chromium.org
First pass; ready for an early look, I think. https://codereview.chromium.org/1923533004/diff/200001/base/trace_event/trace... File base/trace_event/trace_log.cc (right): https://codereview.chromium.org/1923533004/diff/200001/base/trace_event/trace... base/trace_event/trace_log.cc:1323: std::unique_ptr<TraceEvent> new_trace_event(new TraceEvent); TODO: Figure out a way to keep this on the stack instead, combined with the unique_ptr semantics that TraceEvent::MoveFrom uses.
ping primiano: I forgot to poke you about this, sorry. Any initial comments?
On 2016/05/10 16:32:29, Oystein wrote: > ping primiano: I forgot to poke you about this, sorry. Any initial comments? Oops sorry this fell out of my inbox somehow. Will get to this tomorrow EMEA afternoon.
Ok gave a first pass. Architecturally this makes lot of sense to me and LG. Most of my comments are more about implementation details, but nothing major. The only major architecture thing left open here, is dealing with ScopedTracer. Right now there is no way to see/filter COMPLETE events on their creation. https://codereview.chromium.org/1923533004/diff/200001/base/trace_event/trace... File base/trace_event/trace_config_unittest.cc (right): https://codereview.chromium.org/1923533004/diff/200001/base/trace_event/trace... base/trace_event/trace_config_unittest.cc:36: "\"predicate\":\"event_whitelist_predicate\"" maybe if these were called predicate -> filter_predicate args -> filter_args would be a bit more clear. Args feels a bit too generic, this is really args for the given predicate. Also, I wonder if this section should just be "event_filters" instead of "category_event_filters". I know that you want to capture the fact that the category is somehow prioritary, but for somebody who reads this for the first time "category_event_filters" makes it harder to guess what this is about. The reality is that you filter by category AND event, but IMHO the category is more a detail of the filter. Also I think would help if in this example (and the other file below), predicate and args were back to back, e.g. in this sequence: - excluded_cat - included_cat - predicate - args https://codereview.chromium.org/1923533004/diff/200001/base/trace_event/trace... File base/trace_event/trace_log.cc (right): https://codereview.chromium.org/1923533004/diff/200001/base/trace_event/trace... base/trace_event/trace_log.cc:109: TraceEventFilter() {} I think you need a virtual dtor here (and override in the EventNameFilter). Otherwise I believe you might end up leaking the whitelist_ below when you clear() the list. https://codereview.chromium.org/1923533004/diff/200001/base/trace_event/trace... base/trace_event/trace_log.cc:132: return (whitelist_.find(trace_event.name()) != whitelist_.end()); either .count(...) != 0 or ContainsKey from base/std_util.h (which does exactly this) https://codereview.chromium.org/1923533004/diff/200001/base/trace_event/trace... base/trace_event/trace_log.cc:136: std::unordered_map<std::string, bool> whitelist_; Not sure what the bool is for, you seem to always set to true? This smells like an unordererd_set https://codereview.chromium.org/1923533004/diff/200001/base/trace_event/trace... base/trace_event/trace_log.cc:475: uintptr_t GetCategoryIndex(const unsigned char* category_group_enabled) { should this be moved to the anonymous namespace above, instead of having an anonymous namespace in the middle (not strongly on this honestly, I just wonder if there is precedence for having an anonymous namespace in the middle to improve flow readability) https://codereview.chromium.org/1923533004/diff/200001/base/trace_event/trace... base/trace_event/trace_log.cc:523: if (!(g_category_group_filter[category_index] == nullptr)) Isn't a bit more readable if here you cache once the std::list<>*, i'.e. auto& filters_list = g_category_group_filter[category_index].Get(); filters_list.clear(); // I assume it's a very quick noop if the list is empty. for ..... filters_list.push_back From a performance viewpoint, that will limit the acquire_load performed by LazyInstance to one (as opposite to one per loop iteration, as th ecode here) https://codereview.chromium.org/1923533004/diff/200001/base/trace_event/trace... base/trace_event/trace_log.cc:1329: std::list<std::unique_ptr<TraceEventFilter>>* filter_list = maybe that's a good case for auto? :) https://codereview.chromium.org/1923533004/diff/200001/base/trace_event/trace... base/trace_event/trace_log.cc:1331: DCHECK(filter_list); I think the policy in chrome is to not DCHECK for nullptr if you are going to dereference the thing in the same scope. rationale: It will segfault anyways. I'd keep only the 2nd dcheck https://codereview.chromium.org/1923533004/diff/200001/base/trace_event/trace... base/trace_event/trace_log.cc:1336: DCHECK(trace_event_filter); ditto here, remove this dcheck https://codereview.chromium.org/1923533004/diff/200001/base/trace_event/trace... base/trace_event/trace_log.cc:1348: lock.EnsureAcquired(); Instead of copy-pasting this logic here, which is almost identical to the ENABLED_FOR_RECORDING part below, can we instead here do the following? std::unique_ptr<TraceEvent> filtered_trace_event; //nullptr if (ENABLED_FOR_FILTERING) { filtered_trace_event.reset(, thid, offset, bla bla) foreach ... FilterTraceEvent(filtered_trace_event) } if (ENABLED_FOR_RECORDING || filtered_trace_event) { if (thread_local) ... else ... if (filtered_trace_event) trace_event->MoveFrom(filtered_trace_event) } does it make sense?
Thanks! On 2016/05/11 16:11:58, Primiano Tucci wrote: > The only major architecture thing left open here, is dealing with ScopedTracer. > Right now there is no way to see/filter COMPLETE events on their creation. Hmm maybe I'm reading this wrong, but as far as I can see we create the event in the constructor, and just update the timestamp in the destructor. The only thing that won't work is that the filters will "see" the wrong timestamp, but I'm not sure how big of a deal that is. https://codereview.chromium.org/1923533004/diff/200001/base/trace_event/trace... File base/trace_event/trace_config_unittest.cc (right): https://codereview.chromium.org/1923533004/diff/200001/base/trace_event/trace... base/trace_event/trace_config_unittest.cc:36: "\"predicate\":\"event_whitelist_predicate\"" On 2016/05/11 16:11:57, Primiano Tucci wrote: > maybe if these were called > > predicate -> filter_predicate > args -> filter_args > would be a bit more clear. Args feels a bit too generic, this is really args for > the given predicate. > > Also, I wonder if this section should just be "event_filters" instead of > "category_event_filters". I know that you want to capture the fact that the > category is somehow prioritary, but for somebody who reads this for the first > time "category_event_filters" makes it harder to guess what this is about. > The reality is that you filter by category AND event, but IMHO the category is > more a detail of the filter. > > Also I think would help if in this example (and the other file below), predicate > and args were back to back, e.g. in this sequence: > - excluded_cat > - included_cat > - predicate > - args Yeah, I like that better; done. The ordering I can't do much about, it has to be alphabetical (has to be transitive when parsed to and back from JSON). https://codereview.chromium.org/1923533004/diff/200001/base/trace_event/trace... File base/trace_event/trace_log.cc (right): https://codereview.chromium.org/1923533004/diff/200001/base/trace_event/trace... base/trace_event/trace_log.cc:109: TraceEventFilter() {} On 2016/05/11 16:11:57, Primiano Tucci wrote: > I think you need a virtual dtor here (and override in the EventNameFilter). > Otherwise I believe you might end up leaking the whitelist_ below when you > clear() the list. Done. https://codereview.chromium.org/1923533004/diff/200001/base/trace_event/trace... base/trace_event/trace_log.cc:132: return (whitelist_.find(trace_event.name()) != whitelist_.end()); On 2016/05/11 16:11:57, Primiano Tucci wrote: > either .count(...) != 0 or ContainsKey from base/std_util.h (which does exactly > this) Done. https://codereview.chromium.org/1923533004/diff/200001/base/trace_event/trace... base/trace_event/trace_log.cc:136: std::unordered_map<std::string, bool> whitelist_; On 2016/05/11 16:11:57, Primiano Tucci wrote: > Not sure what the bool is for, you seem to always set to true? This smells like > an unordererd_set Done. https://codereview.chromium.org/1923533004/diff/200001/base/trace_event/trace... base/trace_event/trace_log.cc:475: uintptr_t GetCategoryIndex(const unsigned char* category_group_enabled) { On 2016/05/11 16:11:57, Primiano Tucci wrote: > should this be moved to the anonymous namespace above, instead of having an > anonymous namespace in the middle (not strongly on this honestly, I just wonder > if there is precedence for having an anonymous namespace in the middle to > improve flow readability) Ah, done; added it in the middle for expensive while poking at it, forgot to move after :). https://codereview.chromium.org/1923533004/diff/200001/base/trace_event/trace... base/trace_event/trace_log.cc:523: if (!(g_category_group_filter[category_index] == nullptr)) On 2016/05/11 16:11:57, Primiano Tucci wrote: > Isn't a bit more readable if here you cache once the std::list<>*, i'.e. > > auto& filters_list = g_category_group_filter[category_index].Get(); > filters_list.clear(); // I assume it's a very quick noop if the list is empty. > > for ..... > > filters_list.push_back > > From a performance viewpoint, that will limit the acquire_load performed by > LazyInstance to one (as opposite to one per loop iteration, as th ecode here) My idea was to avoid the Get() (and hence the LazyInstance initialization) in the by-far most common case, i.e. no filter present. It should be rare that we have multiple predicates handling a category, too. We can always revisit this if usage changes. https://codereview.chromium.org/1923533004/diff/200001/base/trace_event/trace... base/trace_event/trace_log.cc:1329: std::list<std::unique_ptr<TraceEventFilter>>* filter_list = On 2016/05/11 16:11:58, Primiano Tucci wrote: > maybe that's a good case for auto? :) Done. https://codereview.chromium.org/1923533004/diff/200001/base/trace_event/trace... base/trace_event/trace_log.cc:1331: DCHECK(filter_list); On 2016/05/11 16:11:57, Primiano Tucci wrote: > I think the policy in chrome is to not DCHECK for nullptr if you are going to > dereference the thing in the same scope. rationale: It will segfault anyways. > I'd keep only the 2nd dcheck Done. https://codereview.chromium.org/1923533004/diff/200001/base/trace_event/trace... base/trace_event/trace_log.cc:1336: DCHECK(trace_event_filter); On 2016/05/11 16:11:57, Primiano Tucci wrote: > ditto here, remove this dcheck Done. https://codereview.chromium.org/1923533004/diff/200001/base/trace_event/trace... base/trace_event/trace_log.cc:1348: lock.EnsureAcquired(); On 2016/05/11 16:11:57, Primiano Tucci wrote: > Instead of copy-pasting this logic here, which is almost identical to the > ENABLED_FOR_RECORDING part below, can we instead here do the following? > > std::unique_ptr<TraceEvent> filtered_trace_event; //nullptr > if (ENABLED_FOR_FILTERING) { > filtered_trace_event.reset(, thid, offset, bla bla) > foreach ... > FilterTraceEvent(filtered_trace_event) > } > if (ENABLED_FOR_RECORDING || filtered_trace_event) { > if (thread_local) > ... > else > ... > if (filtered_trace_event) > trace_event->MoveFrom(filtered_trace_event) > } > > does it make sense? Done; it's a little awkward to handle all the cases, but at least there's no duplicated code now.
ssid@chromium.org changed reviewers: + ssid@chromium.org
On 2016/05/20 20:03:44, Oystein wrote: > Thanks! > > On 2016/05/11 16:11:58, Primiano Tucci wrote: > > The only major architecture thing left open here, is dealing with > ScopedTracer. > > Right now there is no way to see/filter COMPLETE events on their creation. > > Hmm maybe I'm reading this wrong, but as far as I can see we create the event in > the constructor, and just update the timestamp in the destructor. The only thing > that won't work is that the filters will "see" the wrong timestamp, but I'm not > sure how big of a deal that is. > Yes, it is very important for the heap profiler to know when the event ended. Because we annotate memory allocation to a function depending on the trace event start and end. If we miss the end of most of the trace events (which use ScopedTracer) then it is not very useful for heap profiler. At UpdateTraceEventDuration we need a call to trace_event_filter->UpdateEventDuration(trace_event); If not UpdateEventDuration something like EndEvent ? https://codereview.chromium.org/1923533004/diff/220001/base/trace_event/trace... File base/trace_event/trace_log.cc (right): https://codereview.chromium.org/1923533004/diff/220001/base/trace_event/trace... base/trace_event/trace_log.cc:1357: if (trace_event_filter.get()->FilterTraceEvent(*new_trace_event)) I dont think ".get()" is needed here. Just trace_event_filter->FilterTraceEvent() ?
On 2016/05/20 21:58:42, ssid wrote: > On 2016/05/20 20:03:44, Oystein wrote: > > Thanks! > > > > On 2016/05/11 16:11:58, Primiano Tucci wrote: > > > The only major architecture thing left open here, is dealing with > > ScopedTracer. > > > Right now there is no way to see/filter COMPLETE events on their creation. > > > > Hmm maybe I'm reading this wrong, but as far as I can see we create the event > in > > the constructor, and just update the timestamp in the destructor. The only > thing > > that won't work is that the filters will "see" the wrong timestamp, but I'm > not > > sure how big of a deal that is. > > > > Yes, it is very important for the heap profiler to know when the event ended. > Because we annotate memory allocation to a function depending on the trace event > start and end. If we miss the end of most of the trace events (which use > ScopedTracer) then it is not very useful for heap profiler. > > At UpdateTraceEventDuration we need a call to > trace_event_filter->UpdateEventDuration(trace_event); > > If not UpdateEventDuration something like EndEvent ? Yep good point; I added EndEvent() now. Is just passing in the event name/category strings enough? As there might not be an actual event created. ptal; preliminary version uploaded (still needs tests if you think this would work ok for memory-infra). https://codereview.chromium.org/1923533004/diff/220001/base/trace_event/trace... File base/trace_event/trace_log.cc (right): https://codereview.chromium.org/1923533004/diff/220001/base/trace_event/trace... base/trace_event/trace_log.cc:1357: if (trace_event_filter.get()->FilterTraceEvent(*new_trace_event)) On 2016/05/20 21:58:42, ssid wrote: > I dont think ".get()" is needed here. Just > trace_event_filter->FilterTraceEvent() ? Done.
Great, this works for memory-infra. Yes, passing the category and name is enough. Thanks.
On 2016/05/24 05:21:15, ssid wrote: > Great, this works for memory-infra. Yes, passing the category and name is > enough. > Thanks. Awesome! Ok, added test; Ready for another test, Primiano.
On 2016/05/24 22:16:43, Oystein wrote: > On 2016/05/24 05:21:15, ssid wrote: > > Great, this works for memory-infra. Yes, passing the category and name is > > enough. > > Thanks. > > Awesome! > > Ok, added test; Ready for another test, Primiano. Err. Ready for another *look*, that is.
Apologies I am super backlogged on codereview. Manage to get only half the way through. Will continue on Monday. https://codereview.chromium.org/1923533004/diff/260001/base/trace_event/trace... File base/trace_event/trace_config.cc (right): https://codereview.chromium.org/1923533004/diff/260001/base/trace_event/trace... base/trace_event/trace_config.cc:117: args_ = tc.args_->CreateDeepCopy(); I wonder, can you just say *this = tc here and reuse the code from the = operator, instead of duping it? https://codereview.chromium.org/1923533004/diff/260001/base/trace_event/trace... base/trace_event/trace_config.cc:158: if (base::MatchPattern(category_group_token.c_str(), nit: i think you dont' need c_str as StringPiece has an implicit ctor from std::string. https://codereview.chromium.org/1923533004/diff/260001/base/trace_event/trace... base/trace_event/trace_config.cc:657: LOG(ERROR) << "Invalid predicate name in category event filter."; I might be wrong but I seem to remember that LOG (i.e. non DLOG or VLOG ) are banned these days as they just spam chrome and nobody looks at them. Maybe just make this a CHECK and turn it into a hard crash, people will look at that :) https://codereview.chromium.org/1923533004/diff/260001/base/trace_event/trace... base/trace_event/trace_config.cc:668: for (size_t i = 0; i < included_list->GetSize(); ++i) { note you use i outside and inside the scope. I thought we had some warning for this. Maybe pick a different letter? :) https://codereview.chromium.org/1923533004/diff/260001/base/trace_event/trace... File base/trace_event/trace_config.h (right): https://codereview.chromium.org/1923533004/diff/260001/base/trace_event/trace... base/trace_event/trace_config.h:86: void SetArgs(std::unique_ptr<base::DictionaryValue> args); nit: you are in base, no need for base:: https://codereview.chromium.org/1923533004/diff/260001/base/trace_event/trace... base/trace_event/trace_config.h:91: base::DictionaryValue* filter_args() const { return args_.get(); } maybe you want this to be a const DictValue*? ditto for base:: https://codereview.chromium.org/1923533004/diff/260001/base/trace_event/trace... File base/trace_event/trace_event.h (right): https://codereview.chromium.org/1923533004/diff/260001/base/trace_event/trace... base/trace_event/trace_event.h:200: base::trace_event::TraceLog::GetInstance()->EndFilteredEvent Not sure you need this macro since you need this only in one place https://codereview.chromium.org/1923533004/diff/260001/base/trace_event/trace... File base/trace_event/trace_log.h (right): https://codereview.chromium.org/1923533004/diff/260001/base/trace_event/trace... base/trace_event/trace_log.h:64: // TODO description mind the todo :)
OK took a second pass. Makes sense to me. I think the only thing we need to clarify is: what is the semantic of a category being: 1) Both enabled in the standard traceconfig categories AND in the filters. 2) Enabled in the filter section BUT not in the categories. This is the trickiest part, better to have a chat offline. I think we should require that if you want to use filtering a category should be listed in both places. In this way (I think) we can guarantee to be backwards compatible and support filtering also for events coming from skia and blink, which are not aware of the _FOR_RECORDING thing. For the rest I have general comments spread over the last replies (sorry for the fragmentation). Thanks a lot for all the work. https://codereview.chromium.org/1923533004/diff/260001/base/trace_event/trace... File base/trace_event/trace_config_unittest.cc (right): https://codereview.chromium.org/1923533004/diff/260001/base/trace_event/trace... base/trace_event/trace_config_unittest.cc:402: EXPECT_STREQ(event_filter.predicate_name().c_str(), I think you want to invert the operands. EXPECT_EQ/STREQ is awkwardly yoda style (but not _GT and _LT because consistency, yay). https://codereview.chromium.org/1923533004/diff/260001/base/trace_event/trace... base/trace_event/trace_config_unittest.cc:404: EXPECT_EQ(event_filter.included_categories().size(), 1u); ditto: all these arguments should be flipped. https://codereview.chromium.org/1923533004/diff/260001/base/trace_event/trace... File base/trace_event/trace_event.h (right): https://codereview.chromium.org/1923533004/diff/260001/base/trace_event/trace... base/trace_event/trace_event.h:80: base::trace_event::TraceLog::ENABLED_FOR_FILTERING)) Do you really need this or-ed here? I'd assume that in the case of filtering the category is enabled for (RECORDING | FILTERING). Otherwise if the category gets only the ENABLED_FOR_FILTERING flag, you won't be able to filter the events coming from the subprojects (blink, skia) without making changes to them. In other words the question here should be: what is the semantic of saying that category "foo" is both part of the enabled categories and the filter? 1) It gets always inserted in the trace and is also passed to the filter 2) It is inserted in the trace only if it passes the filter. https://codereview.chromium.org/1923533004/diff/260001/base/trace_event/trace... File base/trace_event/trace_event_unittest.cc (right): https://codereview.chromium.org/1923533004/diff/260001/base/trace_event/trace... base/trace_event/trace_event_unittest.cc:3149: EXPECT_TRUE(TraceLog::GetInstance()->IsEnabled()); I think you want this to be an ASSERT_TRUE. the code below doesn't make sense otherwise. https://codereview.chromium.org/1923533004/diff/260001/base/trace_event/trace... base/trace_event/trace_event_unittest.cc:3151: TRACE_EVENT0("a cat", "a snake"); Add a comment here explaining that these trace events will go out of scope after the EndTraceAndFlush(), as opposite to "a mushroom". It was initially a bit puzzled by seeing that apparently useless scope: Or if you prefer, instead of comments replace "a snake" "a mushroom" with TRACE_EVENT0("cat", "the end of this event will not be recorded because of its scope") TRACE_EVENT0("cat", "same here") { TRACE_EVENT0("cat", "this one instead will be filtered on both begin and end") } https://codereview.chromium.org/1923533004/diff/260001/base/trace_event/trace... File base/trace_event/trace_log.cc (right): https://codereview.chromium.org/1923533004/diff/260001/base/trace_event/trace... base/trace_event/trace_log.cc:135: g_trace_event_filter_constructor_for_testing = nullptr; I'd move this to be a static field of SetTraceEventFilterCons. Otherwise In order to follow the testing code I have to jump up and down in this file. https://codereview.chromium.org/1923533004/diff/260001/base/trace_event/trace... base/trace_event/trace_log.cc:1343: std::unique_ptr<TraceEvent> new_trace_event(new TraceEvent); Can we avoid this to be heap allocated? What I found recently in the context of TracingV2 is that malloc/new is not really fast. By looking at the code here it seems filtered_trace_event could just be on the stack (+ a boolean to tell whether it was actually filtered or not) Post edit: Ohh I think i see why you did this, because of the signature of TraceEvent::MoveFrom. Ok that is my fault I think. There is no value in taking a unique_ptr there. It should be TraceEvent::MoveFrom(TraceEvent&&). Let's do something. Keep the new here temporarily but add a todo+bug to make TraceEvent a proper move type. It should be a quick CL, but better to keep it separate, there is enough stuff here :) https://codereview.chromium.org/1923533004/diff/260001/base/trace_event/trace... base/trace_event/trace_log.cc:1703: void TraceLog::SetTraceEventFilterConstructorForTesting( Drop this and jusd to TraceEventFilter::factory_for_testing = .... in the unittest https://codereview.chromium.org/1923533004/diff/260001/base/trace_event/trace... File base/trace_event/trace_log.h (right): https://codereview.chromium.org/1923533004/diff/260001/base/trace_event/trace... base/trace_event/trace_log.h:319: virtual bool FilterTraceEvent(const TraceEvent& trace_event) const = 0; Maybe call these // Returns true if the event should be appended to the buffer, false if should be discarded. bool OnEventBegin(..) // ... only for duration events. void OnDurationEventEnd() {} so it's more clear they are callbacks when you see their implementation. Also I'm not sure you want to make the method const. This enforces that the overriding class shall not change its state when filtering events. But they might need to do that (e.g. to implement hit-count filtering) no?
Friendly ping.
On 2016/06/22 17:32:09, ssid wrote: > Friendly ping. I think Oystein is on vacation for two weeks.
On 2016/06/28 15:33:28, Primiano Tucci wrote: > On 2016/06/22 17:32:09, ssid wrote: > > Friendly ping. > > I think Oystein is on vacation for two weeks. Ping.
On 2016/07/19 22:22:17, ssid wrote: > On 2016/06/28 15:33:28, Primiano Tucci wrote: > > On 2016/06/22 17:32:09, ssid wrote: > > > Friendly ping. > > > > I think Oystein is on vacation for two weeks. > > Ping. Sorry for the delay here :/. It's been crazy with planning/design since I got back, I haven't forgotten about this.
Yikes, sorry for the insanely long turnaround time :/. Context switching this back was fun. Agreed about requiring a category to be listed both in the included_categories and in the filtered ones. Being only included in the latter but not the former is semantics which doesn't really make sense with any of the present usecases. https://codereview.chromium.org/1923533004/diff/260001/base/trace_event/trace... File base/trace_event/trace_config.cc (right): https://codereview.chromium.org/1923533004/diff/260001/base/trace_event/trace... base/trace_event/trace_config.cc:117: args_ = tc.args_->CreateDeepCopy(); On 2016/05/27 17:47:06, Primiano Tucci wrote: > I wonder, can you just say *this = tc here and reuse the code from the = > operator, instead of duping it? Done. https://codereview.chromium.org/1923533004/diff/260001/base/trace_event/trace... base/trace_event/trace_config.cc:158: if (base::MatchPattern(category_group_token.c_str(), On 2016/05/27 17:47:06, Primiano Tucci wrote: > nit: i think you dont' need c_str as StringPiece has an implicit ctor from > std::string. Done. https://codereview.chromium.org/1923533004/diff/260001/base/trace_event/trace... base/trace_event/trace_config.cc:657: LOG(ERROR) << "Invalid predicate name in category event filter."; On 2016/05/27 17:47:06, Primiano Tucci wrote: > I might be wrong but I seem to remember that LOG (i.e. non DLOG or VLOG ) are > banned these days as they just spam chrome and nobody looks at them. > Maybe just make this a CHECK and turn it into a hard crash, people will look at > that :) Done. https://codereview.chromium.org/1923533004/diff/260001/base/trace_event/trace... base/trace_event/trace_config.cc:668: for (size_t i = 0; i < included_list->GetSize(); ++i) { On 2016/05/27 17:47:06, Primiano Tucci wrote: > note you use i outside and inside the scope. I thought we had some warning for > this. Maybe pick a different letter? :) Done. https://codereview.chromium.org/1923533004/diff/260001/base/trace_event/trace... File base/trace_event/trace_config_unittest.cc (right): https://codereview.chromium.org/1923533004/diff/260001/base/trace_event/trace... base/trace_event/trace_config_unittest.cc:402: EXPECT_STREQ(event_filter.predicate_name().c_str(), On 2016/05/31 15:41:23, Primiano Tucci wrote: > I think you want to invert the operands. EXPECT_EQ/STREQ is awkwardly yoda style > (but not _GT and _LT because consistency, yay). Done. https://codereview.chromium.org/1923533004/diff/260001/base/trace_event/trace... base/trace_event/trace_config_unittest.cc:404: EXPECT_EQ(event_filter.included_categories().size(), 1u); On 2016/05/31 15:41:23, Primiano Tucci wrote: > ditto: all these arguments should be flipped. Done. https://codereview.chromium.org/1923533004/diff/260001/base/trace_event/trace... File base/trace_event/trace_event.h (right): https://codereview.chromium.org/1923533004/diff/260001/base/trace_event/trace... base/trace_event/trace_event.h:80: base::trace_event::TraceLog::ENABLED_FOR_FILTERING)) On 2016/05/31 15:41:23, Primiano Tucci wrote: > Do you really need this or-ed here? > I'd assume that in the case of filtering the category is enabled for (RECORDING > | FILTERING). > Otherwise if the category gets only the ENABLED_FOR_FILTERING flag, you won't be > able to filter the events coming from the subprojects (blink, skia) without > making changes to them. > > In other words the question here should be: what is the semantic of saying that > category "foo" is both part of the enabled categories and the filter? > 1) It gets always inserted in the trace and is also passed to the filter > 2) It is inserted in the trace only if it passes the filter. I think that's right, 2) makes sense to me. The only reason to keep these separate would be if the config could essentially override the result of the filters, or something similar, but that just introduces confused semantics. Done. https://codereview.chromium.org/1923533004/diff/260001/base/trace_event/trace... File base/trace_event/trace_event_unittest.cc (right): https://codereview.chromium.org/1923533004/diff/260001/base/trace_event/trace... base/trace_event/trace_event_unittest.cc:3149: EXPECT_TRUE(TraceLog::GetInstance()->IsEnabled()); On 2016/05/31 15:41:23, Primiano Tucci wrote: > I think you want this to be an ASSERT_TRUE. the code below doesn't make sense > otherwise. Done. https://codereview.chromium.org/1923533004/diff/260001/base/trace_event/trace... base/trace_event/trace_event_unittest.cc:3151: TRACE_EVENT0("a cat", "a snake"); On 2016/05/31 15:41:23, Primiano Tucci wrote: > Add a comment here explaining that these trace events will go out of scope after > the EndTraceAndFlush(), as opposite to "a mushroom". It was initially a bit > puzzled by seeing that apparently useless scope: > Or if you prefer, instead of comments replace "a snake" "a mushroom" with > > TRACE_EVENT0("cat", "the end of this event will not be recorded because of its > scope") > TRACE_EVENT0("cat", "same here") > > { > TRACE_EVENT0("cat", "this one instead will be filtered on both begin and end") > } Done.
The CQ bit was checked by oysteine@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by oysteine@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
Spoke to Oystein a bit offline to catch up on this CL but I think this looks fine in the current state. Feel like the included/excluded stuff in EventFilterConfig could be simpler but not a big deal since it's not any different than existing. Nothing really jumping out now as really needing to be fixed so I'm fine with landing this now and starting to use it. Update the description along with a link to design doc for the feature. lgtm https://codereview.chromium.org/1923533004/diff/300001/base/trace_event/trace... File base/trace_event/trace_config.cc (right): https://codereview.chromium.org/1923533004/diff/300001/base/trace_event/trace... base/trace_event/trace_config.cc:127: TraceConfig::EventFilterConfig::~EventFilterConfig() {} nit: Add newline https://codereview.chromium.org/1923533004/diff/300001/base/trace_event/trace... File base/trace_event/trace_config.h (right): https://codereview.chromium.org/1923533004/diff/300001/base/trace_event/trace... base/trace_event/trace_config.h:88: EventFilterConfig(const EventFilterConfig& tc); nit: Kinda weird order with constructor/destructor https://codereview.chromium.org/1923533004/diff/300001/base/trace_event/trace... File base/trace_event/trace_event.h (right): https://codereview.chromium.org/1923533004/diff/300001/base/trace_event/trace... base/trace_event/trace_event.h:84: base::trace_event::TraceLog::ENABLED_FOR_FILTERING) nit: indentation seems off here https://codereview.chromium.org/1923533004/diff/300001/base/trace_event/trace... File base/trace_event/trace_log.cc (right): https://codereview.chromium.org/1923533004/diff/300001/base/trace_event/trace... base/trace_event/trace_log.cc:111: if (filter_args->GetList("event_name_whitelist", &whitelist)) { nit: maybe just define these at the top like you do in trace_config.cc
Thanks! primiano: Ping me when you're back from OOO if there's any changes you'd like to see to this, I'd be happy to do an immediate followup CL. https://codereview.chromium.org/1923533004/diff/300001/base/trace_event/trace... File base/trace_event/trace_config.cc (right): https://codereview.chromium.org/1923533004/diff/300001/base/trace_event/trace... base/trace_event/trace_config.cc:127: TraceConfig::EventFilterConfig::~EventFilterConfig() {} On 2016/08/15 21:25:47, shatch wrote: > nit: Add newline Done. https://codereview.chromium.org/1923533004/diff/300001/base/trace_event/trace... File base/trace_event/trace_config.h (right): https://codereview.chromium.org/1923533004/diff/300001/base/trace_event/trace... base/trace_event/trace_config.h:88: EventFilterConfig(const EventFilterConfig& tc); On 2016/08/15 21:25:47, shatch wrote: > nit: Kinda weird order with constructor/destructor Done. https://codereview.chromium.org/1923533004/diff/300001/base/trace_event/trace... File base/trace_event/trace_event.h (right): https://codereview.chromium.org/1923533004/diff/300001/base/trace_event/trace... base/trace_event/trace_event.h:84: base::trace_event::TraceLog::ENABLED_FOR_FILTERING) On 2016/08/15 21:25:47, shatch wrote: > nit: indentation seems off here Yeah it looks weird, but I think it's actually correct; second line's four spaces intended from the start of the statement inside of the UNLIKELY( https://codereview.chromium.org/1923533004/diff/300001/base/trace_event/trace... File base/trace_event/trace_log.cc (right): https://codereview.chromium.org/1923533004/diff/300001/base/trace_event/trace... base/trace_event/trace_log.cc:111: if (filter_args->GetList("event_name_whitelist", &whitelist)) { On 2016/08/15 21:25:47, shatch wrote: > nit: maybe just define these at the top like you do in trace_config.cc Done.
The CQ bit was checked by oysteine@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from simonhatch@chromium.org Link to the patchset: https://codereview.chromium.org/1923533004/#ps320001 (title: "Review fixes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by oysteine@chromium.org
Description was changed from ========== Tracing pre-filtering ========== to ========== Tracing pre-filtering This adds functionality to insert filtering predicates in-between calls to add trace events, and the actual trace buffer. These predicates can be used to implement custom logic for whether or not events should actually be added or not, in addition to the normal category checks. Events can also be modified, if need be. The initial added usecase for this is to be able to add an additional level of filtering to reduce sizes of traces to the bare minimum, for use with perf waterfall TBMv2 metrics, and Slow Reports. ==========
The CQ bit was checked by oysteine@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by oysteine@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Tracing pre-filtering This adds functionality to insert filtering predicates in-between calls to add trace events, and the actual trace buffer. These predicates can be used to implement custom logic for whether or not events should actually be added or not, in addition to the normal category checks. Events can also be modified, if need be. The initial added usecase for this is to be able to add an additional level of filtering to reduce sizes of traces to the bare minimum, for use with perf waterfall TBMv2 metrics, and Slow Reports. ========== to ========== Tracing pre-filtering This adds functionality to insert filtering predicates in-between calls to add trace events, and the actual trace buffer. These predicates can be used to implement custom logic for whether or not events should actually be added or not, in addition to the normal category checks. Events can also be modified, if need be. The initial added usecase for this is to be able to add an additional level of filtering to reduce sizes of traces to the bare minimum, for use with perf waterfall TBMv2 metrics, and Slow Reports. ==========
Message was sent while issue was closed.
Committed patchset #8 (id:320001)
Message was sent while issue was closed.
Description was changed from ========== Tracing pre-filtering This adds functionality to insert filtering predicates in-between calls to add trace events, and the actual trace buffer. These predicates can be used to implement custom logic for whether or not events should actually be added or not, in addition to the normal category checks. Events can also be modified, if need be. The initial added usecase for this is to be able to add an additional level of filtering to reduce sizes of traces to the bare minimum, for use with perf waterfall TBMv2 metrics, and Slow Reports. ========== to ========== Tracing pre-filtering This adds functionality to insert filtering predicates in-between calls to add trace events, and the actual trace buffer. These predicates can be used to implement custom logic for whether or not events should actually be added or not, in addition to the normal category checks. Events can also be modified, if need be. The initial added usecase for this is to be able to add an additional level of filtering to reduce sizes of traces to the bare minimum, for use with perf waterfall TBMv2 metrics, and Slow Reports. Committed: https://crrev.com/ca609aea2cd3f4c25766f1f7d2715f4c1e54e296 Cr-Commit-Position: refs/heads/master@{#412594} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/ca609aea2cd3f4c25766f1f7d2715f4c1e54e296 Cr-Commit-Position: refs/heads/master@{#412594}
Message was sent while issue was closed.
LGTM, if something doesn't work we can fix it later :) |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
