Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1776673002:
base: Add blame context (Closed)
Descriptionbase: Add blame context
This patch introduces the concept of a blame context, which is
a logical entity to which we can attribute various types of costs
(e.g., CPU usage, network bandwidth, memory allocations, etc.). A
follow-up patch will add a specific blame context for web frames.
Design doc:
https://docs.google.com/document/d/15BB-suCb9j-nFt55yCFJBJCGzLg2qUm3WaSOPb8APtI
BUG=546021
Committed: https://crrev.com/e69ffec748f93302e2174fd434e198dc295e5bf3
Cr-Commit-Position: refs/heads/master@{#382401}
Patch Set 1 #Patch Set 2 : Formatting. #Patch Set 3 : Build fix. #Patch Set 4 : Test fix. #Patch Set 5 : Detemplatize. #Patch Set 6 : Bring back the base. #
Total comments: 7
Patch Set 7 : Split out context tree and snapshot types. #Patch Set 8 : debug/ => trace_event/ #
Total comments: 1
Patch Set 9 : Split into BlameContext and TracedBlameContext. #
Total comments: 21
Patch Set 10 : Review comments. #
Total comments: 7
Patch Set 11 : Remove copy constructors. #Patch Set 12 : Unprotect destructor. #
Total comments: 14
Patch Set 13 : Review comments. #
Total comments: 4
Patch Set 14 : DCHECK_IS_ON() #Patch Set 15 : Remove was_initialized. #Patch Set 16 : Windows build fix. #
Messages
Total messages: 44 (12 generated)
Description was changed from ========== base: Add blame context This patch introduces the concept of a blame context, which is a logical entity to which we can attribute various types of costs (e.g., CPU usage, network bandwidth, memory allocations, etc.). A follow-up patch will add a specific blame context for web frames. Design doc: https://docs.google.com/document/d/15BB-suCb9j-nFt55yCFJBJCGzLg2qUm3WaSOPb8APtI BUG=546021 ========== to ========== base: Add blame context This patch introduces the concept of a blame context, which is a logical entity to which we can attribute various types of costs (e.g., CPU usage, network bandwidth, memory allocations, etc.). A follow-up patch will add a specific blame context for web frames. Design doc: https://docs.google.com/document/d/15BB-suCb9j-nFt55yCFJBJCGzLg2qUm3WaSOPb8APtI BUG=546021 ==========
skyostil@chromium.org changed reviewers: + benjhayden@chromium.org, chiniforooshan@chromium.org, nduca@chromium.org
Split out of https://codereview.chromium.org/1447563002/. How are we feeling about this interface?
On 2016/03/08 at 15:02:05, skyostil wrote: > Split out of https://codereview.chromium.org/1447563002/. How are we feeling about this interface? I like the TakeSnapshot and AsValueInto helper methods and the Enter/Leave interface. I'd still prefer a more neutral name like TaskContext, but if I'm the only one then don't worry about it. I've been thinking more about the design. Can we revisit the alternative design where each BlameContext subclass uses all the ENTER/LEAVE/SNAPSHOT macros directly instead of using clever templates to encapsulate them? Sorry, I'm waffling and I hate to be that guy. :-/ But I feel like it would be more maintainable. Templates are not very common in chrome, so the trick where the macros instantiate different category-enabled cache bits just feels too magical. Maybe it's the enabled cache bits in the macros that are too magical, but this template takes the magic to another level. The non-macro code that helps the subclass build the snapshot can still be shared in the base class, but if the subclass handles all the tracing macros, then the base class wouldn't need to be a template. Fadi reminds me that the reason for templatizing on the category and name like this was so that you could define a very neat and tidy abstraction layer. And I do appreciate that neat abstraction. But I'm not sure if that tidiness alone makes it the right abstraction here. There will only ever be 4-5ish subclasses to take advantage of the abstraction, but maybe that's enough, IDK. Strategically, the template-based design puts pressure on the tracing macro system to better support higher-level abstractions like this, whereas putting the tracing macros in the subclasses would play to the tracing macro system's current strengths. Specifically, if this would add a task to the Tracing Fix-It, then it might impact our bandwidth for the other tasks. I.e. if the cached enabled bit is the root problem, then removing it would mean that BlameContext would not need to be a magic template, but would removing the cached enabled bit be easy or would it cause knock-on problems? Maybe the tracing system *should* better support higher-level abstractions like this, but is this particular abstraction one of the directions that it should go? So perhaps this is a strategic direction question. No strong feelings, I'm happy to follow and learn here.
On 2016/03/08 18:53:00, benjhayden_chromium wrote: > On 2016/03/08 at 15:02:05, skyostil wrote: > > Split out of https://codereview.chromium.org/1447563002/. How are we feeling > about this interface? > > I like the TakeSnapshot and AsValueInto helper methods and the Enter/Leave > interface. > I'd still prefer a more neutral name like TaskContext, but if I'm the only one > then don't worry about it. I gotta say I still like 'blame' since it conveys the fact that we are trying to work out who is responsible for some unit of work (as opposed to where that work belongs). I'll use my veto powers to stick to this name unless others speak up :) > I've been thinking more about the design. > Can we revisit the alternative design where each BlameContext subclass uses all > the ENTER/LEAVE/SNAPSHOT macros directly instead of using clever templates to > encapsulate them? > Sorry, I'm waffling and I hate to be that guy. :-/ > But I feel like it would be more maintainable. Templates are not very common in > chrome, so the trick where the macros instantiate different category-enabled > cache bits just feels too magical. > Maybe it's the enabled cache bits in the macros that are too magical, but this > template takes the magic to another level. > The non-macro code that helps the subclass build the snapshot can still be > shared in the base class, but if the subclass handles all the tracing macros, > then the base class wouldn't need to be a template. > Fadi reminds me that the reason for templatizing on the category and name like > this was so that you could define a very neat and tidy abstraction layer. And I > do appreciate that neat abstraction. > But I'm not sure if that tidiness alone makes it the right abstraction here. > There will only ever be 4-5ish subclasses to take advantage of the abstraction, > but maybe that's enough, IDK. > > Strategically, the template-based design puts pressure on the tracing macro > system to better support higher-level abstractions like this, whereas putting > the tracing macros in the subclasses would play to the tracing macro system's > current strengths. > Specifically, if this would add a task to the Tracing Fix-It, then it might > impact our bandwidth for the other tasks. I.e. if the cached enabled bit is the > root problem, then removing it would mean that BlameContext would not need to be > a magic template, but would removing the cached enabled bit be easy or would it > cause knock-on problems? > Maybe the tracing system *should* better support higher-level abstractions like > this, but is this particular abstraction one of the directions that it should > go? > So perhaps this is a strategic direction question. > > No strong feelings, I'm happy to follow and learn here. I agree that the template isn't great, but I don't think we should be proliferating the trace macros all over the code base either. I've now done a pass to de-templatize by using the lower level tracing APIs (still public ones). Note that we still need the abstract BlameContextBase to let blink provide a wrapped blame context back to the platform. I think this looks pretty workable now -- WDYT?
The CL generally looks good to me, but I have one more design question before diving into details... :) https://codereview.chromium.org/1776673002/diff/100001/base/debug/blame_conte... File base/debug/blame_context.h (right): https://codereview.chromium.org/1776673002/diff/100001/base/debug/blame_conte... base/debug/blame_context.h:48: // is considered to count against all of that context's children too. This is Oh, does this attributing work to all children work for the corss-process solution? I just added a comment in the design doc, too: if we are doing some work in process A in frame context AF and then delegate some of the work to two different processes B and C. They will create their own contexts BF and CF, respectively (since contexts are per-process things). What should be the parent-child relationships so that the work attribution work correctly?
https://codereview.chromium.org/1776673002/diff/100001/base/debug/blame_conte... File base/debug/blame_context.h (right): https://codereview.chromium.org/1776673002/diff/100001/base/debug/blame_conte... base/debug/blame_context.h:48: // is considered to count against all of that context's children too. This is On 2016/03/10 16:18:55, chiniforooshan wrote: > Oh, does this attributing work to all children work for the corss-process > solution? I just added a comment in the design doc, too: if we are doing some > work in process A in frame context AF and then delegate some of the work to two > different processes B and C. They will create their own contexts BF and CF, > respectively (since contexts are per-process things). What should be the > parent-child relationships so that the work attribution work correctly? I commented on the doc but I'll repeat it here too: the relationship between frame contexts across processes isn't really parent-child but more like a sibling link that helps us connect the different shards of a single frame together. In your example the browser frame tree would have nodes AF', BF' and CF' with links to the respective blame contexts in the three renderers. When it's time to find out how much work AF', BF' and CF' are doing, we follow the links to each respective blame context and sum everything up, obeying the parent-child relationships that form the three separate per-renderer frame trees. Does that make sense?
Ok to "blame". :-) I think the lower-level tracing API fits well here. Nice! https://codereview.chromium.org/1776673002/diff/100001/base/debug/blame_conte... File base/debug/blame_context.cc (right): https://codereview.chromium.org/1776673002/diff/100001/base/debug/blame_conte... base/debug/blame_context.cc:38: << "Parent blame context must have the same name"; I thought that a WebViewImpl context could be a parent of a LocalFrame context? https://codereview.chromium.org/1776673002/diff/100001/base/debug/blame_conte... base/debug/blame_context.cc:45: id_, 0, nullptr, nullptr, nullptr, nullptr, TRACE_EVENT_FLAG_HAS_ID); Should the first nullptr should be a 0 for num_args?
https://codereview.chromium.org/1776673002/diff/100001/base/debug/blame_conte... File base/debug/blame_context.cc (right): https://codereview.chromium.org/1776673002/diff/100001/base/debug/blame_conte... base/debug/blame_context.cc:38: << "Parent blame context must have the same name"; On 2016/03/10 23:18:43, benjhayden_chromium wrote: > I thought that a WebViewImpl context could be a parent of a LocalFrame context? Yeah, I've been going back and forth about this part. I think there are two needs: 1. Each context tree needs to have an identifier that defines the the tree type. This is to support entering multiple context trees at the same time. 2. Each blame context needs to have a separate type which tells the importer what type of object it is dealing with. The same context tree may have heterogeneous context types (e.g., "frame" vs. "thread" in our case). In this patch I've only had the first type and handwaved away the second requirement since you can kind of "tell" the type from the id scope. I think I'll instead make both types explicit again. https://codereview.chromium.org/1776673002/diff/100001/base/debug/blame_conte... base/debug/blame_context.cc:45: id_, 0, nullptr, nullptr, nullptr, nullptr, TRACE_EVENT_FLAG_HAS_ID); On 2016/03/10 23:18:43, benjhayden_chromium wrote: > Should the first nullptr should be a 0 for num_args? Hmm, no? The first nullptr is the array of arg_names. I've added a named constant for num_args to make it a little more obvious.
lgtm Thanks! https://codereview.chromium.org/1776673002/diff/100001/base/debug/blame_conte... File base/debug/blame_context.cc (right): https://codereview.chromium.org/1776673002/diff/100001/base/debug/blame_conte... base/debug/blame_context.cc:45: id_, 0, nullptr, nullptr, nullptr, nullptr, TRACE_EVENT_FLAG_HAS_ID); On 2016/03/14 at 16:59:14, Sami wrote: > On 2016/03/10 23:18:43, benjhayden_chromium wrote: > > Should the first nullptr should be a 0 for num_args? > > Hmm, no? The first nullptr is the array of arg_names. I've added a named constant for num_args to make it a little more obvious. Oh, sorry! Codesearch showed me the trace-event.h in v8, which takes bind_id between id and num_args, whereas the trace_event.h in base uses a WITH_BIND_ID macro variant.
skyostil@chromium.org changed reviewers: + danakj@chromium.org
Thanks Ben! Dana, would you like to review this for reals?
Why in base/debug/ and not in base/trace_event?
On 2016/03/14 18:06:23, danakj wrote: > Why in base/debug/ and not in base/trace_event? It's somewhat arbitrary -- I could see this being in either place. The move to base/debug/ came after it was pointed out that blame contexts don't necessarily need to be tied together with tracing. Right now they're definitely closely intertwined, but I have ideas about a runtime v2 which doesn't require tracing to be useful. Nat, do you have a feel here?
We threw some dice with Nat and base/trace_event/ does seem like the more appropriate location. base/debug/ seems to be reserved for more specific, lower level things. The only problem with base/trace_event/ is that there's already a lot of stuff there. I've filed https://bugs.chromium.org/p/chromium/issues/detail?id=594630 for rethinking some of that.
OK cool, then u don't need owners from me, but I can still review if u want. One intro question.. https://codereview.chromium.org/1776673002/diff/140001/base/trace_event/blame... File base/trace_event/blame_context.h (right): https://codereview.chromium.org/1776673002/diff/140001/base/trace_event/blame... base/trace_event/blame_context.h:22: class BASE_EXPORT BlameContextBase { What's the goal of having this base class?
Broadly, debug/ i think is probably not right because thats for very specific stuff not related to measuring and stuff. Someday, I think we take metrics, and tracing, and we make a directory thats something about performance. And then in there there's some organization underneath that makes this all less messy. But clearly, that's something for the future, but an important something. How about we file a bug which is "break apart tracing directory into pieces and explain relationship to metrics"? For now, we could put this in tracing/ for now and for people who have concerns, we note that bug?
On 2016/03/14 18:43:59, danakj wrote: > OK cool, then u don't need owners from me, but I can still review if u want. One > intro question.. Happy to get a review if you're up for it. > https://codereview.chromium.org/1776673002/diff/140001/base/trace_event/blame... > File base/trace_event/blame_context.h (right): > > https://codereview.chromium.org/1776673002/diff/140001/base/trace_event/blame... > base/trace_event/blame_context.h:22: class BASE_EXPORT BlameContextBase { > What's the goal of having this base class? It's there because components/scheduler/base/ wants to enter and leave a blame context which is implemented by Blink and we can't easily make that be the concrete BlameContext object. See https://code.google.com/p/chromium/codesearch#chromium/src/components/schedul... One alternative is that I define the abstract interface in the scheduler instead, although that would mean some more wrapping in content/. Another option is to come up with a better name for the base interface (maybe BlameContext & SnapshottedBlameContext, or BlameContext & TracedBlameContext, *shrug*).
Decided to split this into an abstract BlameContext and a TracedBlameContext subclass. I think that makes the division of functionality clearer.
https://codereview.chromium.org/1776673002/diff/160001/base/test/trace_event_... File base/test/trace_event_analyzer.cc (right): https://codereview.chromium.org/1776673002/diff/160001/base/test/trace_event_... base/test/trace_event_analyzer.cc:46: for (auto it = rhs.arg_values.cbegin(); it != rhs.arg_values.cend(); ++it) { for (const auto& pair: rhs.arg_values) arg_values[pair.first] = pair.second->CreateDeepCopy(); ? https://codereview.chromium.org/1776673002/diff/160001/base/test/trace_event_... base/test/trace_event_analyzer.cc:164: std::map<std::string, scoped_ptr<base::Value>>::const_iterator i = auto please. |it| instead of |i| https://codereview.chromium.org/1776673002/diff/160001/base/test/trace_event_... File base/test/trace_event_analyzer.h (right): https://codereview.chromium.org/1776673002/diff/160001/base/test/trace_event_... base/test/trace_event_analyzer.h:123: void operator=(const TraceEvent& rhs); TraceEvent is pretty heavy. Does it need to be copyable? Can it be moveable instead? Anyhow, in this case a move would be far cheaper than a copy (this thing is full of stl containers), so if you're going to add a copy operator i recomment a move one too (and a move constructor). https://codereview.chromium.org/1776673002/diff/160001/base/test/trace_event_... base/test/trace_event_analyzer.h:180: The amount of whitespace in this class is impressive :) https://codereview.chromium.org/1776673002/diff/160001/base/test/trace_event_... File base/test/trace_event_analyzer_unittest.cc (right): https://codereview.chromium.org/1776673002/diff/160001/base/test/trace_event_... base/test/trace_event_analyzer_unittest.cc:101: event.arg_values["dict"] = make_scoped_ptr(new base::DictionaryValue()); Do you unit test the from json method? https://codereview.chromium.org/1776673002/diff/160001/base/trace_event/blame... File base/trace_event/blame_context.h (right): https://codereview.chromium.org/1776673002/diff/160001/base/trace_event/blame... base/trace_event/blame_context.h:35: virtual ~BlameContext() {} do you need to define this destructor? making a destructor has side effects, like it deletes default operators and stuff (do you need a constructor or DISALLOW_COPY_AND_ASSIGN either for an interface? those seem like they all belong on a concrete class not an interface) https://codereview.chromium.org/1776673002/diff/160001/base/trace_event/trace... File base/trace_event/traced_blame_context.h (right): https://codereview.chromium.org/1776673002/diff/160001/base/trace_event/trace... base/trace_event/traced_blame_context.h:36: // to the tracing category |category|, identified by |id| from to the |scope| "using the tracing" "from the |scope|" https://codereview.chromium.org/1776673002/diff/160001/base/trace_event/trace... base/trace_event/traced_blame_context.h:46: // Construct a blame context belonging to the blame context tree|name|, using "tree |name|" https://codereview.chromium.org/1776673002/diff/160001/base/trace_event/trace... base/trace_event/traced_blame_context.h:47: // to the tracing category |category|, identified by |id| from to the |scope| "using the tracing" "from the |scope|" https://codereview.chromium.org/1776673002/diff/160001/base/trace_event/trace... base/trace_event/traced_blame_context.h:61: void Enter() override; I'd still like to see us do this without virtuals, which I think we can do with a type alias in public/platform/ to let core and content use the same concrete type from base.
https://codereview.chromium.org/1776673002/diff/160001/base/test/trace_event_... File base/test/trace_event_analyzer.cc (right): https://codereview.chromium.org/1776673002/diff/160001/base/test/trace_event_... base/test/trace_event_analyzer.cc:46: for (auto it = rhs.arg_values.cbegin(); it != rhs.arg_values.cend(); ++it) { On 2016/03/16 18:29:49, danakj wrote: > for (const auto& pair: rhs.arg_values) > arg_values[pair.first] = pair.second->CreateDeepCopy(); > > ? Much neater, thanks. https://codereview.chromium.org/1776673002/diff/160001/base/test/trace_event_... base/test/trace_event_analyzer.cc:164: std::map<std::string, scoped_ptr<base::Value>>::const_iterator i = On 2016/03/16 18:29:49, danakj wrote: > auto please. > > |it| instead of |i| Done. Fixed the others too. https://codereview.chromium.org/1776673002/diff/160001/base/test/trace_event_... File base/test/trace_event_analyzer.h (right): https://codereview.chromium.org/1776673002/diff/160001/base/test/trace_event_... base/test/trace_event_analyzer.h:123: void operator=(const TraceEvent& rhs); On 2016/03/16 18:29:49, danakj wrote: > TraceEvent is pretty heavy. Does it need to be copyable? Can it be moveable > instead? > > Anyhow, in this case a move would be far cheaper than a copy (this thing is full > of stl containers), so if you're going to add a copy operator i recomment a move > one too (and a move constructor). It's kept around in stl containers so it needs to be copyable at minimum. A move operator & constructor sounds like a good idea -- done. https://codereview.chromium.org/1776673002/diff/160001/base/test/trace_event_... base/test/trace_event_analyzer.h:180: On 2016/03/16 18:29:49, danakj wrote: > The > > amount > > of > > whitespace > > in > > this > > class > > is > > impressive :) Now that you mention it I think I'll just compact it :) https://codereview.chromium.org/1776673002/diff/160001/base/test/trace_event_... File base/test/trace_event_analyzer_unittest.cc (right): https://codereview.chromium.org/1776673002/diff/160001/base/test/trace_event_... base/test/trace_event_analyzer_unittest.cc:101: event.arg_values["dict"] = make_scoped_ptr(new base::DictionaryValue()); On 2016/03/16 18:29:49, danakj wrote: > Do you unit test the from json method? Looks like it was missing -- added. https://codereview.chromium.org/1776673002/diff/160001/base/trace_event/blame... File base/trace_event/blame_context.h (right): https://codereview.chromium.org/1776673002/diff/160001/base/trace_event/blame... base/trace_event/blame_context.h:35: virtual ~BlameContext() {} On 2016/03/16 18:29:49, danakj wrote: > do you need to define this destructor? making a destructor has side effects, > like it deletes default operators and stuff > > (do you need a constructor or DISALLOW_COPY_AND_ASSIGN either for an interface? > those seem like they all belong on a concrete class not an interface) I guess I wouldn't have needed this for the interface. It's now gone but thanks for the tip anyway. https://codereview.chromium.org/1776673002/diff/160001/base/trace_event/trace... File base/trace_event/traced_blame_context.h (right): https://codereview.chromium.org/1776673002/diff/160001/base/trace_event/trace... base/trace_event/traced_blame_context.h:36: // to the tracing category |category|, identified by |id| from to the |scope| On 2016/03/16 18:29:50, danakj wrote: > "using the tracing" > > "from the |scope|" Argh, looks like these got mangled somewhere in during the N refactorings. Fixed. https://codereview.chromium.org/1776673002/diff/160001/base/trace_event/trace... base/trace_event/traced_blame_context.h:46: // Construct a blame context belonging to the blame context tree|name|, using On 2016/03/16 18:29:50, danakj wrote: > "tree |name|" Done. https://codereview.chromium.org/1776673002/diff/160001/base/trace_event/trace... base/trace_event/traced_blame_context.h:47: // to the tracing category |category|, identified by |id| from to the |scope| On 2016/03/16 18:29:50, danakj wrote: > "using the tracing" > > "from the |scope|" Done. https://codereview.chromium.org/1776673002/diff/160001/base/trace_event/trace... base/trace_event/traced_blame_context.h:61: void Enter() override; On 2016/03/16 18:29:50, danakj wrote: > I'd still like to see us do this without virtuals, which I think we can do with > a type alias in public/platform/ to let core and content use the same concrete > type from base. Looks like it all fits together, thanks for the suggestion.
https://codereview.chromium.org/1776673002/diff/160001/base/test/trace_event_... File base/test/trace_event_analyzer.h (right): https://codereview.chromium.org/1776673002/diff/160001/base/test/trace_event_... base/test/trace_event_analyzer.h:123: void operator=(const TraceEvent& rhs); On 2016/03/17 12:27:13, Sami wrote: > On 2016/03/16 18:29:49, danakj wrote: > > TraceEvent is pretty heavy. Does it need to be copyable? Can it be moveable > > instead? > > > > Anyhow, in this case a move would be far cheaper than a copy (this thing is > full > > of stl containers), so if you're going to add a copy operator i recomment a > move > > one too (and a move constructor). > > It's kept around in stl containers so it needs to be copyable at minimum. A move > operator & constructor sounds like a good idea -- done. STL containers don't need to copy anymore with C++11.
I think this LGTM now overall :) (Should have a tracing owner look at it maybe?) But if you can delete the copy constructors you'll be happier I suspect. At least with the move ones, now STL containers will no longer use the copy ones. https://codereview.chromium.org/1776673002/diff/180001/base/test/trace_event_... File base/test/trace_event_analyzer.cc (right): https://codereview.chromium.org/1776673002/diff/180001/base/test/trace_event_... base/test/trace_event_analyzer.cc:33: TraceEvent::TraceEvent(TraceEvent&& other) { does = default work for this? https://codereview.chromium.org/1776673002/diff/180001/base/test/trace_event_... base/test/trace_event_analyzer.cc:56: TraceEvent& TraceEvent::operator=(TraceEvent&& rhs) { does = default work for this? https://codereview.chromium.org/1776673002/diff/180001/base/trace_event/blame... File base/trace_event/blame_context.h (right): https://codereview.chromium.org/1776673002/diff/180001/base/trace_event/blame... base/trace_event/blame_context.h:35: class BASE_EXPORT BlameContext Yay concrete class
vmpstr@chromium.org changed reviewers: + vmpstr@chromium.org
https://codereview.chromium.org/1776673002/diff/180001/base/test/trace_event_... File base/test/trace_event_analyzer.cc (right): https://codereview.chromium.org/1776673002/diff/180001/base/test/trace_event_... base/test/trace_event_analyzer.cc:56: TraceEvent& TraceEvent::operator=(TraceEvent&& rhs) { On 2016/03/18 21:34:56, danakj wrote: > does = default work for this? iirc msvc2013 might not like it
https://codereview.chromium.org/1776673002/diff/180001/base/test/trace_event_... File base/test/trace_event_analyzer.cc (right): https://codereview.chromium.org/1776673002/diff/180001/base/test/trace_event_... base/test/trace_event_analyzer.cc:56: TraceEvent& TraceEvent::operator=(TraceEvent&& rhs) { On 2016/03/18 21:36:18, vmpstr wrote: > On 2016/03/18 21:34:56, danakj wrote: > > does = default work for this? > > iirc msvc2013 might not like it Well, we're on 2015 now :)
https://codereview.chromium.org/1776673002/diff/180001/base/test/trace_event_... File base/test/trace_event_analyzer.cc (right): https://codereview.chromium.org/1776673002/diff/180001/base/test/trace_event_... base/test/trace_event_analyzer.cc:33: TraceEvent::TraceEvent(TraceEvent&& other) { On 2016/03/18 21:34:56, danakj wrote: > does = default work for this? Looks like it does -- done. https://codereview.chromium.org/1776673002/diff/180001/base/test/trace_event_... base/test/trace_event_analyzer.cc:56: TraceEvent& TraceEvent::operator=(TraceEvent&& rhs) { On 2016/03/18 21:39:10, danakj wrote: > On 2016/03/18 21:36:18, vmpstr wrote: > > On 2016/03/18 21:34:56, danakj wrote: > > > does = default work for this? > > > > iirc msvc2013 might not like it > > Well, we're on 2015 now :) Let's see if the trybots agree :)
primiano@chromium.org changed reviewers: + primiano@chromium.org
The overall code LG, just some minor comments below. I think the major issue here might be this: in which case do you expect the snapshot to be dumped in the trace. If I remember all the tracing code correctly, and am reading this one right, I think that you are now in the state where: - If the BlameContext object was created (hence the EnabledStateObserver was registered) before tracing was started, you will get a snapshot in the trace. - If the blameContext is created after the start of tracing, you will get no snapshot. Which might end up creating some headaches. I had some similar problems in memory-infra, see https://code.google.com/p/chromium/codesearch#chromium/src/base/trace_event/m... Or maybe I am just misunderstanding the rationale of the OnTraceLogEnabled here? https://codereview.chromium.org/1776673002/diff/220001/base/trace_event/blame... File base/trace_event/blame_context.cc (right): https://codereview.chromium.org/1776673002/diff/220001/base/trace_event/blame... base/trace_event/blame_context.cc:58: num_args, nullptr, nullptr, nullptr, nullptr, or just 0 /* num_args */ if you prefer :) https://codereview.chromium.org/1776673002/diff/220001/base/trace_event/blame... base/trace_event/blame_context.cc:76: int num_args = 1; nit: kNumArgs? https://codereview.chromium.org/1776673002/diff/220001/base/trace_event/blame... base/trace_event/blame_context.cc:87: TakeSnapshot(); What is the reason for this? Is it to make it so that your BlameContext is always dumped? IIRC OnTraceLogEnabled is NOT invoked if you register the observer after tracing has started * Does this mean that if you create a BlameContext after tracing has started you will miss the snapshot? * I am looking at my own comment in https://code.google.com/p/chromium/codesearch#chromium/src/base/trace_event/m... https://codereview.chromium.org/1776673002/diff/220001/base/trace_event/blame... File base/trace_event/blame_context.h (right): https://codereview.chromium.org/1776673002/diff/220001/base/trace_event/blame... base/trace_event/blame_context.h:42: // strings must have application lifetime. Maybe an example of how the actual args to the ctor (up there on lines 21-34 where you have a sample use case) might help to understand this. https://codereview.chromium.org/1776673002/diff/220001/base/trace_event/blame... base/trace_event/blame_context.h:47: int64_t id); if this is expected to be overridden, I wonder if you want just one ctor with the BlameContext* prent_context, and force clients to just pass nullptr, for the only sake of having to maintain only one ctor in the derived classes. I guess is just a matter of how often you expect this class to be overridden https://codereview.chromium.org/1776673002/diff/220001/base/trace_event/blame... base/trace_event/blame_context.h:49: // Construct a blame context belonging to the blame context tree |name|, using I'd probably not duplicate this comment, and in the above comment just say "If the optional |parent_content| is provided ..." https://codereview.chromium.org/1776673002/diff/220001/base/trace_event/blame... File base/trace_event/blame_context_unittest.cc (right): https://codereview.chromium.org/1776673002/diff/220001/base/trace_event/blame... base/trace_event/blame_context_unittest.cc:125: } shouldn't you also look for the presence of the snapshot here?
Thanks Primiano. Triggering the snapshot should now work in all cases like we discussed. https://codereview.chromium.org/1776673002/diff/220001/base/trace_event/blame... File base/trace_event/blame_context.cc (right): https://codereview.chromium.org/1776673002/diff/220001/base/trace_event/blame... base/trace_event/blame_context.cc:58: num_args, nullptr, nullptr, nullptr, nullptr, On 2016/03/21 14:26:29, Primiano (traveling) wrote: > or just 0 /* num_args */ if you prefer :) Done. https://codereview.chromium.org/1776673002/diff/220001/base/trace_event/blame... base/trace_event/blame_context.cc:76: int num_args = 1; On 2016/03/21 14:26:29, Primiano (traveling) wrote: > nit: kNumArgs? Done. https://codereview.chromium.org/1776673002/diff/220001/base/trace_event/blame... base/trace_event/blame_context.cc:87: TakeSnapshot(); On 2016/03/21 14:26:29, Primiano (traveling) wrote: > What is the reason for this? Is it to make it so that your BlameContext is > always dumped? > IIRC OnTraceLogEnabled is NOT invoked if you register the observer after tracing > has started * > > Does this mean that if you create a BlameContext after tracing has started you > will miss the snapshot? > > * I am looking at my own comment in > https://code.google.com/p/chromium/codesearch#chromium/src/base/trace_event/m... Changed to require users to call Initialize() like we discussed. https://codereview.chromium.org/1776673002/diff/220001/base/trace_event/blame... File base/trace_event/blame_context.h (right): https://codereview.chromium.org/1776673002/diff/220001/base/trace_event/blame... base/trace_event/blame_context.h:42: // strings must have application lifetime. On 2016/03/21 14:26:29, Primiano (traveling) wrote: > Maybe an example of how the actual args to the ctor (up there on lines 21-34 > where you have a sample use case) might help to understand this. Good idea, added. https://codereview.chromium.org/1776673002/diff/220001/base/trace_event/blame... base/trace_event/blame_context.h:47: int64_t id); On 2016/03/21 14:26:29, Primiano (traveling) wrote: > if this is expected to be overridden, I wonder if you want just one ctor with > the BlameContext* prent_context, and force clients to just pass nullptr, for the > only sake of having to maintain only one ctor in the derived classes. > I guess is just a matter of how often you expect this class to be overridden Agreed, I've now combined them (although there was no need for derived classes to expose both constructors unless they needed to). https://codereview.chromium.org/1776673002/diff/220001/base/trace_event/blame... base/trace_event/blame_context.h:49: // Construct a blame context belonging to the blame context tree |name|, using On 2016/03/21 14:26:29, Primiano (traveling) wrote: > I'd probably not duplicate this comment, and in the above comment just say "If > the optional |parent_content| is provided ..." Done. https://codereview.chromium.org/1776673002/diff/220001/base/trace_event/blame... File base/trace_event/blame_context_unittest.cc (right): https://codereview.chromium.org/1776673002/diff/220001/base/trace_event/blame... base/trace_event/blame_context_unittest.cc:125: } On 2016/03/21 14:26:29, Primiano (traveling) wrote: > shouldn't you also look for the presence of the snapshot here? The automatic snapshotting is now covered by the TakeSnapshot test.
LGTM with some micro comments on the last patchset. Thanks for the test! https://codereview.chromium.org/1776673002/diff/230009/base/trace_event/blame... File base/trace_event/blame_context.cc (right): https://codereview.chromium.org/1776673002/diff/230009/base/trace_event/blame... base/trace_event/blame_context.cc:27: #ifndef NDEBUG Should this be just: #if DCHECK_IS_ON() ? and use just DCHECK below when you actually want to check? Otherwise you'll end up relying on the relationship between dcheck and debug=1 which is getting more and more losely copuled (e.g., dcheck_always_on, crrev.com/1814423002). Also that would also make the code more readable https://codereview.chromium.org/1776673002/diff/230009/base/trace_event/blame... base/trace_event/blame_context.cc:69: if (!*category_group_enabled_) Isn't TRACE_EVENT_API_ADD_TRACE_EVENT supposed to make this a no-op for you? Or are you earlying out regardless for performance reasons?
Thanks Primiano -- enjoy the poutine! https://codereview.chromium.org/1776673002/diff/230009/base/trace_event/blame... File base/trace_event/blame_context.cc (right): https://codereview.chromium.org/1776673002/diff/230009/base/trace_event/blame... base/trace_event/blame_context.cc:27: #ifndef NDEBUG On 2016/03/21 17:12:55, Primiano (traveling) wrote: > Should this be just: > #if DCHECK_IS_ON() ? > > and use just DCHECK below when you actually want to check? > Otherwise you'll end up relying on the relationship between dcheck and debug=1 > which is getting more and more losely copuled (e.g., dcheck_always_on, > crrev.com/1814423002). > > Also that would also make the code more readable Good idea, thanks. It turned out I still needed to guard the DCHECKs with DCHECK_IS_ON() since otherwise they won't build in release mode. This looked a little messy so I simplified to just reusing category_group_enabled_ for this. https://codereview.chromium.org/1776673002/diff/230009/base/trace_event/blame... base/trace_event/blame_context.cc:69: if (!*category_group_enabled_) On 2016/03/21 17:12:55, Primiano (traveling) wrote: > Isn't TRACE_EVENT_API_ADD_TRACE_EVENT supposed to make this a no-op for you? > Or are you earlying out regardless for performance reasons? Yes, it'll be a no-op, but I also want to avoid the work of preparing the snapshot.
The CQ bit was checked by skyostil@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from benjhayden@chromium.org, danakj@chromium.org, primiano@chromium.org Link to the patchset: https://codereview.chromium.org/1776673002/#ps270001 (title: "Remove was_initialized.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1776673002/270001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1776673002/270001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by skyostil@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from primiano@chromium.org, benjhayden@chromium.org, danakj@chromium.org Link to the patchset: https://codereview.chromium.org/1776673002/#ps290001 (title: "Windows build fix.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1776673002/290001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1776673002/290001
Message was sent while issue was closed.
Description was changed from ========== base: Add blame context This patch introduces the concept of a blame context, which is a logical entity to which we can attribute various types of costs (e.g., CPU usage, network bandwidth, memory allocations, etc.). A follow-up patch will add a specific blame context for web frames. Design doc: https://docs.google.com/document/d/15BB-suCb9j-nFt55yCFJBJCGzLg2qUm3WaSOPb8APtI BUG=546021 ========== to ========== base: Add blame context This patch introduces the concept of a blame context, which is a logical entity to which we can attribute various types of costs (e.g., CPU usage, network bandwidth, memory allocations, etc.). A follow-up patch will add a specific blame context for web frames. Design doc: https://docs.google.com/document/d/15BB-suCb9j-nFt55yCFJBJCGzLg2qUm3WaSOPb8APtI BUG=546021 ==========
Message was sent while issue was closed.
Committed patchset #16 (id:290001)
Message was sent while issue was closed.
Description was changed from ========== base: Add blame context This patch introduces the concept of a blame context, which is a logical entity to which we can attribute various types of costs (e.g., CPU usage, network bandwidth, memory allocations, etc.). A follow-up patch will add a specific blame context for web frames. Design doc: https://docs.google.com/document/d/15BB-suCb9j-nFt55yCFJBJCGzLg2qUm3WaSOPb8APtI BUG=546021 ========== to ========== base: Add blame context This patch introduces the concept of a blame context, which is a logical entity to which we can attribute various types of costs (e.g., CPU usage, network bandwidth, memory allocations, etc.). A follow-up patch will add a specific blame context for web frames. Design doc: https://docs.google.com/document/d/15BB-suCb9j-nFt55yCFJBJCGzLg2qUm3WaSOPb8APtI BUG=546021 Committed: https://crrev.com/e69ffec748f93302e2174fd434e198dc295e5bf3 Cr-Commit-Position: refs/heads/master@{#382401} ==========
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/e69ffec748f93302e2174fd434e198dc295e5bf3 Cr-Commit-Position: refs/heads/master@{#382401} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
