Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1980743002:
Track thread activities in order to diagnose hangs. (Closed)
|
Created:
4 years, 7 months ago by bcwhite Modified:
4 years, 1 month ago Reviewers:
manzagop (departed), Mark Mentovai, Sigurður Ásgeirsson, Alexei Svitkine (slow), brucedawson CC:
chromium-reviews, asvitkine+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@readwrite-mmf Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionTrack thread activities in order to diagnose hangs.
There are a large number of "unclean shutdowns" in Chrome
with little information available as to what the various
threads of Chrome were doing that prevented a proper
close.
The ActivityTracker class provides a low-overhead way
of keeping track of what each thread is doing in a
persistent manner that can be analyzed both in real-
time and post-mortem.
The ActivityAnalyzer class provides an interface into
the persisted data for analysis. Using that data is
for a future CL.
Operation is controlled by an experiment. Enable
manually with:
--enable-features=StabilityDebugging
BUG=620813
Committed: https://crrev.com/5d3470e5a33aaad936e171bee625b6b978ee3c34
Committed: https://crrev.com/d9705964f525c278a5939b6fcd29a10f732149d0
Cr-Original-Commit-Position: refs/heads/master@{#410470}
Cr-Commit-Position: refs/heads/master@{#410938}
Patch Set 1 #Patch Set 2 : general improvements and scoped activities #
Total comments: 2
Patch Set 3 : moved to base/debug and added FieldTrial enabling #Patch Set 4 : rebased #
Total comments: 61
Patch Set 5 : addressed review comments #
Total comments: 9
Patch Set 6 : addressed review comments #Patch Set 7 : added support for storing to disk #
Total comments: 2
Patch Set 8 : track locks and waitable events #
Total comments: 10
Patch Set 9 : review comments and add fetches for name and time and rename StackEntry to Activity #Patch Set 10 : track thread-join and process-wait; build separate 'analyzer' classes #Patch Set 11 : fixed some build problems #Patch Set 12 : added generic activity tracking #Patch Set 13 : cleaned up for review #
Total comments: 50
Patch Set 14 : more clean-up and addressed review comments #
Total comments: 19
Patch Set 15 : addressed review comments; capture complete stack-trace on SYZYASAN builds #Patch Set 16 : fixed thread-id discovery #
Total comments: 19
Patch Set 17 : restored source/origin tracking #Patch Set 18 : addressed review comments by Bruce #Patch Set 19 : rebased #
Total comments: 76
Patch Set 20 : rebased #Patch Set 21 : address review comments by manzagop #
Total comments: 8
Patch Set 22 : rebased #Patch Set 23 : address review comments by manzagop #
Total comments: 3
Patch Set 24 : rebased #
Total comments: 4
Patch Set 25 : addressed minor review comments; added ownership #
Total comments: 16
Patch Set 26 : addressed review comments #
Total comments: 15
Patch Set 27 : cleaned up feature instantiation; addressed review comments #Patch Set 28 : rebased #Messages
Total messages: 178 (106 generated)
siggi@chromium.org changed reviewers: + siggi@chromium.org
Sorry about tardiness, did not look at implementation in any kind of detail - just looked at the API. https://codereview.chromium.org/1980743002/diff/70001/base/debug/activity_tra... File base/debug/activity_tracker.h (right): https://codereview.chromium.org/1980743002/diff/70001/base/debug/activity_tra... base/debug/activity_tracker.h:26: namespace debug { A general comment on thread identity here. First it's not clear to me that this implementation would ever free an allocated ThreadActivityTracker. This is a problem because now you have ambiguity in the very common case of thread ID reuse. As another comment, thread objects in base are named - it'd be awesome if we could suck these names into the annotations and expose them all the way out to crash. It'd also be worth some amount of trouble to figure out how to "type" threads in these annotations - e.g. is this a thread in the blocking pool, or whatever. Conceptually these would be the bottom-of-stack annotations, though it's not clear that it's possible to mechanically arrange for this - so perhaps the binary data should have a carve-out for this sort of data, and allow writing it at any convenient time? https://codereview.chromium.org/1980743002/diff/70001/base/debug/activity_tra... base/debug/activity_tracker.h:36: class BASE_EXPORT ThreadActivityTracker { I assume each instance of this class has thread affinity, and is not thread-safe? Is this a pure implementation class? Do you need to even expose it in the interface? https://codereview.chromium.org/1980743002/diff/70001/base/debug/activity_tra... base/debug/activity_tracker.h:44: struct StackEntry { nit: the fields warrant a little bit of documentation? https://codereview.chromium.org/1980743002/diff/70001/base/debug/activity_tra... base/debug/activity_tracker.h:46: uint8_t activity_type; I think what we need is an easily extensible tagged union. For e.g. the GPU sync IPC case, we'll want to note the fact that we're doing sync IPC, as well as perhaps the channel, file handle, or the PID of the GPU process. Something that gives us a trail of breadcrumbs to chase after the right process at hang dump time. For post-mortem analysis, it might also be interesting if we can have e.g. the method invoked, as this may help narrow the field. The most important feature we want, however, is agility - agility all the way from here through to human-readable display in the crash UI. Let's think on how you'd build this mechanism such that with a few lines of changes you can get human-readable data from canary the next day. https://codereview.chromium.org/1980743002/diff/70001/base/debug/activity_tra... base/debug/activity_tracker.h:78: void RecordStart(const void* source, RecordXXX implies tracing, but here we're maintaining a stack. PushActivity/PopActivity? https://codereview.chromium.org/1980743002/diff/70001/base/debug/activity_tra... base/debug/activity_tracker.h:90: uint32_t CopyStack(std::vector<StackEntry>* stack); is this ForTesting, or do you foresee this being a useful part of the interface? If this is supposed to allow accessing from another thread, I'd say you want to split the interface into thread affine and multithread-capable, or else there's sure to be trouble in usage. https://codereview.chromium.org/1980743002/diff/70001/base/debug/activity_tra... base/debug/activity_tracker.h:115: class BASE_EXPORT GlobalActivityTracker { so one of these guys manages one per thread of the other guys? Depending on experiment state there's zero or one of these? https://codereview.chromium.org/1980743002/diff/70001/base/debug/activity_tra... base/debug/activity_tracker.h:171: ThreadActivityTracker* GetOrCreateTrackerForCurrentThread() { I dunno about Chromium coding guidelines, but I like separating interface and implementation. You can still define the methods inline at the end of the file. https://codereview.chromium.org/1980743002/diff/70001/base/debug/activity_tra... base/debug/activity_tracker.h:219: class BASE_EXPORT ScopedTaskActivity it looks like this class is the interface to the mechanism. I'd put this class at the top of the file, and consider hiding as much as possible of the rest of the implementation, as there's nothing to gain in exposing it to all and sundry.
On more comment - I'm not sure this is feasible unless we turn this on 100% :). https://codereview.chromium.org/1980743002/diff/70001/base/debug/task_annotat... File base/debug/task_annotator.cc (right): https://codereview.chromium.org/1980743002/diff/70001/base/debug/task_annotat... base/debug/task_annotator.cc:51: const void* program_counter = pending_task.posted_from.program_counter(); can we subsume some or all of the stuff here - queueing time is quite interesting post-mortem, as a case in point?
manzagop@chromium.org changed reviewers: + manzagop@chromium.org
Small comments before starting another pass. https://codereview.chromium.org/1980743002/diff/40001/base/metrics/activity_t... File base/metrics/activity_tracker.h (right): https://codereview.chromium.org/1980743002/diff/40001/base/metrics/activity_t... base/metrics/activity_tracker.h:33: class BASE_EXPORT ThreadActivityTracker { If this is a stack, then does the interface need an argument to RecordFinish? https://codereview.chromium.org/1980743002/diff/40001/base/metrics/activity_t... base/metrics/activity_tracker.h:109: class BASE_EXPORT GlobalActivityTracker { TrackerManager or some such to make it clearer this is not like the other trackers? https://codereview.chromium.org/1980743002/diff/70001/base/debug/activity_tra... File base/debug/activity_tracker.h (right): https://codereview.chromium.org/1980743002/diff/70001/base/debug/activity_tra... base/debug/activity_tracker.h:26: namespace debug { > As another comment, thread objects in base are named - it'd be awesome if we > could suck these names into the annotations and expose them all the way out to > crash. > It'd also be worth some amount of trouble to figure out how to "type" threads in > these annotations - e.g. is this a thread in the blocking pool, or whatever. > Conceptually these would be the bottom-of-stack annotations, though it's not > clear that it's possible to mechanically arrange for this - so perhaps the > binary data should have a carve-out for this sort of data, and allow writing it > at any convenient time? +1 There's also some system wide info that would be nice to have, such as the system state (logging off) or system load information.
This is really cool! I'm not all the way through. High level: - where will we put system-wide state? Should it be part of the same class? - should we split reader / writer interfaces? Also had a few questions/nits. https://codereview.chromium.org/1980743002/diff/70001/base/debug/activity_tra... File base/debug/activity_tracker.cc (right): https://codereview.chromium.org/1980743002/diff/70001/base/debug/activity_tra... base/debug/activity_tracker.cc:21: // A number that indetifies memory has having been initialized. nit: typos https://codereview.chromium.org/1980743002/diff/70001/base/debug/activity_tra... base/debug/activity_tracker.cc:22: const uint64_t kHeaderCookie = 0x98476A390137E67A + 1; // v1 Comment on where this value comes from? https://codereview.chromium.org/1980743002/diff/70001/base/debug/activity_tra... base/debug/activity_tracker.cc:42: const size_t kMemorySize = 1 << 20; // 1 MiB Can you comment on how you chose this number? (k threads * depth of 3 * stack entry size of s?) https://codereview.chromium.org/1980743002/diff/70001/base/debug/activity_tra... base/debug/activity_tracker.cc:52: if (group_name == "InMemory") nit: braces when multiline. https://codereview.chromium.org/1980743002/diff/70001/base/debug/activity_tra... base/debug/activity_tracker.cc:56: NOTREACHED() << group_name; Isn't it better to silently disable, and the person looking at the experiment will notice few users instead of crashes? https://codereview.chromium.org/1980743002/diff/70001/base/debug/activity_tra... base/debug/activity_tracker.cc:81: std::atomic<uint32_t> depth; Is atomic<POD> considered POD? Can there be version shear issues? https://codereview.chromium.org/1980743002/diff/70001/base/debug/activity_tra... base/debug/activity_tracker.cc:89: std::atomic<int> unchanged; int vs bool? https://codereview.chromium.org/1980743002/diff/70001/base/debug/activity_tra... base/debug/activity_tracker.cc:96: slots_((size - sizeof(Header)) / sizeof(StackEntry)) { DCHECK(base)? https://codereview.chromium.org/1980743002/diff/70001/base/debug/activity_tra... base/debug/activity_tracker.cc:198: // It's possible for the data to change while reading it. Make several Nit: add that (only) some of these changes invalidate a read? https://codereview.chromium.org/1980743002/diff/70001/base/debug/activity_tra... base/debug/activity_tracker.cc:207: if (!valid_) The return value is ambiguous wrt the function succeeded. Should revise the interface? https://codereview.chromium.org/1980743002/diff/70001/base/debug/activity_tra... base/debug/activity_tracker.cc:225: // Check to make sure everything was unchanged during the copy. Is there a constraint there may be at most 1 reader? If so, mention it in the class doc. https://codereview.chromium.org/1980743002/diff/70001/base/debug/activity_tra... File base/debug/activity_tracker.h (right): https://codereview.chromium.org/1980743002/diff/70001/base/debug/activity_tra... base/debug/activity_tracker.h:73: // A ThreadActivityTracker runs on top of memory that is managed externally. Mention the precondition on the size (large enough for header and a few entries)? https://codereview.chromium.org/1980743002/diff/70001/base/debug/activity_tra... base/debug/activity_tracker.h:92: // Returns whether the current data is valid or not. Fetching a copy of the When can the data be not valid? Is it only for a reader if the writer died mid-way through initialization? Are you thinking of splitting the reader and the writer? https://codereview.chromium.org/1980743002/diff/70001/base/debug/activity_tra... base/debug/activity_tracker.h:96: // Calculates the memory size required for a given stack depth. nit: clarify this includes the header, eg "required by the tracker". https://codereview.chromium.org/1980743002/diff/70001/base/debug/activity_tra... base/debug/activity_tracker.h:100: friend class Iterator; Is this needed? https://codereview.chromium.org/1980743002/diff/70001/base/debug/activity_tra... base/debug/activity_tracker.h:106: const uint32_t slots_; nit: max_stack_depth_ or stack_slots_ clearer?
Patchset #2 (id:20001) has been deleted
Patchset #5 (id:90001) has been deleted
Patchset #5 (id:110001) has been deleted
https://codereview.chromium.org/1980743002/diff/70001/base/debug/activity_tra... File base/debug/activity_tracker.cc (right): https://codereview.chromium.org/1980743002/diff/70001/base/debug/activity_tra... base/debug/activity_tracker.cc:21: // A number that indetifies memory has having been initialized. On 2016/05/20 18:19:29, manzagop wrote: > nit: typos Done. (I have done zero proof-reading so far.) https://codereview.chromium.org/1980743002/diff/70001/base/debug/activity_tra... base/debug/activity_tracker.cc:22: const uint64_t kHeaderCookie = 0x98476A390137E67A + 1; // v1 On 2016/05/20 18:19:29, manzagop wrote: > Comment on where this value comes from? Done. https://codereview.chromium.org/1980743002/diff/70001/base/debug/activity_tra... base/debug/activity_tracker.cc:42: const size_t kMemorySize = 1 << 20; // 1 MiB On 2016/05/20 18:19:30, manzagop wrote: > Can you comment on how you chose this number? (k threads * depth of 3 * stack > entry size of s?) Acknowledged. https://codereview.chromium.org/1980743002/diff/70001/base/debug/activity_tra... base/debug/activity_tracker.cc:52: if (group_name == "InMemory") On 2016/05/20 18:19:29, manzagop wrote: > nit: braces when multiline. Done. https://codereview.chromium.org/1980743002/diff/70001/base/debug/activity_tra... base/debug/activity_tracker.cc:56: NOTREACHED() << group_name; On 2016/05/20 18:19:30, manzagop wrote: > Isn't it better to silently disable, and the person looking at the experiment > will notice few users instead of crashes? That's what'll happen in a release build. https://codereview.chromium.org/1980743002/diff/70001/base/debug/activity_tra... base/debug/activity_tracker.cc:81: std::atomic<uint32_t> depth; On 2016/05/20 18:19:30, manzagop wrote: > Is atomic<POD> considered POD? Can there be version shear issues? Yes. No. It's exactly the underlying type but with extra access methods. https://codereview.chromium.org/1980743002/diff/70001/base/debug/activity_tra... base/debug/activity_tracker.cc:89: std::atomic<int> unchanged; On 2016/05/20 18:19:30, manzagop wrote: > int vs bool? I chose to use a natural word size so there's no need for the CPU to ever deal with a partial-memory-write. I think x86 is fine with such but other architectures might not like it. https://codereview.chromium.org/1980743002/diff/70001/base/debug/activity_tra... base/debug/activity_tracker.cc:96: slots_((size - sizeof(Header)) / sizeof(StackEntry)) { On 2016/05/20 18:19:29, manzagop wrote: > DCHECK(base)? Done. https://codereview.chromium.org/1980743002/diff/70001/base/debug/activity_tra... base/debug/activity_tracker.cc:198: // It's possible for the data to change while reading it. Make several On 2016/05/20 18:19:30, manzagop wrote: > Nit: add that (only) some of these changes invalidate a read? Done. https://codereview.chromium.org/1980743002/diff/70001/base/debug/activity_tra... base/debug/activity_tracker.cc:207: if (!valid_) On 2016/05/20 18:19:30, manzagop wrote: > The return value is ambiguous wrt the function succeeded. Should revise the > interface? Caller can always check is_valid() first. Returning zero isn't unreasonable for invalid data. https://codereview.chromium.org/1980743002/diff/70001/base/debug/activity_tra... base/debug/activity_tracker.cc:225: // Check to make sure everything was unchanged during the copy. On 2016/05/20 18:19:29, manzagop wrote: > Is there a constraint there may be at most 1 reader? If so, mention it in the > class doc. Good point. Multiple readers would be possible if each set a different bit of "unchanged" but it's not worth the complication. Done. https://codereview.chromium.org/1980743002/diff/70001/base/debug/activity_tra... File base/debug/activity_tracker.h (right): https://codereview.chromium.org/1980743002/diff/70001/base/debug/activity_tra... base/debug/activity_tracker.h:26: namespace debug { > A general comment on thread identity here. First it's not clear to me that this > implementation would ever free an allocated ThreadActivityTracker. My understanding is that the thread-local-storage, when a thread dies, calls the associated destructor (namely OnTLSDestroy()) and free the object. But I haven't written a test for this yet. Okay, now there is a test and the above behavior is confirmed. > As another comment, thread objects in base are named - it'd be awesome if we > could suck these names into the annotations and expose them all the way out to > crash. Done. > It'd also be worth some amount of trouble to figure out how to "type" threads in > these annotations - e.g. is this a thread in the blocking pool, or whatever. > Conceptually these would be the bottom-of-stack annotations, though it's not > clear that it's possible to mechanically arrange for this - so perhaps the > binary data should have a carve-out for this sort of data, and allow writing it > at any convenient time? I'll keep it in mind. https://codereview.chromium.org/1980743002/diff/70001/base/debug/activity_tra... base/debug/activity_tracker.h:26: namespace debug { On 2016/05/20 13:55:42, manzagop wrote: > > As another comment, thread objects in base are named - it'd be awesome if we > > could suck these names into the annotations and expose them all the way out to > > crash. > > It'd also be worth some amount of trouble to figure out how to "type" threads > in > > these annotations - e.g. is this a thread in the blocking pool, or whatever. > > Conceptually these would be the bottom-of-stack annotations, though it's not > > clear that it's possible to mechanically arrange for this - so perhaps the > > binary data should have a carve-out for this sort of data, and allow writing > it > > at any convenient time? > > +1 > > There's also some system wide info that would be nice to have, such as the > system state (logging off) or system load information. That's outside of the scope of this module but I've added a method to retrieve the underlying PersistentMemoryAllocator object. Callers can allocate other objects (with unique type-ids so they can be found and recognized) there to store whatever additional information they want to make available during analysis. https://codereview.chromium.org/1980743002/diff/70001/base/debug/activity_tra... base/debug/activity_tracker.h:36: class BASE_EXPORT ThreadActivityTracker { On 2016/05/20 13:12:05, Sigurður Ásgeirsson wrote: > I assume each instance of this class has thread affinity, and is not > thread-safe? Correct. A ThreadChecker ensures this. > Is this a pure implementation class? Do you need to even expose it > in the interface? I believe it does. If the final result ends otherwise, I'll move it into the .cc file at that time. https://codereview.chromium.org/1980743002/diff/70001/base/debug/activity_tra... base/debug/activity_tracker.h:44: struct StackEntry { On 2016/05/20 13:12:05, Sigurður Ásgeirsson wrote: > nit: the fields warrant a little bit of documentation? Yup. Documentation is definitely unfinished. It'll improve as I work on it but I haven't spent effort on it when I'm not sure if this will even remain looking anything close to what it is at the moment. Hence the "please no nits" request. ;-) https://codereview.chromium.org/1980743002/diff/70001/base/debug/activity_tra... base/debug/activity_tracker.h:46: uint8_t activity_type; > I think what we need is an easily extensible tagged union. I agree. That'll simplify some other places, too, where there are too many parameters being passed. https://codereview.chromium.org/1980743002/diff/70001/base/debug/activity_tra... base/debug/activity_tracker.h:73: // A ThreadActivityTracker runs on top of memory that is managed externally. On 2016/05/20 18:19:30, manzagop wrote: > Mention the precondition on the size (large enough for header and a few > entries)? Done. https://codereview.chromium.org/1980743002/diff/70001/base/debug/activity_tra... base/debug/activity_tracker.h:78: void RecordStart(const void* source, On 2016/05/20 13:12:04, Sigurður Ásgeirsson wrote: > RecordXXX implies tracing, but here we're maintaining a stack. > PushActivity/PopActivity? Done. https://codereview.chromium.org/1980743002/diff/70001/base/debug/activity_tra... base/debug/activity_tracker.h:90: uint32_t CopyStack(std::vector<StackEntry>* stack); On 2016/05/20 13:12:05, Sigurður Ásgeirsson wrote: > is this ForTesting, or do you foresee this being a useful part of the interface? It's not for testing. It's the primary interface for an analysis task to get information about what the thread is doing. SnapshotStack() would be a better name. > If this is supposed to allow accessing from another thread, I'd say you want to > split the interface into thread affine and multithread-capable, or else there's > sure to be trouble in usage. Done. https://codereview.chromium.org/1980743002/diff/70001/base/debug/activity_tra... base/debug/activity_tracker.h:92: // Returns whether the current data is valid or not. Fetching a copy of the On 2016/05/20 18:19:30, manzagop wrote: > When can the data be not valid? Is it only for a reader if the writer died > mid-way through initialization? Are you thinking of splitting the reader and the > writer? Done. https://codereview.chromium.org/1980743002/diff/70001/base/debug/activity_tra... base/debug/activity_tracker.h:96: // Calculates the memory size required for a given stack depth. On 2016/05/20 18:19:30, manzagop wrote: > nit: clarify this includes the header, eg "required by the tracker". Done. https://codereview.chromium.org/1980743002/diff/70001/base/debug/activity_tra... base/debug/activity_tracker.h:100: friend class Iterator; On 2016/05/20 18:19:30, manzagop wrote: > Is this needed? No. The iterator went away in favor of CopyStack(). https://codereview.chromium.org/1980743002/diff/70001/base/debug/activity_tra... base/debug/activity_tracker.h:106: const uint32_t slots_; On 2016/05/20 18:19:30, manzagop wrote: > nit: max_stack_depth_ or stack_slots_ clearer? Done. The "depth" can actually be greater; it's just that no entries will be stored for them. https://codereview.chromium.org/1980743002/diff/70001/base/debug/activity_tra... base/debug/activity_tracker.h:115: class BASE_EXPORT GlobalActivityTracker { On 2016/05/20 13:12:05, Sigurður Ásgeirsson wrote: > so one of these guys manages one per thread of the other guys? > Depending on experiment state there's zero or one of these? Correct. https://codereview.chromium.org/1980743002/diff/70001/base/debug/activity_tra... base/debug/activity_tracker.h:171: ThreadActivityTracker* GetOrCreateTrackerForCurrentThread() { On 2016/05/20 13:12:04, Sigurður Ásgeirsson wrote: > I dunno about Chromium coding guidelines, but I like separating interface and > implementation. You can still define the methods inline at the end of the file. I've seen it done both ways. It was defined inline for optimization purposes but I may undo that. The Scoped*Activity class isn't inline so not sure there's much benefit to doing so here. https://codereview.chromium.org/1980743002/diff/70001/base/debug/activity_tra... base/debug/activity_tracker.h:219: class BASE_EXPORT ScopedTaskActivity On 2016/05/20 13:12:04, Sigurður Ásgeirsson wrote: > it looks like this class is the interface to the mechanism. I'd put this class > at the top of the file, and consider hiding as much as possible of the rest of > the implementation, as there's nothing to gain in exposing it to all and sundry. Acknowledged. https://codereview.chromium.org/1980743002/diff/70001/base/debug/task_annotat... File base/debug/task_annotator.cc (right): https://codereview.chromium.org/1980743002/diff/70001/base/debug/task_annotat... base/debug/task_annotator.cc:51: const void* program_counter = pending_task.posted_from.program_counter(); On 2016/05/20 13:16:53, Sigurður Ásgeirsson wrote: > can we subsume some or all of the stuff here - queueing time is quite > interesting post-mortem, as a case in point? Dunno. Something to consider once we know what it isn't providing.
A few more questions! https://codereview.chromium.org/1980743002/diff/70001/base/debug/activity_tra... File base/debug/activity_tracker.cc (right): https://codereview.chromium.org/1980743002/diff/70001/base/debug/activity_tra... base/debug/activity_tracker.cc:312: DCHECK_LT(stack_memory_, allocator_->GetAllocSize(mem_reference)); Is this not EQ? Or LE? https://codereview.chromium.org/1980743002/diff/70001/base/debug/activity_tra... File base/debug/activity_tracker.h (right): https://codereview.chromium.org/1980743002/diff/70001/base/debug/activity_tra... base/debug/activity_tracker.h:26: namespace debug { On 2016/05/20 19:19:19, bcwhite wrote: > On 2016/05/20 13:55:42, manzagop wrote: > > > As another comment, thread objects in base are named - it'd be awesome if we > > > could suck these names into the annotations and expose them all the way out > to > > > crash. > > > It'd also be worth some amount of trouble to figure out how to "type" > threads > > in > > > these annotations - e.g. is this a thread in the blocking pool, or whatever. > > > Conceptually these would be the bottom-of-stack annotations, though it's not > > > clear that it's possible to mechanically arrange for this - so perhaps the > > > binary data should have a carve-out for this sort of data, and allow writing > > it > > > at any convenient time? > > > > +1 > > > > There's also some system wide info that would be nice to have, such as the > > system state (logging off) or system load information. > > That's outside of the scope of this module but I've added a method to retrieve > the underlying PersistentMemoryAllocator object. Callers can allocate other > objects (with unique type-ids so they can be found and recognized) there to > store whatever additional information they want to make available during > analysis. Great! https://codereview.chromium.org/1980743002/diff/70001/base/debug/activity_tra... base/debug/activity_tracker.h:208: const size_t stack_memory_; nit: stack_memory_size_? https://codereview.chromium.org/1980743002/diff/130001/base/debug/activity_tr... File base/debug/activity_tracker.h (right): https://codereview.chromium.org/1980743002/diff/130001/base/debug/activity_tr... base/debug/activity_tracker.h:101: std::unique_ptr<ThreadActivityAnalyzer> CreateAnalyzer(); I'm not sure we need this, as I don't think trackers and analyzers will ever coexist in the same process? https://codereview.chromium.org/1980743002/diff/130001/base/debug/activity_tr... base/debug/activity_tracker.h:137: class BASE_EXPORT ThreadActivityAnalyzer : private ThreadActivityTracker { Would it be cleaner for the analyzer not to derive from the tracker?
https://codereview.chromium.org/1980743002/diff/70001/base/debug/activity_tra... File base/debug/activity_tracker.cc (right): https://codereview.chromium.org/1980743002/diff/70001/base/debug/activity_tra... base/debug/activity_tracker.cc:312: DCHECK_LT(stack_memory_, allocator_->GetAllocSize(mem_reference)); On 2016/05/20 20:24:21, manzagop wrote: > Is this not EQ? Or LE? The space gets rounded up for alignment reasons so it should be LE. Odd that it didn't crash during testing -- will have to investigate that. https://codereview.chromium.org/1980743002/diff/130001/base/debug/activity_tr... File base/debug/activity_tracker.h (right): https://codereview.chromium.org/1980743002/diff/130001/base/debug/activity_tr... base/debug/activity_tracker.h:101: std::unique_ptr<ThreadActivityAnalyzer> CreateAnalyzer(); On 2016/05/20 20:24:22, manzagop wrote: > I'm not sure we need this, as I don't think trackers and analyzers will ever > coexist in the same process? They do for testing. :-) It could be CreateAnalyzerForTesting() but there didn't seem to be any point to the restriction. https://codereview.chromium.org/1980743002/diff/130001/base/debug/activity_tr... base/debug/activity_tracker.h:137: class BASE_EXPORT ThreadActivityAnalyzer : private ThreadActivityTracker { On 2016/05/20 20:24:22, manzagop wrote: > Would it be cleaner for the analyzer not to derive from the tracker? Perhaps. This re-uses the initialization and validation code which is handy. It could be an embedded class, too and just be a friend of the embedded class to get access to the information.
https://codereview.chromium.org/1980743002/diff/70001/base/debug/activity_tra... File base/debug/activity_tracker.h (right): https://codereview.chromium.org/1980743002/diff/70001/base/debug/activity_tra... base/debug/activity_tracker.h:171: ThreadActivityTracker* GetOrCreateTrackerForCurrentThread() { On 2016/05/20 19:19:19, bcwhite wrote: > On 2016/05/20 13:12:04, Sigurður Ásgeirsson wrote: > > I dunno about Chromium coding guidelines, but I like separating interface and > > implementation. You can still define the methods inline at the end of the > file. > > I've seen it done both ways. It was defined inline for optimization purposes > but I may undo that. Right - I'm saying you can have your cake and eat it by defining the method "inline" in this file, after the class declaration. You don't need to do it inline in the class declaration. https://codereview.chromium.org/1980743002/diff/130001/base/debug/activity_tr... File base/debug/activity_tracker.h (right): https://codereview.chromium.org/1980743002/diff/130001/base/debug/activity_tr... base/debug/activity_tracker.h:101: std::unique_ptr<ThreadActivityAnalyzer> CreateAnalyzer(); On 2016/05/20 20:41:18, bcwhite wrote: > On 2016/05/20 20:24:22, manzagop wrote: > > I'm not sure we need this, as I don't think trackers and analyzers will ever > > coexist in the same process? > > They do for testing. :-) > > It could be CreateAnalyzerForTesting() but there didn't seem to be any point to > the restriction. You generally want to start with the MVI - minimum viable interface. It can grow at need, but it's practically impossible to shrink an interface you've let onto the world, and now you're stuck with maintaining it. https://codereview.chromium.org/1980743002/diff/130001/base/debug/activity_tr... base/debug/activity_tracker.h:137: class BASE_EXPORT ThreadActivityAnalyzer : private ThreadActivityTracker { no private inheritance in C++: https://google.github.io/styleguide/cppguide.html#Inheritance https://codereview.chromium.org/1980743002/diff/130001/base/debug/activity_tr... base/debug/activity_tracker.h:137: class BASE_EXPORT ThreadActivityAnalyzer : private ThreadActivityTracker { On 2016/05/20 20:41:18, bcwhite wrote: > On 2016/05/20 20:24:22, manzagop wrote: > > Would it be cleaner for the analyzer not to derive from the tracker? > > Perhaps. This re-uses the initialization and validation code which is handy. > It could be an embedded class, too and just be a friend of the embedded class to > get access to the information. Private inheritance is verboten in Chromium and Google C++ <https://google.github.io/styleguide/cppguide.html#Inheritance>. For a good reason. Composition and interfaces are to be used, as implementation inheritance comes with a boatload of problems.
https://codereview.chromium.org/1980743002/diff/70001/base/debug/activity_tra... File base/debug/activity_tracker.h (right): https://codereview.chromium.org/1980743002/diff/70001/base/debug/activity_tra... base/debug/activity_tracker.h:171: ThreadActivityTracker* GetOrCreateTrackerForCurrentThread() { On 2016/05/24 14:11:50, Sigurður Ásgeirsson wrote: > On 2016/05/20 19:19:19, bcwhite wrote: > > On 2016/05/20 13:12:04, Sigurður Ásgeirsson wrote: > > > I dunno about Chromium coding guidelines, but I like separating interface > and > > > implementation. You can still define the methods inline at the end of the > > file. > > > > I've seen it done both ways. It was defined inline for optimization purposes > > but I may undo that. > > Right - I'm saying you can have your cake and eat it by defining the method > "inline" in this file, after the class declaration. You don't need to do it > inline in the class declaration. > Understood. I'm saying that I've seen Chromium code do it both in-definition and after-definition. Not sure if there is a policy about it. I'm also saying I may stop doing it inline (in any form) as it may have few benefits; I'll know more as the code develops. https://codereview.chromium.org/1980743002/diff/70001/base/debug/activity_tra... base/debug/activity_tracker.h:208: const size_t stack_memory_; On 2016/05/20 20:24:22, manzagop wrote: > nit: stack_memory_size_? Done. https://codereview.chromium.org/1980743002/diff/130001/base/debug/activity_tr... File base/debug/activity_tracker.h (right): https://codereview.chromium.org/1980743002/diff/130001/base/debug/activity_tr... base/debug/activity_tracker.h:101: std::unique_ptr<ThreadActivityAnalyzer> CreateAnalyzer(); On 2016/05/24 14:11:50, Sigurður Ásgeirsson wrote: > On 2016/05/20 20:41:18, bcwhite wrote: > > On 2016/05/20 20:24:22, manzagop wrote: > > > I'm not sure we need this, as I don't think trackers and analyzers will ever > > > coexist in the same process? > > > > They do for testing. :-) > > > > It could be CreateAnalyzerForTesting() but there didn't seem to be any point > to > > the restriction. > > You generally want to start with the MVI - minimum viable interface. It can grow > at need, but it's practically impossible to shrink an interface you've let onto > the world, and now you're stuck with maintaining it. Done. https://codereview.chromium.org/1980743002/diff/130001/base/debug/activity_tr... base/debug/activity_tracker.h:137: class BASE_EXPORT ThreadActivityAnalyzer : private ThreadActivityTracker { On 2016/05/24 14:11:50, Sigurður Ásgeirsson wrote: > On 2016/05/20 20:41:18, bcwhite wrote: > > On 2016/05/20 20:24:22, manzagop wrote: > > > Would it be cleaner for the analyzer not to derive from the tracker? > > > > Perhaps. This re-uses the initialization and validation code which is handy. > > It could be an embedded class, too and just be a friend of the embedded class > to > > get access to the information. > > Private inheritance is verboten in Chromium and Google C++ > <https://google.github.io/styleguide/cppguide.html#Inheritance>. For a good > reason. > Composition and interfaces are to be used, as implementation inheritance comes > with a boatload of problems. Acknowledged.
Patchset #6 (id:150001) has been deleted
Patchset #6 (id:170001) has been deleted
Patchset #6 (id:190001) has been deleted
Description was changed from ========== Track thread activities in order to diagnose hangs. BUG= ========== to ========== Track thread activities in order to diagnose hangs. --force-fieldtrials=ActivityTracking/InMemory --force-fieldtrials=ActivityTracking/OnDisk BUG= ==========
Description was changed from ========== Track thread activities in order to diagnose hangs. --force-fieldtrials=ActivityTracking/InMemory --force-fieldtrials=ActivityTracking/OnDisk BUG= ========== to ========== Track thread activities in order to diagnose hangs. --enable-features=ActivityTracking BUG= ==========
Patchset #7 (id:230001) has been deleted
Patchset #7 (id:250001) has been deleted
Patchset #7 (id:270001) has been deleted
Patchset #8 (id:310001) has been deleted
Patchset #8 (id:330001) has been deleted
Patchset #8 (id:350001) has been deleted
No comment of substance on the new addition. Were you planning on adding anything to the CL? https://codereview.chromium.org/1980743002/diff/290001/chrome/browser/chrome_... File chrome/browser/chrome_browser_field_trials_desktop.cc (right): https://codereview.chromium.org/1980743002/diff/290001/chrome/browser/chrome_... chrome/browser/chrome_browser_field_trials_desktop.cc:62: base::debug::SetupGlobalActivityTrackerFieldTrial(activity_file); Should this be inside the if? If so, should you add uma metric for failures? https://codereview.chromium.org/1980743002/diff/370001/base/debug/activity_tr... File base/debug/activity_tracker.cc (right): https://codereview.chromium.org/1980743002/diff/370001/base/debug/activity_tr... base/debug/activity_tracker.cc:381: // Failure. This should never happen. It can if we didn't plan for enough memory, right? https://codereview.chromium.org/1980743002/diff/370001/base/debug/activity_tr... base/debug/activity_tracker.cc:494: // Count was successfully incremented to refrect the new value added. typo: reflect https://codereview.chromium.org/1980743002/diff/370001/base/debug/activity_tr... File base/debug/activity_tracker.h (right): https://codereview.chromium.org/1980743002/diff/370001/base/debug/activity_tr... base/debug/activity_tracker.h:30: #if !defined(OS_NACL) // NACL doesn't support any kind of file access in build. Do you need this #if at the callsite in chrome_browser_field_trials_desktop.cc? https://codereview.chromium.org/1980743002/diff/370001/base/debug/activity_tr... base/debug/activity_tracker.h:67: static StackEntryData ForLock(const void* lock) { Add a comment to specificy this is "ForLockAcquisistion"? I wonder how we could handle the lock in addition to the lock acquisition. Doesn't seem to fit in a stack... https://codereview.chromium.org/1980743002/diff/370001/base/synchronization/w... File base/synchronization/waitable_event_win.cc (right): https://codereview.chromium.org/1980743002/diff/370001/base/synchronization/w... base/synchronization/waitable_event_win.cc:81: base::debug::ScopedEventActivity event_activity(events[0]); A comment for this?
https://codereview.chromium.org/1980743002/diff/290001/chrome/browser/chrome_... File chrome/browser/chrome_browser_field_trials_desktop.cc (right): https://codereview.chromium.org/1980743002/diff/290001/chrome/browser/chrome_... chrome/browser/chrome_browser_field_trials_desktop.cc:62: base::debug::SetupGlobalActivityTrackerFieldTrial(activity_file); On 2016/06/01 21:59:40, manzagop wrote: > Should this be inside the if? > If so, should you add uma metric for failures? Get() should always return the same value for a given key at a given place in the code. I'll just turn this into a DCHECK. https://codereview.chromium.org/1980743002/diff/370001/base/debug/activity_tr... File base/debug/activity_tracker.cc (right): https://codereview.chromium.org/1980743002/diff/370001/base/debug/activity_tr... base/debug/activity_tracker.cc:381: // Failure. This should never happen. On 2016/06/01 21:59:40, manzagop wrote: > It can if we didn't plan for enough memory, right? Right. I'll add to the comment. https://codereview.chromium.org/1980743002/diff/370001/base/debug/activity_tr... base/debug/activity_tracker.cc:494: // Count was successfully incremented to refrect the new value added. On 2016/06/01 21:59:41, manzagop wrote: > typo: reflect Done. https://codereview.chromium.org/1980743002/diff/370001/base/debug/activity_tr... File base/debug/activity_tracker.h (right): https://codereview.chromium.org/1980743002/diff/370001/base/debug/activity_tr... base/debug/activity_tracker.h:30: #if !defined(OS_NACL) // NACL doesn't support any kind of file access in build. On 2016/06/01 21:59:41, manzagop wrote: > Do you need this #if at the callsite in chrome_browser_field_trials_desktop.cc? No because that file is under chrome/ which isn't compiled for NaCL while all of base/ is. https://codereview.chromium.org/1980743002/diff/370001/base/debug/activity_tr... base/debug/activity_tracker.h:67: static StackEntryData ForLock(const void* lock) { On 2016/06/01 21:59:41, manzagop wrote: > Add a comment to specificy this is "ForLockAcquisistion"? > > I wonder how we could handle the lock in addition to the lock acquisition. > Doesn't seem to fit in a stack... Yeah, lots of comments required everywhere. The stack can only track those activities which block in some way or take a potentially significant amount of time to run. However, it would be useful to know where a lock/event was unblocked in the past as an indication of a "failure by omission" because it's not happening now. This would be a separate set of information collected aside from the stack. Something to discuss in a group. https://codereview.chromium.org/1980743002/diff/370001/base/synchronization/w... File base/synchronization/waitable_event_win.cc (right): https://codereview.chromium.org/1980743002/diff/370001/base/synchronization/w... base/synchronization/waitable_event_win.cc:81: base::debug::ScopedEventActivity event_activity(events[0]); On 2016/06/01 21:59:41, manzagop wrote: > A comment for this? Done.
Patchset #10 (id:410001) has been deleted
Patchset #10 (id:430001) has been deleted
Patchset #11 (id:470001) has been deleted
Patchset #11 (id:490001) has been deleted
Patchset #12 (id:530001) has been deleted
Patchset #13 (id:570001) has been deleted
Ready for detailed review EXCEPT for activity_analyzer files. Still working on that side.
First batch of comments - will keep reviewing from marker (please ignore :). https://codereview.chromium.org/1980743002/diff/590001/base/debug/activity_an... File base/debug/activity_analyzer.cc (right): https://codereview.chromium.org/1980743002/diff/590001/base/debug/activity_an... base/debug/activity_analyzer.cc:63: uint32_t type; Maybe tuck most of the body of this function into a private Snapshot* member function for readability? https://codereview.chromium.org/1980743002/diff/590001/base/debug/activity_an... base/debug/activity_analyzer.cc:97: analyzers_[analyzer->GetThreadKey()] = std::move(analyzer); not having read the analyzer code, it's not clear to me that GetThreadKey is necessarily unique. What would be the consequences of non-unique keys here? https://codereview.chromium.org/1980743002/diff/590001/base/debug/activity_an... File base/debug/activity_analyzer.h (right): https://codereview.chromium.org/1980743002/diff/590001/base/debug/activity_an... base/debug/activity_analyzer.h:22: class BASE_EXPORT ThreadActivityAnalyzer { describe purpose and perhaps intended usage of class. https://codereview.chromium.org/1980743002/diff/590001/base/debug/activity_an... base/debug/activity_analyzer.h:46: // Creates an analyzer for an existing activity-tracker. The passed tracker nit: |tracker| throughout this comment? https://codereview.chromium.org/1980743002/diff/590001/base/debug/activity_an... base/debug/activity_analyzer.h:51: // Creates an anaylzer for a block of memory currently or previously in-use nit: analyzer https://codereview.chromium.org/1980743002/diff/590001/base/debug/activity_an... base/debug/activity_analyzer.h:57: // Creates an anaylzer for a block of memory held within a persistent-memory nit: analyzer https://codereview.chromium.org/1980743002/diff/590001/base/debug/activity_an... base/debug/activity_analyzer.h:62: ThreadActivityAnalyzer(PersistentMemoryAllocator* allocator, as a general comment on this interface, you have preconditions on at least this constructor. Not having read any further, it's not clear to me whether this precondition can always be met. If not, then perhaps it's better/safer to initialize with an Initialize method, which can then return error? https://codereview.chromium.org/1980743002/diff/590001/base/debug/activity_an... base/debug/activity_analyzer.h:67: // Returns if the contained data is valid. Results from all other methods ubernit: and if contained data is invalid, never returns :) Maybe: Returns true iff ... https://codereview.chromium.org/1980743002/diff/590001/base/debug/activity_an... base/debug/activity_analyzer.h:72: std::string& GetThreadName() { no non-const references. https://codereview.chromium.org/1980743002/diff/590001/base/debug/activity_an... base/debug/activity_analyzer.h:90: PersistentMemoryAllocator::Reference allocator_reference_ = 0; nice - I didn't know we could do this. https://codereview.chromium.org/1980743002/diff/590001/base/debug/activity_an... base/debug/activity_analyzer.h:96: class BASE_EXPORT GlobalActivityAnalyzer { Describe class purpose and intended usage? https://codereview.chromium.org/1980743002/diff/590001/base/debug/activity_an... base/debug/activity_analyzer.h:100: explicit GlobalActivityAnalyzer( document member functions https://codereview.chromium.org/1980743002/diff/590001/base/debug/activity_an... base/debug/activity_analyzer.h:109: ThreadActivityAnalyzer* GetFirstAnalyzer(); document member functions - presumably this class retains ownership throughout? https://codereview.chromium.org/1980743002/diff/590001/base/debug/activity_an... File base/debug/activity_analyzer_unittest.cc (right): https://codereview.chromium.org/1980743002/diff/590001/base/debug/activity_an... base/debug/activity_analyzer_unittest.cc:47: const int kMemorySize = 1 << 10; // 1KiB static? https://codereview.chromium.org/1980743002/diff/590001/base/debug/activity_an... base/debug/activity_analyzer_unittest.cc:78: // TODO(bcwhite): More tests. Are there already cross-process tests? https://codereview.chromium.org/1980743002/diff/590001/base/debug/activity_an... base/debug/activity_analyzer_unittest.cc:93: ThreadActivityAnalyzer* ta1 = analyzer.GetFirstAnalyzer(); I'm not sure this is a safe assumption, given the way we (used to) run tests - if your tests rely on process-global state, they may need configuration or some form of exclusion. https://codereview.chromium.org/1980743002/diff/590001/base/debug/activity_tr... File base/debug/activity_tracker.cc (right): https://codereview.chromium.org/1980743002/diff/590001/base/debug/activity_tr... base/debug/activity_tracker.cc:74: int64_t process_id; if these things share a segment across multiple process lifetimes, then a process_id/thread_id pair is not unique on Windows. On Windows process_id/creation time is guaranteed to uniquely identify a process instance (so long as time goes monotonically forwards). I don't know whether this is a problem here - but assuming e.g. start_time & start_ticks aren't abused, you should have a unique identifier by including those. https://codereview.chromium.org/1980743002/diff/590001/base/debug/activity_tr... base/debug/activity_tracker.cc:87: uint32_t stack_slots; Does the analyzer guard against OOB reads if this is abused? https://codereview.chromium.org/1980743002/diff/590001/base/debug/activity_tr... base/debug/activity_tracker.cc:92: std::atomic<uint32_t> current_depth; if the underlying segment is shared, can I cause another process to OOB-write by abusing this? How about the analyzer, will it OOB if this is abused? https://codereview.chromium.org/1980743002/diff/590001/base/debug/activity_tr... base/debug/activity_tracker.cc:193: uint32_t depth = header_->current_depth.load(std::memory_order_relaxed); It seems you should be able to maintain the current depth as a member variable, and only ever write to the shared segment? This is almost certainly a performance win? https://codereview.chromium.org/1980743002/diff/590001/base/debug/activity_tr... base/debug/activity_tracker.cc:219: void ThreadActivityTracker::ChangeActivity(const void* source, Siggi: Continue from here. https://codereview.chromium.org/1980743002/diff/590001/base/debug/activity_tr... File base/debug/activity_tracker.h (right): https://codereview.chromium.org/1980743002/diff/590001/base/debug/activity_tr... base/debug/activity_tracker.h:45: // a persistent manner. In order to support an operational mode where another it implements a bounded-size stack in a fixed-size allocation. Maybe talk the data format, e.g. header followed stack elements? https://codereview.chromium.org/1980743002/diff/590001/base/debug/activity_tr... base/debug/activity_tracker.h:457: // provide stack-depth requsted during construction. nit: the ... requested
Couple of more comments - still not done. https://codereview.chromium.org/1980743002/diff/590001/base/debug/activity_tr... File base/debug/activity_tracker.cc (right): https://codereview.chromium.org/1980743002/diff/590001/base/debug/activity_tr... base/debug/activity_tracker.cc:334: output_snapshot->thread_name = header_->thread_name; if thread_name is not zero-terminated (abused) this'll OOB - needs more care here. https://codereview.chromium.org/1980743002/diff/590001/base/debug/activity_tr... File base/debug/activity_tracker.h (right): https://codereview.chromium.org/1980743002/diff/590001/base/debug/activity_tr... base/debug/activity_tracker.h:273: bool Snapshot(ActivitySnapshot* output_snapshot) const; this is IMHO out of place in the public interface of this class, as this way the same interface contains both mutation and snapshotting. It's IMHO kind of weird to instantiate the mutator on some other process/thread's data for snapshotting? If it were up to me, I'd push the data management to a *Base class, then use two subclasses to provide the Mutation and Snapshot functionality.
https://codereview.chromium.org/1980743002/diff/590001/base/debug/activity_an... File base/debug/activity_analyzer.cc (right): https://codereview.chromium.org/1980743002/diff/590001/base/debug/activity_an... base/debug/activity_analyzer.cc:63: uint32_t type; On 2016/06/14 15:28:12, Sigurður Ásgeirsson wrote: > Maybe tuck most of the body of this function into a private Snapshot* member > function for readability? Ummm... I specifically said that the activity_analyzer_* code wasn't ready for review.
On Tue, Jun 14, 2016 at 11:54 AM <bcwhite@chromium.org> wrote: > > > https://codereview.chromium.org/1980743002/diff/590001/base/debug/activity_an... > File base/debug/activity_analyzer.cc (right): > > > https://codereview.chromium.org/1980743002/diff/590001/base/debug/activity_an... > base/debug/activity_analyzer.cc:63: uint32_t type; > On 2016/06/14 15:28:12, Sigurður Ásgeirsson wrote: > > Maybe tuck most of the body of this function into a private Snapshot* > member > > function for readability? > > Ummm... I specifically said that the activity_analyzer_* code wasn't > ready for review. > > oops, my bad - will not look no moar :). -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Updates to activity_analyzer files complete. https://codereview.chromium.org/1980743002/diff/590001/base/debug/activity_an... File base/debug/activity_analyzer.cc (right): https://codereview.chromium.org/1980743002/diff/590001/base/debug/activity_an... base/debug/activity_analyzer.cc:63: uint32_t type; On 2016/06/14 15:28:12, Sigurður Ásgeirsson wrote: > Maybe tuck most of the body of this function into a private Snapshot* member > function for readability? Done. https://codereview.chromium.org/1980743002/diff/590001/base/debug/activity_an... base/debug/activity_analyzer.cc:97: analyzers_[analyzer->GetThreadKey()] = std::move(analyzer); On 2016/06/14 15:28:12, Sigurður Ásgeirsson wrote: > not having read the analyzer code, it's not clear to me that GetThreadKey is > necessarily unique. What would be the consequences of non-unique keys here? Only active (currently executing) trackers are included and so there is no duplication. A non-unique key would simply point to the last one found. Confusing, sure, but not fatal. It shouldn't happen. https://codereview.chromium.org/1980743002/diff/590001/base/debug/activity_an... File base/debug/activity_analyzer.h (right): https://codereview.chromium.org/1980743002/diff/590001/base/debug/activity_an... base/debug/activity_analyzer.h:22: class BASE_EXPORT ThreadActivityAnalyzer { On 2016/06/14 15:28:13, Sigurður Ásgeirsson wrote: > describe purpose and perhaps intended usage of class. Done. https://codereview.chromium.org/1980743002/diff/590001/base/debug/activity_an... base/debug/activity_analyzer.h:46: // Creates an analyzer for an existing activity-tracker. The passed tracker On 2016/06/14 15:28:12, Sigurður Ásgeirsson wrote: > nit: |tracker| throughout this comment? Done. https://codereview.chromium.org/1980743002/diff/590001/base/debug/activity_an... base/debug/activity_analyzer.h:51: // Creates an anaylzer for a block of memory currently or previously in-use On 2016/06/14 15:28:12, Sigurður Ásgeirsson wrote: > nit: analyzer Done. https://codereview.chromium.org/1980743002/diff/590001/base/debug/activity_an... base/debug/activity_analyzer.h:57: // Creates an anaylzer for a block of memory held within a persistent-memory On 2016/06/14 15:28:13, Sigurður Ásgeirsson wrote: > nit: analyzer Done. https://codereview.chromium.org/1980743002/diff/590001/base/debug/activity_an... base/debug/activity_analyzer.h:62: ThreadActivityAnalyzer(PersistentMemoryAllocator* allocator, On 2016/06/14 15:28:13, Sigurður Ásgeirsson wrote: > as a general comment on this interface, you have preconditions on at least this > constructor. Not having read any further, it's not clear to me whether this > precondition can always be met. > If not, then perhaps it's better/safer to initialize with an Initialize method, > which can then return error? Like all other constructors, passing bad data will not fail but IsValid() will return false. This is true whether the parameters are bad or the contained data is bad. https://codereview.chromium.org/1980743002/diff/590001/base/debug/activity_an... base/debug/activity_analyzer.h:67: // Returns if the contained data is valid. Results from all other methods On 2016/06/14 15:28:13, Sigurður Ásgeirsson wrote: > ubernit: and if contained data is invalid, never returns :) > > Maybe: Returns true iff ... Done. :-) https://codereview.chromium.org/1980743002/diff/590001/base/debug/activity_an... base/debug/activity_analyzer.h:72: std::string& GetThreadName() { On 2016/06/14 15:28:12, Sigurður Ásgeirsson wrote: > no non-const references. Done. https://codereview.chromium.org/1980743002/diff/590001/base/debug/activity_an... base/debug/activity_analyzer.h:90: PersistentMemoryAllocator::Reference allocator_reference_ = 0; On 2016/06/14 15:28:13, Sigurður Ásgeirsson wrote: > nice - I didn't know we could do this. Inline initialization? New in C++11. Very handy! https://codereview.chromium.org/1980743002/diff/590001/base/debug/activity_an... base/debug/activity_analyzer.h:96: class BASE_EXPORT GlobalActivityAnalyzer { On 2016/06/14 15:28:12, Sigurður Ásgeirsson wrote: > Describe class purpose and intended usage? Done. https://codereview.chromium.org/1980743002/diff/590001/base/debug/activity_an... base/debug/activity_analyzer.h:100: explicit GlobalActivityAnalyzer( On 2016/06/14 15:28:12, Sigurður Ásgeirsson wrote: > document member functions Done. https://codereview.chromium.org/1980743002/diff/590001/base/debug/activity_an... base/debug/activity_analyzer.h:109: ThreadActivityAnalyzer* GetFirstAnalyzer(); On 2016/06/14 15:28:13, Sigurður Ásgeirsson wrote: > document member functions - presumably this class retains ownership throughout? Done. https://codereview.chromium.org/1980743002/diff/590001/base/debug/activity_an... File base/debug/activity_analyzer_unittest.cc (right): https://codereview.chromium.org/1980743002/diff/590001/base/debug/activity_an... base/debug/activity_analyzer_unittest.cc:47: const int kMemorySize = 1 << 10; // 1KiB On 2016/06/14 15:28:13, Sigurður Ásgeirsson wrote: > static? In general it doesn't matter since the compiler will optimize it out given that it's memory address is never taken. The use of "static" beyond that is controversial as though a static will initialize only once, it requires a flag and code to ensure it happens only once. For simple data types, it's generally faster to always do direct store of the constant data every time that test a flag and skip. https://codereview.chromium.org/1980743002/diff/590001/base/debug/activity_an... base/debug/activity_analyzer_unittest.cc:78: // TODO(bcwhite): More tests. On 2016/06/14 15:28:13, Sigurður Ásgeirsson wrote: > Are there already cross-process tests? No but in practice it doesn't matter since there are not static members in the class. Thus, separate threads in the same process can simulate the same conditions. https://codereview.chromium.org/1980743002/diff/590001/base/debug/activity_an... base/debug/activity_analyzer_unittest.cc:93: ThreadActivityAnalyzer* ta1 = analyzer.GetFirstAnalyzer(); On 2016/06/14 15:28:13, Sigurður Ásgeirsson wrote: > I'm not sure this is a safe assumption, given the way we (used to) run tests - > if your tests rely on process-global state, they may need configuration or some > form of exclusion. I'll look into it. https://codereview.chromium.org/1980743002/diff/590001/base/debug/activity_tr... File base/debug/activity_tracker.cc (right): https://codereview.chromium.org/1980743002/diff/590001/base/debug/activity_tr... base/debug/activity_tracker.cc:74: int64_t process_id; On 2016/06/14 15:28:13, Sigurður Ásgeirsson wrote: > if these things share a segment across multiple process lifetimes, then a > process_id/thread_id pair is not unique on Windows. On Windows > process_id/creation time is guaranteed to uniquely identify a process instance > (so long as time goes monotonically forwards). > I don't know whether this is a problem here - but assuming e.g. start_time & > start_ticks aren't abused, you should have a unique identifier by including > those. Right. "Among all *active* trackers". Once a process or thread dies, its data is no longer valid. I don't foresee it being necessary to differentiate between active and exited processes but, as you say, there are ways to resolve that should it become necessary. https://codereview.chromium.org/1980743002/diff/590001/base/debug/activity_tr... base/debug/activity_tracker.cc:87: uint32_t stack_slots; On 2016/06/14 15:28:13, Sigurður Ásgeirsson wrote: > Does the analyzer guard against OOB reads if this is abused? Yes. It validates the data structures during initialization and actually uses its own stack_slots_ member for the limiting. https://codereview.chromium.org/1980743002/diff/590001/base/debug/activity_tr... base/debug/activity_tracker.cc:92: std::atomic<uint32_t> current_depth; On 2016/06/14 15:28:13, Sigurður Ásgeirsson wrote: > if the underlying segment is shared, can I cause another process to OOB-write by > abusing this? How about the analyzer, will it OOB if this is abused? This field is only manipulated by the thread being tracked. Access to stack data during a snapshot (which is read-only) is limited by the stack_slots_ member local to the object taking the snapshot and so can't cross process boundaries. https://codereview.chromium.org/1980743002/diff/590001/base/debug/activity_tr... base/debug/activity_tracker.cc:193: uint32_t depth = header_->current_depth.load(std::memory_order_relaxed); On 2016/06/14 15:28:13, Sigurður Ásgeirsson wrote: > It seems you should be able to maintain the current depth as a member variable, > and only ever write to the shared segment? This is almost certainly a > performance win? I suppose it could... but I really don't like that. It seems like an error waiting to happen. Plus, these are "relaxed" loads which don't add any memory barriers so performance impact should be minimal if any. https://codereview.chromium.org/1980743002/diff/590001/base/debug/activity_tr... base/debug/activity_tracker.cc:334: output_snapshot->thread_name = header_->thread_name; On 2016/06/14 15:53:26, Sigurður Ásgeirsson wrote: > if thread_name is not zero-terminated (abused) this'll OOB - needs more care > here. Done. https://codereview.chromium.org/1980743002/diff/590001/base/debug/activity_tr... File base/debug/activity_tracker.h (right): https://codereview.chromium.org/1980743002/diff/590001/base/debug/activity_tr... base/debug/activity_tracker.h:45: // a persistent manner. In order to support an operational mode where another On 2016/06/14 15:28:13, Sigurður Ásgeirsson wrote: > it implements a bounded-size stack in a fixed-size allocation. Done. https://codereview.chromium.org/1980743002/diff/590001/base/debug/activity_tr... base/debug/activity_tracker.h:273: bool Snapshot(ActivitySnapshot* output_snapshot) const; On 2016/06/14 15:53:26, Sigurður Ásgeirsson wrote: > this is IMHO out of place in the public interface of this class, as this way the > same interface contains both mutation and snapshotting. It's IMHO kind of weird > to instantiate the mutator on some other process/thread's data for snapshotting? > If it were up to me, I'd push the data management to a *Base class, then use two > subclasses to provide the Mutation and Snapshot functionality. You can request a snapshot from the same instance object that is doing the tracking. That seems normal enough. Creating a second tracker on the same memory in order to request a snapshot from within a different process is a bit odd but I don't think it's worth any added complexity to separate it out. https://codereview.chromium.org/1980743002/diff/590001/base/debug/activity_tr... base/debug/activity_tracker.h:457: // provide stack-depth requsted during construction. On 2016/06/14 15:28:13, Sigurður Ásgeirsson wrote: > nit: the ... requested Done.
I'm still looking at the main files. Sending a batch of mostly nits on the peripheral files. https://codereview.chromium.org/1980743002/diff/610001/base/process/process_p... File base/process/process_posix.cc (right): https://codereview.chromium.org/1980743002/diff/610001/base/process/process_p... base/process/process_posix.cc:358: // Record the event that this thread is blocking upon (for hang diagonsis). typo https://codereview.chromium.org/1980743002/diff/610001/base/process/process_w... File base/process/process_win.cc (right): https://codereview.chromium.org/1980743002/diff/610001/base/process/process_w... base/process/process_win.cc:146: // Record the event that this thread is blocking upon (for hang diagonsis). typo! https://codereview.chromium.org/1980743002/diff/610001/base/synchronization/w... File base/synchronization/waitable_event_posix.cc (right): https://codereview.chromium.org/1980743002/diff/610001/base/synchronization/w... base/synchronization/waitable_event_posix.cc:161: // Record the event that this thread is blocking upon (for hang diagonsis). typo: disgonsis, here and below. https://codereview.chromium.org/1980743002/diff/610001/base/synchronization/w... base/synchronization/waitable_event_posix.cc:236: // Record the event that this thread is blocking upon (for hang diagonsis). Make note only considering first event? https://codereview.chromium.org/1980743002/diff/610001/base/synchronization/w... base/synchronization/waitable_event_posix.cc:236: // Record the event that this thread is blocking upon (for hang diagonsis). Can |count| be 0? https://codereview.chromium.org/1980743002/diff/610001/base/synchronization/w... base/synchronization/waitable_event_posix.cc:237: // Extra parenthesis needed so C++ doesn't think this is a function prototype. Dead comment? https://codereview.chromium.org/1980743002/diff/610001/base/synchronization/w... File base/synchronization/waitable_event_win.cc (right): https://codereview.chromium.org/1980743002/diff/610001/base/synchronization/w... base/synchronization/waitable_event_win.cc:51: // Record the event that this thread is blocking upon (for hang diagonsis). typo: diagonsis, here an below. https://codereview.chromium.org/1980743002/diff/610001/base/synchronization/w... base/synchronization/waitable_event_win.cc:87: // Record the event that this thread is blocking upon (for hang diagonsis). Put a note that this is looking at the first event? https://codereview.chromium.org/1980743002/diff/610001/base/synchronization/w... base/synchronization/waitable_event_win.cc:88: base::debug::ScopedEventWaitActivity event_activity(events[0]); Can count be 0? https://codereview.chromium.org/1980743002/diff/610001/base/threading/platfor... File base/threading/platform_thread_posix.cc (right): https://codereview.chromium.org/1980743002/diff/610001/base/threading/platfor... base/threading/platform_thread_posix.cc:206: // Record the event that this thread is blocking upon (for hang diagonsis). typo: diagonsis https://codereview.chromium.org/1980743002/diff/610001/chrome/browser/chrome_... File chrome/browser/chrome_browser_field_trials_desktop.cc (right): https://codereview.chromium.org/1980743002/diff/610001/chrome/browser/chrome_... chrome/browser/chrome_browser_field_trials_desktop.cc:62: activity_file = activity_file.AppendASCII("ActivityTracker"); Is there a "marketing concern" around the name ActivityTracker? We want it to be clear this is Chrome's internal activity, not the user's. Also, tracking has a precise meaning which is about mirroring the internal state. Perhaps a file named DebugState/DebugInfo or some such would communicate more accurately to the user what this is? The communication concern may not extend to the class's name though?
https://codereview.chromium.org/1980743002/diff/610001/base/process/process_p... File base/process/process_posix.cc (right): https://codereview.chromium.org/1980743002/diff/610001/base/process/process_p... base/process/process_posix.cc:358: // Record the event that this thread is blocking upon (for hang diagonsis). On 2016/06/16 15:19:40, manzagop wrote: > typo And of course it would be in a comment copied to multiple files... <sigh> Done here and everywhere. https://codereview.chromium.org/1980743002/diff/610001/base/synchronization/w... File base/synchronization/waitable_event_posix.cc (right): https://codereview.chromium.org/1980743002/diff/610001/base/synchronization/w... base/synchronization/waitable_event_posix.cc:236: // Record the event that this thread is blocking upon (for hang diagonsis). On 2016/06/16 15:19:41, manzagop wrote: > Can |count| be 0? There's a DCHECK(count) below, so no. But I should probably move this var to below that. https://codereview.chromium.org/1980743002/diff/610001/base/synchronization/w... base/synchronization/waitable_event_posix.cc:237: // Extra parenthesis needed so C++ doesn't think this is a function prototype. On 2016/06/16 15:19:40, manzagop wrote: > Dead comment? Done. https://codereview.chromium.org/1980743002/diff/610001/base/synchronization/w... File base/synchronization/waitable_event_win.cc (right): https://codereview.chromium.org/1980743002/diff/610001/base/synchronization/w... base/synchronization/waitable_event_win.cc:87: // Record the event that this thread is blocking upon (for hang diagonsis). On 2016/06/16 15:19:41, manzagop wrote: > Put a note that this is looking at the first event? Done. https://codereview.chromium.org/1980743002/diff/610001/base/synchronization/w... base/synchronization/waitable_event_win.cc:88: base::debug::ScopedEventWaitActivity event_activity(events[0]); On 2016/06/16 15:19:41, manzagop wrote: > Can count be 0? Done. https://codereview.chromium.org/1980743002/diff/610001/chrome/browser/chrome_... File chrome/browser/chrome_browser_field_trials_desktop.cc (right): https://codereview.chromium.org/1980743002/diff/610001/chrome/browser/chrome_... chrome/browser/chrome_browser_field_trials_desktop.cc:62: activity_file = activity_file.AppendASCII("ActivityTracker"); On 2016/06/16 15:19:41, manzagop wrote: > Is there a "marketing concern" around the name ActivityTracker? We want it to be > clear this is Chrome's internal activity, not the user's. Also, tracking has a > precise meaning which is about mirroring the internal state. Good question. I'm assuming that it being under "base::debug" is a sufficient qualifier, plus the comments at the top of the file itself. I can add a comment here, too.
Description was changed from ========== Track thread activities in order to diagnose hangs. --enable-features=ActivityTracking BUG= ========== to ========== Track thread activities in order to diagnose hangs. --enable-features=ActivityTracking BUG=620813 ==========
Patchset #16 (id:650001) has been deleted
bcwhite@chromium.org changed reviewers: + brucedawson@chromium.org
Bruce, you said you would be willing to review some lock-free algorithm code... Would you mind double-checking base/debug/activity_tracker.cc Thanks!
On 2016/06/17 13:58:55, bcwhite wrote: > Bruce, you said you would be willing to review some lock-free algorithm code... > > Would you mind double-checking > base/debug/activity_tracker.cc > > Thanks! Looking now...
A few comments - I'm mostly just concerned about the unpublishing of data. May need to chat on hangouts if all isn't clear. https://codereview.chromium.org/1980743002/diff/670001/base/debug/activity_tr... File base/debug/activity_tracker.cc (right): https://codereview.chromium.org/1980743002/diff/670001/base/debug/activity_tr... base/debug/activity_tracker.cc:196: header_->cookie = kHeaderCookie; Is this supposed to be the last thing initialized? That is, do you depend on this being the last thing initialized so that the cookie == 0 check above guarantees that the entire structure is initialized? If so and if this is touched in a lock-free manner then |cookie| needs to be an atomic type with memory_order type of memory_order_acq_rel (fastest) or memory_order_seq_cst (safest). On 32-bit platforms you would need to have two 32-bit values instead of one 64-bit value, I think. If lock-free is not relevant here then just ignore. https://codereview.chromium.org/1980743002/diff/670001/base/debug/activity_tr... base/debug/activity_tracker.cc:255: header_->current_depth.store(depth + 1, std::memory_order_release); Good analysis. https://codereview.chromium.org/1980743002/diff/670001/base/debug/activity_tr... base/debug/activity_tracker.cc:264: uint32_t depth = header_->current_depth.load(std::memory_order_relaxed); I'm confused as to why this isn't memory_order_acquire. Doesn't this grant access to stack_[depth - 1] and therefore need to occur before? If not then a comment explaining why not would be good. https://codereview.chromium.org/1980743002/diff/670001/base/debug/activity_tr... base/debug/activity_tracker.cc:298: header_->stack_unchanged.store(0, std::memory_order_release); Is any guarantee of the timing of this write needed? There could be an arbitrarily long time from the fetch_sub to when this zero write occurs. Is this okay? In general trying to 'unpublish' data is somewhere between hard and impossible. https://codereview.chromium.org/1980743002/diff/670001/base/debug/activity_tr... base/debug/activity_tracker.cc:357: if (!header_->stack_unchanged.load(std::memory_order_seq_cst)) I think that this is assuming that if the update of current_depth on line 287 has occurred then the zeroing of stack_unchanged on line 298 will also have happened. If so then that is a race condition.
https://codereview.chromium.org/1980743002/diff/670001/base/debug/activity_tr... File base/debug/activity_tracker.cc (right): https://codereview.chromium.org/1980743002/diff/670001/base/debug/activity_tr... base/debug/activity_tracker.cc:196: header_->cookie = kHeaderCookie; On 2016/06/17 22:46:55, brucedawson wrote: > Is this supposed to be the last thing initialized? That is, do you depend on > this being the last thing initialized so that the cookie == 0 check above > guarantees that the entire structure is initialized? > > If so and if this is touched in a lock-free manner then |cookie| needs to be an > atomic type with memory_order type of memory_order_acq_rel (fastest) or > memory_order_seq_cst (safest). > > On 32-bit platforms you would need to have two 32-bit values instead of one > 64-bit value, I think. > > If lock-free is not relevant here then just ignore. Since at this point, no data is shared elsewhere, order doesn't really matter. I can't see the thread crashing somewhere in the middle but the process continuing to run. I just placed it at the end so it matched the description of the field. https://codereview.chromium.org/1980743002/diff/670001/base/debug/activity_tr... base/debug/activity_tracker.cc:255: header_->current_depth.store(depth + 1, std::memory_order_release); On 2016/06/17 22:46:55, brucedawson wrote: > Good analysis. Acknowledged. https://codereview.chromium.org/1980743002/diff/670001/base/debug/activity_tr... base/debug/activity_tracker.cc:264: uint32_t depth = header_->current_depth.load(std::memory_order_relaxed); On 2016/06/17 22:46:55, brucedawson wrote: > I'm confused as to why this isn't memory_order_acquire. Doesn't this grant > access to stack_[depth - 1] and therefore need to occur before? > > If not then a comment explaining why not would be good. My reasoning was that because |depth| is used to determine the address of the reads of Activity data, then the read must complete before any access to that data, thus providing an implicit "acquire". However, it seems wise to make that explicit. This isn't a performance-critical portion of code. https://codereview.chromium.org/1980743002/diff/670001/base/debug/activity_tr... base/debug/activity_tracker.cc:298: header_->stack_unchanged.store(0, std::memory_order_release); On 2016/06/17 22:46:55, brucedawson wrote: > Is any guarantee of the timing of this write needed? There could be an > arbitrarily long time from the fetch_sub to when this zero write occurs. Is this > okay? In general trying to 'unpublish' data is somewhere between hard and > impossible. It's safe because the contents being guarded haven't been changed. The write just needs to happen sometime before the next "push" occurs as that _will_ change the data. Being that only one thread does push/pop, that is guaranteed. I'll add this to the comment. https://codereview.chromium.org/1980743002/diff/670001/base/debug/activity_tr... base/debug/activity_tracker.cc:357: if (!header_->stack_unchanged.load(std::memory_order_seq_cst)) On 2016/06/17 22:46:55, brucedawson wrote: > I think that this is assuming that if the update of current_depth on line 287 > has occurred then the zeroing of stack_unchanged on line 298 will also have > happened. If so then that is a race condition. Possible but not a problem. Depth may have decremented due to a pop during the read and if "unchanged" doesn't get cleared then that decrement will go unnoticed. However, that's okay because the data is all still valid, just slightly out-of-date as though it had been read 1us earlier.
https://codereview.chromium.org/1980743002/diff/670001/base/debug/activity_tr... File base/debug/activity_tracker.cc (right): https://codereview.chromium.org/1980743002/diff/670001/base/debug/activity_tr... base/debug/activity_tracker.cc:196: header_->cookie = kHeaderCookie; Makes sense. https://codereview.chromium.org/1980743002/diff/670001/base/debug/activity_tr... base/debug/activity_tracker.cc:264: uint32_t depth = header_->current_depth.load(std::memory_order_relaxed); The best way to make that explicit would be to use memory_order_consume. Without that the code is technically illegal. This covers the subject quite well: http://preshing.com/20140709/the-purpose-of-memory_order_consume-in-cpp11/ Note that memory_order_consume is slightly trickier than memory_order_acquire. And, memory_order_acquire is free on x86 and x64. https://codereview.chromium.org/1980743002/diff/670001/base/debug/activity_tr... base/debug/activity_tracker.cc:298: header_->stack_unchanged.store(0, std::memory_order_release); Adding to the comment sounds good. It occurs to me that I am not sure of the consequences of having one thread writing to this with sequential consistency and the other thread not. https://codereview.chromium.org/1980743002/diff/670001/base/debug/activity_tr... base/debug/activity_tracker.cc:357: if (!header_->stack_unchanged.load(std::memory_order_seq_cst)) Okay - I think that makes sense. It's too bad that seq_cst is so expensive.
https://codereview.chromium.org/1980743002/diff/670001/base/debug/activity_tr... File base/debug/activity_tracker.cc (right): https://codereview.chromium.org/1980743002/diff/670001/base/debug/activity_tr... base/debug/activity_tracker.cc:264: uint32_t depth = header_->current_depth.load(std::memory_order_relaxed); On 2016/06/21 00:41:44, brucedawson wrote: > The best way to make that explicit would be to use memory_order_consume. Without > that the code is technically illegal. This covers the subject quite well: > > http://preshing.com/20140709/the-purpose-of-memory_order_consume-in-cpp11/ > > Note that memory_order_consume is slightly trickier than memory_order_acquire. > And, memory_order_acquire is free on x86 and x64. Cool. Done. https://codereview.chromium.org/1980743002/diff/670001/base/debug/activity_tr... base/debug/activity_tracker.cc:298: header_->stack_unchanged.store(0, std::memory_order_release); > It occurs to me that I am not sure of the consequences of having one thread > writing to this with sequential consistency and the other thread not. I don't follow. https://codereview.chromium.org/1980743002/diff/670001/base/debug/activity_tr... base/debug/activity_tracker.cc:357: if (!header_->stack_unchanged.load(std::memory_order_seq_cst)) On 2016/06/21 00:41:44, brucedawson wrote: > Okay - I think that makes sense. It's too bad that seq_cst is so expensive. Luckily it is only done by whatever thread is doing analysis so has no direct impact on those threads doing actual work.
I think that activity_tracker.cc lgtm. https://codereview.chromium.org/1980743002/diff/670001/base/debug/activity_tr... File base/debug/activity_tracker.cc (right): https://codereview.chromium.org/1980743002/diff/670001/base/debug/activity_tr... base/debug/activity_tracker.cc:298: header_->stack_unchanged.store(0, std::memory_order_release); With InterlockedIncrement you only get the desired guarantees if you only ever access the memory using Interlocked operations. I'm not sure if the same sort of requirement applies to seq_cst.
https://codereview.chromium.org/1980743002/diff/670001/base/debug/activity_tr... File base/debug/activity_tracker.cc (right): https://codereview.chromium.org/1980743002/diff/670001/base/debug/activity_tr... base/debug/activity_tracker.cc:298: header_->stack_unchanged.store(0, std::memory_order_release); On 2016/06/21 17:16:56, brucedawson wrote: > With InterlockedIncrement you only get the desired guarantees if you only ever > access the memory using Interlocked operations. I'm not sure if the same sort of > requirement applies to seq_cst. Maybe the CST should be just acq_rel? According to the documentation, CST is that plus more so I expect it would be safe.
Description was changed from ========== Track thread activities in order to diagnose hangs. --enable-features=ActivityTracking BUG=620813 ========== to ========== Track thread activities in order to diagnose hangs. There are a large number of "unclean shutdowns" in Chrome with little information available as to what the various threads of Chrome were doing that prevented a proper close. The ActivityTracker class provides a low-overhead way of keeping track of what each thread is doing in a persistent manner that can be analyzed both in real- time and post-mortem. The ActivityAnalyzer class provides an interface into the persisted data for analysis. Using that data is for a future CL. Operation is controlled by an experiment. Enable manually with: --enable-features=ActivityTracking BUG=620813 ==========
bcwhite@chromium.org changed reviewers: + mark@chromium.org
Nice! A few questions. https://codereview.chromium.org/1980743002/diff/610001/chrome/browser/chrome_... File chrome/browser/chrome_browser_field_trials_desktop.cc (right): https://codereview.chromium.org/1980743002/diff/610001/chrome/browser/chrome_... chrome/browser/chrome_browser_field_trials_desktop.cc:62: activity_file = activity_file.AppendASCII("ActivityTracker"); On 2016/06/16 17:04:43, bcwhite wrote: > On 2016/06/16 15:19:41, manzagop wrote: > > Is there a "marketing concern" around the name ActivityTracker? We want it to > be > > clear this is Chrome's internal activity, not the user's. Also, tracking has a > > precise meaning which is about mirroring the internal state. > > Good question. I'm assuming that it being under "base::debug" is a sufficient > qualifier, plus the comments at the top of the file itself. > > I can add a comment here, too. Sgtm for the the code naming. For the file name, I think users finding an 'ActivityTracker' file in their user data dir might wonder what it is, and it would be good to communicate this is debug data. https://codereview.chromium.org/1980743002/diff/730001/base/debug/activity_tr... File base/debug/activity_tracker.cc (right): https://codereview.chromium.org/1980743002/diff/730001/base/debug/activity_tr... base/debug/activity_tracker.cc:7: #include <atomic> nit: already in .h https://codereview.chromium.org/1980743002/diff/730001/base/debug/activity_tr... base/debug/activity_tracker.cc:34: const uint64_t kHeaderCookie = 0xC0029B240D4A3092ULL + 1; // v1 Could we simply use kTypeIdActivityTracker? It seems like the two have the same requirements wrt identity and versioning? https://codereview.chromium.org/1980743002/diff/730001/base/debug/activity_tr... base/debug/activity_tracker.cc:69: // This unique number indicates a valid initialization of the memory. Should this be atomic, so we never get a torn read on 32bits? That might lead to giving up on reading instead of retrying (snapshot early exits if not valid)? https://codereview.chromium.org/1980743002/diff/730001/base/debug/activity_tr... base/debug/activity_tracker.cc:81: // is can be converted to a thread-id with a system call. nit: is can https://codereview.chromium.org/1980743002/diff/730001/base/debug/activity_tr... base/debug/activity_tracker.cc:110: // the contents didn't change while being copied. Mention there can be only one reader to avoid ABA? This should likely be mentioned in the .h as well. https://codereview.chromium.org/1980743002/diff/730001/base/debug/activity_tr... base/debug/activity_tracker.cc:130: header.thread_ref.as_id = 0; // Zero the union in case other is smaller. Do you need a static_assert to ensure the other things are no larger? Ah great, you check it below, at l.166! https://codereview.chromium.org/1980743002/diff/730001/base/debug/activity_tr... base/debug/activity_tracker.cc:201: // This is a file with existing data. Perform basic consistency checks. Is this from when the analyzer shared code with the tracker? No longer valid? Got it: analyzer uses a tracker to snapshot. https://codereview.chromium.org/1980743002/diff/730001/base/debug/activity_tr... base/debug/activity_tracker.cc:272: // Update the information if it is being recorded (i.e. within slot limit). Hm, I'm not sure I follow this. Keep in mind I'm a lock-free newbie. ;) How does |depth| guard the payload? Isn't the reader free to read the topmost activity as it is being changed? This seems different from push / pop, where depth alters the readable range (and unchanged defends against ABA). https://codereview.chromium.org/1980743002/diff/730001/base/debug/activity_tr... base/debug/activity_tracker.cc:287: void ThreadActivityTracker::PopActivity() { thread_checker? https://codereview.chromium.org/1980743002/diff/730001/base/debug/activity_tr... base/debug/activity_tracker.cc:357: output_snapshot->activity_stack.resize(count); Is it better to avoid the allocation while attempting the snapshot (faster snapshot has higher chance of success)? You could preallocate to the max size above, then crop? https://codereview.chromium.org/1980743002/diff/730001/base/debug/activity_tr... base/debug/activity_tracker.cc:385: // the memory reused by a new one. Try again. Doesn't the header need to be guarded as well? Otherwise, the thread's name might be wrong? https://codereview.chromium.org/1980743002/diff/730001/base/debug/activity_tr... File base/debug/activity_tracker.h (right): https://codereview.chromium.org/1980743002/diff/730001/base/debug/activity_tr... base/debug/activity_tracker.h:7: // and after it has terminated. Its primary purpose is to help locate reasons has terminated -> has terminated unexpectedly? https://codereview.chromium.org/1980743002/diff/730001/base/debug/activity_tr... base/debug/activity_tracker.h:14: #include <atomic> Document this is an exception, to avoid spread? https://codereview.chromium.org/1980743002/diff/730001/base/debug/activity_tr... base/debug/activity_tracker.h:115: uint64_t sequence_id; // The sequience identifier of the posted task. typo: sequience https://codereview.chromium.org/1980743002/diff/730001/base/debug/activity_tr... base/debug/activity_tracker.h:191: // Array of program-counters that make up the call stack. Despite the "... that make up the call stack." nit: or the top of it? https://codereview.chromium.org/1980743002/diff/730001/base/debug/activity_tr... base/debug/activity_tracker.h:204: uint8_t padding[7]; Validate the alignment with a static assert on offsetof? Alternatively, is pragma pack used? https://codereview.chromium.org/1980743002/diff/730001/base/debug/activity_tr... base/debug/activity_tracker.h:230: // The current set of activities that are underway for this thread. It is set -> stack? https://codereview.chromium.org/1980743002/diff/730001/base/debug/activity_tr... base/debug/activity_tracker.h:288: void ChangeActivity(ActivityType type, const ActivityData& data); I'm not sure I see when this comes in handy. And the origin stays the same? https://codereview.chromium.org/1980743002/diff/730001/base/debug/activity_tr... base/debug/activity_tracker.h:342: kTypeIdActivityTrackerFree = 0x3F0272FB + 1, // SHA1(ActivityTrackerFree) Out of curiosity, will the Free type ever change version number? The version number encodes the size of the free block, but I guess that's already known by the allocator's metadata? Unless it's for reuse? https://codereview.chromium.org/1980743002/diff/730001/base/debug/activity_tr... base/debug/activity_tracker.h:364: // allowed (because a lock is being tracked which would cause recursion) Is this still the case? From a quick look below, it looks like creating a tracker might now be lock free? Ah, it's for the thread checker? Maybe a comment? https://codereview.chromium.org/1980743002/diff/730001/base/debug/activity_tr... base/debug/activity_tracker.h:392: // for referece by whatever process reads it. typo: referece https://codereview.chromium.org/1980743002/diff/730001/base/debug/activity_tr... base/debug/activity_tracker.h:444: // If more than this number gets created, tracking of them may cease. For extra clarity: gets created -> run concurrently? https://codereview.chromium.org/1980743002/diff/730001/base/debug/activity_tr... base/debug/activity_tracker.h:457: // The reference into persistent memory from which this the tread-tracker's nit: from which this the https://codereview.chromium.org/1980743002/diff/730001/base/debug/activity_tr... base/debug/activity_tracker.h:467: // created object is activated so tracking will begin immediately upon return. nit: will begin immediately upon return -> is already started on return. https://codereview.chromium.org/1980743002/diff/730001/base/debug/activity_tr... base/debug/activity_tracker.h:485: // provide the stack-depth requsted during construction. typo: requsted https://codereview.chromium.org/1980743002/diff/730001/base/debug/activity_tr... base/debug/activity_tracker.h:499: static GlobalActivityTracker* g_tracker_; I'm guessing we can assume writing a pointer is atomic (no old platforms)?
Nice! Just one more question and some nits. https://codereview.chromium.org/1980743002/diff/730001/base/debug/activity_an... File base/debug/activity_analyzer.h (right): https://codereview.chromium.org/1980743002/diff/730001/base/debug/activity_an... base/debug/activity_analyzer.h:96: // This class manages analyzers for all threads of a process as stored in a nit: "all threads of a process" vs below "all known processes and threads" https://codereview.chromium.org/1980743002/diff/730001/base/debug/activity_an... base/debug/activity_analyzer.h:97: // persistent memory allocator. It supports retreival of them through iteration typo: retreival https://codereview.chromium.org/1980743002/diff/730001/base/debug/activity_an... base/debug/activity_analyzer.h:99: // resolved. Perhaps mention the snapshots may not be consistent with each other if the processes are live? https://codereview.chromium.org/1980743002/diff/730001/base/debug/activity_an... base/debug/activity_analyzer.h:131: // Finds, creates, and indexes analyzers for all known procsses and threads. typo: procsses https://codereview.chromium.org/1980743002/diff/730001/base/debug/activity_tr... File base/debug/activity_tracker.cc (right): https://codereview.chromium.org/1980743002/diff/730001/base/debug/activity_tr... base/debug/activity_tracker.cc:491: int count = available_memories_count_.load(std::memory_order_acquire); I wonder if there can be an ABA issue here? Suppose you have 1 available memory. T1 (interrupted pop) - loads the count - copies the reference R1 - <suspends> T2 (pop) - loads the count - copies the reference R1 - lowers the count to 0 - set the reference to 0 T3 (push) - loads the count (0) - writes the ref R2 - increments the count back to 1 T1 resumes, hits ABA thinking he has R1 - lowers the count back to 0 - set the reference to 0 https://codereview.chromium.org/1980743002/diff/730001/base/debug/activity_tr... base/debug/activity_tracker.cc:549: "UMA.ActivityTracker.ThreadTrackers.MemLimit", nit: MemLimit sounds like it measures memory (eg in MB). MemLimitTrackerCount? https://codereview.chromium.org/1980743002/diff/730001/base/debug/activity_tr... base/debug/activity_tracker.cc:564: old_count + 1, kMaxThreadCount); Is it a problem that old_count can be > kMaxThreadCount?
https://codereview.chromium.org/1980743002/diff/730001/base/debug/activity_an... File base/debug/activity_analyzer.h (right): https://codereview.chromium.org/1980743002/diff/730001/base/debug/activity_an... base/debug/activity_analyzer.h:96: // This class manages analyzers for all threads of a process as stored in a On 2016/07/04 21:20:56, manzagop wrote: > nit: "all threads of a process" vs below "all known processes and threads" Done. https://codereview.chromium.org/1980743002/diff/730001/base/debug/activity_an... base/debug/activity_analyzer.h:97: // persistent memory allocator. It supports retreival of them through iteration On 2016/07/04 21:20:56, manzagop wrote: > typo: retreival Done. https://codereview.chromium.org/1980743002/diff/730001/base/debug/activity_an... base/debug/activity_analyzer.h:99: // resolved. On 2016/07/04 21:20:56, manzagop wrote: > Perhaps mention the snapshots may not be consistent with each other if the > processes are live? Done. https://codereview.chromium.org/1980743002/diff/730001/base/debug/activity_an... base/debug/activity_analyzer.h:131: // Finds, creates, and indexes analyzers for all known procsses and threads. On 2016/07/04 21:20:56, manzagop wrote: > typo: procsses Done. https://codereview.chromium.org/1980743002/diff/730001/base/debug/activity_tr... File base/debug/activity_tracker.cc (right): https://codereview.chromium.org/1980743002/diff/730001/base/debug/activity_tr... base/debug/activity_tracker.cc:491: int count = available_memories_count_.load(std::memory_order_acquire); On 2016/07/04 21:20:56, manzagop wrote: > I wonder if there can be an ABA issue here? > > Suppose you have 1 available memory. > > T1 (interrupted pop) > - loads the count > - copies the reference R1 > - <suspends> > > T2 (pop) > - loads the count > - copies the reference R1 > - lowers the count to 0 > - set the reference to 0 > > T3 (push) > - loads the count (0) > - writes the ref R2 > - increments the count back to 1 > > T1 resumes, hits ABA thinking he has R1 > - lowers the count back to 0 > - set the reference to 0 Good observation! Fixed. (I think. ;-) https://codereview.chromium.org/1980743002/diff/730001/base/debug/activity_tr... base/debug/activity_tracker.cc:549: "UMA.ActivityTracker.ThreadTrackers.MemLimit", On 2016/07/04 21:20:56, manzagop wrote: > nit: MemLimit sounds like it measures memory (eg in MB). MemLimitTrackerCount? Done. https://codereview.chromium.org/1980743002/diff/730001/base/debug/activity_tr... base/debug/activity_tracker.cc:564: old_count + 1, kMaxThreadCount); On 2016/07/04 21:20:56, manzagop wrote: > Is it a problem that old_count can be > kMaxThreadCount? Histograms have an "overflow" bucket for anything too large.
https://codereview.chromium.org/1980743002/diff/610001/chrome/browser/chrome_... File chrome/browser/chrome_browser_field_trials_desktop.cc (right): https://codereview.chromium.org/1980743002/diff/610001/chrome/browser/chrome_... chrome/browser/chrome_browser_field_trials_desktop.cc:62: activity_file = activity_file.AppendASCII("ActivityTracker"); On 2016/07/01 18:26:24, manzagop wrote: > On 2016/06/16 17:04:43, bcwhite wrote: > > On 2016/06/16 15:19:41, manzagop wrote: > > > Is there a "marketing concern" around the name ActivityTracker? We want it > to > > be > > > clear this is Chrome's internal activity, not the user's. Also, tracking has > a > > > precise meaning which is about mirroring the internal state. > > > > Good question. I'm assuming that it being under "base::debug" is a sufficient > > qualifier, plus the comments at the top of the file itself. > > > > I can add a comment here, too. > > Sgtm for the the code naming. > > For the file name, I think users finding an 'ActivityTracker' file in their user > data dir might wonder what it is, and it would be good to communicate this is > debug data. Gotcha. Done. https://codereview.chromium.org/1980743002/diff/730001/base/debug/activity_tr... File base/debug/activity_tracker.cc (right): https://codereview.chromium.org/1980743002/diff/730001/base/debug/activity_tr... base/debug/activity_tracker.cc:7: #include <atomic> On 2016/07/01 18:26:25, manzagop wrote: > nit: already in .h Done. https://codereview.chromium.org/1980743002/diff/730001/base/debug/activity_tr... base/debug/activity_tracker.cc:34: const uint64_t kHeaderCookie = 0xC0029B240D4A3092ULL + 1; // v1 On 2016/07/01 18:26:25, manzagop wrote: > Could we simply use kTypeIdActivityTracker? It seems like the two have the same > requirements wrt identity and versioning? Not really. That identifier is part of the GlobalActivityTracker and it's management of memory within a persistent allocator. This is used by the ThreadActivityTracker which operates only on a block of memory given to it during construction. The class using this value doesn't (and shouldn't) be accessing the Global one. https://codereview.chromium.org/1980743002/diff/730001/base/debug/activity_tr... base/debug/activity_tracker.cc:69: // This unique number indicates a valid initialization of the memory. On 2016/07/01 18:26:25, manzagop wrote: > Should this be atomic, so we never get a torn read on 32bits? That might lead to > giving up on reading instead of retrying (snapshot early exits if not valid)? Tearing is fine on a value that is only written once before it is ever shared. https://codereview.chromium.org/1980743002/diff/730001/base/debug/activity_tr... base/debug/activity_tracker.cc:81: // is can be converted to a thread-id with a system call. On 2016/07/01 18:26:25, manzagop wrote: > nit: is can Done. https://codereview.chromium.org/1980743002/diff/730001/base/debug/activity_tr... base/debug/activity_tracker.cc:110: // the contents didn't change while being copied. On 2016/07/01 18:26:25, manzagop wrote: > Mention there can be only one reader to avoid ABA? This should likely be > mentioned in the .h as well. It actually could support it if each snapshot operation were to set a different bit of this number though that is not the current implementation. Comments added. https://codereview.chromium.org/1980743002/diff/730001/base/debug/activity_tr... base/debug/activity_tracker.cc:272: // Update the information if it is being recorded (i.e. within slot limit). On 2016/07/01 18:26:25, manzagop wrote: > Hm, I'm not sure I follow this. Keep in mind I'm a lock-free newbie. ;) > > How does |depth| guard the payload? Isn't the reader free to read the topmost > activity as it is being changed? > > This seems different from push / pop, where depth alters the readable range (and > unchanged defends against ABA). Right. The update of the information is not atomic and a snapshot could catch it in the middle of an update. I'll add a comment. https://codereview.chromium.org/1980743002/diff/730001/base/debug/activity_tr... base/debug/activity_tracker.cc:287: void ThreadActivityTracker::PopActivity() { On 2016/07/01 18:26:25, manzagop wrote: > thread_checker? Done, though thread-checking is complicated due to the way it uses locks that cause re-entry into this code. https://codereview.chromium.org/1980743002/diff/730001/base/debug/activity_tr... base/debug/activity_tracker.cc:357: output_snapshot->activity_stack.resize(count); On 2016/07/01 18:26:25, manzagop wrote: > Is it better to avoid the allocation while attempting the snapshot (faster > snapshot has higher chance of success)? You could preallocate to the max size > above, then crop? Done. https://codereview.chromium.org/1980743002/diff/730001/base/debug/activity_tr... base/debug/activity_tracker.cc:385: // the memory reused by a new one. Try again. On 2016/07/01 18:26:25, manzagop wrote: > Doesn't the header need to be guarded as well? Otherwise, the thread's name > might be wrong? Not sure what you mean. Memory gets zeroed when a thread exits and loaded with new values when its reused. The name can't be anything but empty or valid for the current pid/tid. https://codereview.chromium.org/1980743002/diff/730001/base/debug/activity_tr... File base/debug/activity_tracker.h (right): https://codereview.chromium.org/1980743002/diff/730001/base/debug/activity_tr... base/debug/activity_tracker.h:7: // and after it has terminated. Its primary purpose is to help locate reasons On 2016/07/01 18:26:25, manzagop wrote: > has terminated -> has terminated unexpectedly? Done. https://codereview.chromium.org/1980743002/diff/730001/base/debug/activity_tr... base/debug/activity_tracker.h:14: #include <atomic> On 2016/07/01 18:26:26, manzagop wrote: > Document this is an exception, to avoid spread? Done. https://codereview.chromium.org/1980743002/diff/730001/base/debug/activity_tr... base/debug/activity_tracker.h:115: uint64_t sequence_id; // The sequience identifier of the posted task. On 2016/07/01 18:26:25, manzagop wrote: > typo: sequience Done. https://codereview.chromium.org/1980743002/diff/730001/base/debug/activity_tr... base/debug/activity_tracker.h:191: // Array of program-counters that make up the call stack. Despite the On 2016/07/01 18:26:26, manzagop wrote: > "... that make up the call stack." > nit: or the top of it? Done. https://codereview.chromium.org/1980743002/diff/730001/base/debug/activity_tr... base/debug/activity_tracker.h:204: uint8_t padding[7]; On 2016/07/01 18:26:26, manzagop wrote: > Validate the alignment with a static assert on offsetof? Alternatively, is > pragma pack used? Done. https://codereview.chromium.org/1980743002/diff/730001/base/debug/activity_tr... base/debug/activity_tracker.h:230: // The current set of activities that are underway for this thread. It is On 2016/07/01 18:26:25, manzagop wrote: > set -> stack? Done. https://codereview.chromium.org/1980743002/diff/730001/base/debug/activity_tr... base/debug/activity_tracker.h:288: void ChangeActivity(ActivityType type, const ActivityData& data); On 2016/07/01 18:26:25, manzagop wrote: > I'm not sure I see when this comes in handy. And the origin stays the same? It's more for manual debugging. If someone is trying to narrow down where things are hanging within a method, they can create a scoped tracker but update it with new information as the code progresses. https://codereview.chromium.org/1980743002/diff/730001/base/debug/activity_tr... base/debug/activity_tracker.h:342: kTypeIdActivityTrackerFree = 0x3F0272FB + 1, // SHA1(ActivityTrackerFree) On 2016/07/01 18:26:25, manzagop wrote: > Out of curiosity, will the Free type ever change version number? The version > number encodes the size of the free block, but I guess that's already known by > the allocator's metadata? Unless it's for reuse? It didn't have a version number originally but if it the size changes then any records for older versions might not be big enough. Yes, a separate check on the size would be possible but it seemed easier to just make sure bad-sized free blocks won't be found. https://codereview.chromium.org/1980743002/diff/730001/base/debug/activity_tr... base/debug/activity_tracker.h:364: // allowed (because a lock is being tracked which would cause recursion) On 2016/07/01 18:26:26, manzagop wrote: > Is this still the case? From a quick look below, it looks like creating a > tracker might now be lock free? > > Ah, it's for the thread checker? Maybe a comment? How about I just remove the thread-tracker completely? It causes more problems than it helps, I think. https://codereview.chromium.org/1980743002/diff/730001/base/debug/activity_tr... base/debug/activity_tracker.h:392: // for referece by whatever process reads it. On 2016/07/01 18:26:26, manzagop wrote: > typo: referece Done. https://codereview.chromium.org/1980743002/diff/730001/base/debug/activity_tr... base/debug/activity_tracker.h:444: // If more than this number gets created, tracking of them may cease. On 2016/07/01 18:26:25, manzagop wrote: > For extra clarity: gets created -> run concurrently? Done. https://codereview.chromium.org/1980743002/diff/730001/base/debug/activity_tr... base/debug/activity_tracker.h:457: // The reference into persistent memory from which this the tread-tracker's On 2016/07/01 18:26:26, manzagop wrote: > nit: from which this the You missed "tread-tracker" typo. :-) https://codereview.chromium.org/1980743002/diff/730001/base/debug/activity_tr... base/debug/activity_tracker.h:467: // created object is activated so tracking will begin immediately upon return. On 2016/07/01 18:26:25, manzagop wrote: > nit: will begin immediately upon return -> is already started on return. Done. https://codereview.chromium.org/1980743002/diff/730001/base/debug/activity_tr... base/debug/activity_tracker.h:485: // provide the stack-depth requsted during construction. On 2016/07/01 18:26:26, manzagop wrote: > typo: requsted Done. https://codereview.chromium.org/1980743002/diff/730001/base/debug/activity_tr... base/debug/activity_tracker.h:499: static GlobalActivityTracker* g_tracker_; On 2016/07/01 18:26:26, manzagop wrote: > I'm guessing we can assume writing a pointer is atomic (no old platforms)? A std::atomic<GlobalActivityTracker*> would be better but that causes problems with current compilers. But yes, given that it's a natural word there's no reason why it won't write atomically even if doesn't necessary flush out to physical memory until some time later. At worst, activation will get delayed a few microseconds.
Patchset #21 (id:770001) has been deleted
The CQ bit was checked by bcwhite@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: This issue passed the CQ dry run.
The CQ bit was checked by bcwhite@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: This issue passed the CQ dry run.
The CQ bit was checked by bcwhite@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: This issue passed the CQ dry run.
The CQ bit was checked by bcwhite@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 checked by bcwhite@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: This issue passed the CQ dry run.
Cool! LGTM! https://codereview.chromium.org/1980743002/diff/730001/base/debug/activity_tr... File base/debug/activity_tracker.cc (right): https://codereview.chromium.org/1980743002/diff/730001/base/debug/activity_tr... base/debug/activity_tracker.cc:385: // the memory reused by a new one. Try again. On 2016/07/11 22:03:29, bcwhite wrote: > On 2016/07/01 18:26:25, manzagop wrote: > > Doesn't the header need to be guarded as well? Otherwise, the thread's name > > might be wrong? > > Not sure what you mean. Memory gets zeroed when a thread exits and loaded with > new values when its reused. The name can't be anything but empty or valid for > the current pid/tid. I *think* I meant that you can read a wrong/empty name if the thread exits in between reading thread_id and thread_name. It's also possible if it exits after loading stack_unchanged and pid+tid get reused (very unlucky). https://codereview.chromium.org/1980743002/diff/730001/base/debug/activity_tr... base/debug/activity_tracker.cc:491: int count = available_memories_count_.load(std::memory_order_acquire); On 2016/07/11 15:24:18, bcwhite wrote: > On 2016/07/04 21:20:56, manzagop wrote: > > I wonder if there can be an ABA issue here? > > > > Suppose you have 1 available memory. > > > > T1 (interrupted pop) > > - loads the count > > - copies the reference R1 > > - <suspends> > > > > T2 (pop) > > - loads the count > > - copies the reference R1 > > - lowers the count to 0 > > - set the reference to 0 > > > > T3 (push) > > - loads the count (0) > > - writes the ref R2 > > - increments the count back to 1 > > > > T1 resumes, hits ABA thinking he has R1 > > - lowers the count back to 0 > > - set the reference to 0 > > Good observation! Fixed. (I think. ;-) Looks good! https://codereview.chromium.org/1980743002/diff/730001/base/debug/activity_tr... File base/debug/activity_tracker.h (right): https://codereview.chromium.org/1980743002/diff/730001/base/debug/activity_tr... base/debug/activity_tracker.h:12: #define BASE_METRICS_ACTIVITY_TRACKER_H_ nit: BASE_DEBUG_...? https://codereview.chromium.org/1980743002/diff/730001/base/debug/activity_tr... base/debug/activity_tracker.h:15: #include <memory> nit: include string and vector https://codereview.chromium.org/1980743002/diff/730001/base/debug/activity_tr... base/debug/activity_tracker.h:457: // The reference into persistent memory from which this the tread-tracker's On 2016/07/11 22:03:30, bcwhite wrote: > On 2016/07/01 18:26:26, manzagop wrote: > > nit: from which this the > > You missed "tread-tracker" typo. :-) Darn. I'll have to reset my "days since typo missed" counter! ;) https://codereview.chromium.org/1980743002/diff/730001/base/debug/activity_tr... base/debug/activity_tracker.h:551: ScopedTaskRunActivity(const base::PendingTask& task); nit: explicit? And below. https://codereview.chromium.org/1980743002/diff/790001/base/debug/activity_tr... File base/debug/activity_tracker.cc (right): https://codereview.chromium.org/1980743002/diff/790001/base/debug/activity_tr... base/debug/activity_tracker.cc:348: output_snapshot->activity_stack.resize(stack_slots_); nit: use reserve for a clearer intent? https://codereview.chromium.org/1980743002/diff/790001/base/debug/activity_tr... base/debug/activity_tracker.cc:371: output_snapshot->activity_stack.resize(count); Resize before the copy so we never copy outside the vector's "active" range in a case where the depth increases between 2 attempts? As is works (resizing never reduces the capacity), but resizing before seems more correct. https://codereview.chromium.org/1980743002/diff/790001/base/debug/activity_tr... base/debug/activity_tracker.cc:545: allocator_->ChangeType(mem_reference, kTypeIdActivityTracker, DCHECK this returns true?
Patchset #22 (id:810001) has been deleted
Patchset #22 (id:830001) has been deleted
Patchset #22 (id:850001) has been deleted
Patchset #22 (id:870001) has been deleted
address review comments by manzagop
The CQ bit was checked by bcwhite@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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
The CQ bit was checked by bcwhite@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 checked by bcwhite@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: This issue passed the CQ dry run.
Patchset #24 (id:930001) has been deleted
https://codereview.chromium.org/1980743002/diff/730001/base/debug/activity_tr... File base/debug/activity_tracker.cc (right): https://codereview.chromium.org/1980743002/diff/730001/base/debug/activity_tr... base/debug/activity_tracker.cc:385: // the memory reused by a new one. Try again. > > > Doesn't the header need to be guarded as well? Otherwise, the thread's name > > > might be wrong? > > > > Not sure what you mean. Memory gets zeroed when a thread exits and loaded > with > > new values when its reused. The name can't be anything but empty or valid for > > the current pid/tid. > > I *think* I meant that you can read a wrong/empty name if the thread exits in > between reading thread_id and thread_name. Yes but then the pid/tid comparison on 386 would fail and restart the loop. Of course... There's currently no guarantee that access to those values can't be reordered by the cpu or compiler. Looks like I need an atomic for the IDs so I can insert a memory barrier. <sigh> > It's also possible if it exits after loading stack_unchanged and pid+tid get > reused (very unlucky). I think tid is unique across the lifetime of a given pid... Though I went through so many iteration of tid types that I could be wrong. It's still not perfect, but it should be enough to deal with the rare race condition. https://codereview.chromium.org/1980743002/diff/730001/base/debug/activity_tr... File base/debug/activity_tracker.h (right): https://codereview.chromium.org/1980743002/diff/730001/base/debug/activity_tr... base/debug/activity_tracker.h:12: #define BASE_METRICS_ACTIVITY_TRACKER_H_ On 2016/07/26 21:25:33, manzagop wrote: > nit: BASE_DEBUG_...? Done. https://codereview.chromium.org/1980743002/diff/730001/base/debug/activity_tr... base/debug/activity_tracker.h:15: #include <memory> On 2016/07/26 21:25:33, manzagop wrote: > nit: include string and vector Done. https://codereview.chromium.org/1980743002/diff/730001/base/debug/activity_tr... base/debug/activity_tracker.h:551: ScopedTaskRunActivity(const base::PendingTask& task); On 2016/07/26 21:25:33, manzagop wrote: > nit: explicit? And below. Done. https://codereview.chromium.org/1980743002/diff/790001/base/debug/activity_tr... File base/debug/activity_tracker.cc (right): https://codereview.chromium.org/1980743002/diff/790001/base/debug/activity_tr... base/debug/activity_tracker.cc:348: output_snapshot->activity_stack.resize(stack_slots_); On 2016/07/26 21:25:34, manzagop wrote: > nit: use reserve for a clearer intent? Done. https://codereview.chromium.org/1980743002/diff/790001/base/debug/activity_tr... base/debug/activity_tracker.cc:371: output_snapshot->activity_stack.resize(count); On 2016/07/26 21:25:34, manzagop wrote: > Resize before the copy so we never copy outside the vector's "active" range in a > case where the depth increases between 2 attempts? > > As is works (resizing never reduces the capacity), but resizing before seems > more correct. Done, though it was previously resized/reserved to the maximum possible size so it could only resize smaller at this point. https://codereview.chromium.org/1980743002/diff/790001/base/debug/activity_tr... base/debug/activity_tracker.cc:545: allocator_->ChangeType(mem_reference, kTypeIdActivityTracker, On 2016/07/26 21:25:34, manzagop wrote: > DCHECK this returns true? Done.
https://codereview.chromium.org/1980743002/diff/730001/base/debug/activity_tr... File base/debug/activity_tracker.cc (right): https://codereview.chromium.org/1980743002/diff/730001/base/debug/activity_tr... base/debug/activity_tracker.cc:385: // the memory reused by a new one. Try again. On 2016/07/29 17:38:38, bcwhite wrote: > > > > Doesn't the header need to be guarded as well? Otherwise, the thread's > name > > > > might be wrong? > > > > > > Not sure what you mean. Memory gets zeroed when a thread exits and loaded > > with > > > new values when its reused. The name can't be anything but empty or valid > for > > > the current pid/tid. > > > > I *think* I meant that you can read a wrong/empty name if the thread exits in > > between reading thread_id and thread_name. > > Yes but then the pid/tid comparison on 386 would fail and restart the loop. No sure I understand. Here's what I meant: - Analyzer reads pid (l.373) - Analyzer reads tid (l.374) <thread exits and header gets cleared> - Analyzer reads thread name (l.375) In this case pid/tid will succeed on l.386. > Of course... There's currently no guarantee that access to those values can't > be reordered by the cpu or compiler. Looks like I need an atomic for the IDs so > I can insert a memory barrier. <sigh> > > > > It's also possible if it exits after loading stack_unchanged and pid+tid get > > reused (very unlucky). > > I think tid is unique across the lifetime of a given pid... Though I went > through so many iteration of tid types that I could be wrong. It's still not > perfect, but it should be enough to deal with the rare race condition. From talking to Siggi I thought PIDs and TIDs could be reused as soon as the object they identify no longer exists. So they'd only uniquely identify live things. Double check this? Couldn't you fix this using the same principle you use for the stack? (an "unchanged" sentinel) If the thread exited, if will have cleared the sentinel. And since you have a single reader, nothing could set it back to one. https://codereview.chromium.org/1980743002/diff/790001/base/debug/activity_tr... File base/debug/activity_tracker.cc (right): https://codereview.chromium.org/1980743002/diff/790001/base/debug/activity_tr... base/debug/activity_tracker.cc:371: output_snapshot->activity_stack.resize(count); On 2016/07/29 17:38:38, bcwhite wrote: > On 2016/07/26 21:25:34, manzagop wrote: > > Resize before the copy so we never copy outside the vector's "active" range in > a > > case where the depth increases between 2 attempts? > > > > As is works (resizing never reduces the capacity), but resizing before seems > > more correct. > > Done, though it was previously resized/reserved to the maximum possible size so > it could only resize smaller at this point. Ah, the issue I was seeing is across many attempts, eg if you fail after having made it smaller, it's possible on the retry the stack grew larger. https://codereview.chromium.org/1980743002/diff/910001/base/debug/activity_tr... File base/debug/activity_tracker.cc (right): https://codereview.chromium.org/1980743002/diff/910001/base/debug/activity_tr... base/debug/activity_tracker.cc:400: // changed while reading it. What if it's a new thread in the same process? Process id will be the same? Do we need a separate sentinel like for the stack?
The CQ bit was checked by bcwhite@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: This issue passed the CQ dry run.
https://codereview.chromium.org/1980743002/diff/730001/base/debug/activity_tr... File base/debug/activity_tracker.cc (right): https://codereview.chromium.org/1980743002/diff/730001/base/debug/activity_tr... base/debug/activity_tracker.cc:385: // the memory reused by a new one. Try again. > - Analyzer reads pid (l.373) > - Analyzer reads tid (l.374) > <thread exits and header gets cleared> > - Analyzer reads thread name (l.375) > > In this case pid/tid will succeed on l.386. Right. Which is why the latest code loads, atomically, the PID last. > > I think tid is unique across the lifetime of a given pid... Though I went > > through so many iteration of tid types that I could be wrong. It's still not > > perfect, but it should be enough to deal with the rare race condition. > > From talking to Siggi I thought PIDs and TIDs could be reused as soon as the > object they identify no longer exists. So they'd only uniquely identify live > things. Double check this? > > Couldn't you fix this using the same principle you use for the stack? (an > "unchanged" sentinel) If the thread exited, if will have cleared the sentinel. > And since you have a single reader, nothing could set it back to one. Yes, but I don't think it's important. The likelihood of a thread ending, the data getting cleared, a new thread being created with the same id, and it performing an action that instantiates tracking... all within this little critical section seems too unlikely to expend a lot of effort to avoid. https://codereview.chromium.org/1980743002/diff/790001/base/debug/activity_tr... File base/debug/activity_tracker.cc (right): https://codereview.chromium.org/1980743002/diff/790001/base/debug/activity_tr... base/debug/activity_tracker.cc:371: output_snapshot->activity_stack.resize(count); On 2016/07/29 18:44:33, manzagop wrote: > On 2016/07/29 17:38:38, bcwhite wrote: > > On 2016/07/26 21:25:34, manzagop wrote: > > > Resize before the copy so we never copy outside the vector's "active" range > in > > a > > > case where the depth increases between 2 attempts? > > > > > > As is works (resizing never reduces the capacity), but resizing before seems > > > more correct. > > > > Done, though it was previously resized/reserved to the maximum possible size > so > > it could only resize smaller at this point. > > Ah, the issue I was seeing is across many attempts, eg if you fail after having > made it smaller, it's possible on the retry the stack grew larger. Acknowledged. https://codereview.chromium.org/1980743002/diff/910001/base/debug/activity_tr... File base/debug/activity_tracker.cc (right): https://codereview.chromium.org/1980743002/diff/910001/base/debug/activity_tr... base/debug/activity_tracker.cc:400: // changed while reading it. On 2016/07/29 18:44:33, manzagop wrote: > What if it's a new thread in the same process? Process id will be the same? Do > we need a separate sentinel like for the stack? The pid is used atomically to guarantee the memory ordering so that reads will happen in a somewhat predictable manner. The ABA failure case is possible because they can be re-used but the likelihood of them being re-used within such a short window doesn't seem rate significant counter-measures.
lgtm https://codereview.chromium.org/1980743002/diff/910001/base/debug/activity_tr... File base/debug/activity_tracker.cc (right): https://codereview.chromium.org/1980743002/diff/910001/base/debug/activity_tr... base/debug/activity_tracker.cc:400: // changed while reading it. On 2016/08/01 14:51:37, bcwhite wrote: > On 2016/07/29 18:44:33, manzagop wrote: > > What if it's a new thread in the same process? Process id will be the same? Do > > we need a separate sentinel like for the stack? > > The pid is used atomically to guarantee the memory ordering so that reads will > happen in a somewhat predictable manner. The ABA failure case is possible > because they can be re-used but the likelihood of them being re-used within such > a short window doesn't seem rate significant counter-measures. Sgtm. This should probably be mentioned in the comment though.
Patchset #24 (id:950001) has been deleted
LGTM https://codereview.chromium.org/1980743002/diff/970001/base/debug/activity_an... File base/debug/activity_analyzer.h (right): https://codereview.chromium.org/1980743002/diff/970001/base/debug/activity_an... base/debug/activity_analyzer.h:30: ThreadKey(int64_t pid, int64_t tid) : pid_(pid), tid_(tid) {} You’ve gone to some work to disambiguate threads across processes, but the system’s idea of the thread ID already does this on all platforms we support, so it should be possible to do this without using process IDs to disambiguate. Unfortunately, Chrome’s threading support in base doesn’t really use system thread IDs for much outside of Windows, you’re stuck with pthread handles, which aren’t unique system-wide. I wish the world were better, but I guess this ThreadKey thing is necessary for now. https://codereview.chromium.org/1980743002/diff/970001/base/debug/activity_tr... File base/debug/activity_tracker.h (right): https://codereview.chromium.org/1980743002/diff/970001/base/debug/activity_tr... base/debug/activity_tracker.h:4: // Make this a blank line instead of a // so that I’m more likely to read what’s after it. (Because nobody really reads the copyright.)
addressed minor review comments; added ownership
https://codereview.chromium.org/1980743002/diff/970001/base/debug/activity_an... File base/debug/activity_analyzer.h (right): https://codereview.chromium.org/1980743002/diff/970001/base/debug/activity_an... base/debug/activity_analyzer.h:30: ThreadKey(int64_t pid, int64_t tid) : pid_(pid), tid_(tid) {} On 2016/08/01 15:59:37, Mark Mentovai wrote: > You’ve gone to some work to disambiguate threads across processes, but the > system’s idea of the thread ID already does this on all platforms we support, so > it should be possible to do this without using process IDs to disambiguate. > Unfortunately, Chrome’s threading support in base doesn’t really use system > thread IDs for much outside of Windows, you’re stuck with pthread handles, which > aren’t unique system-wide. > > I wish the world were better, but I guess this ThreadKey thing is necessary for > now. Acknowledged. https://codereview.chromium.org/1980743002/diff/970001/base/debug/activity_tr... File base/debug/activity_tracker.h (right): https://codereview.chromium.org/1980743002/diff/970001/base/debug/activity_tr... base/debug/activity_tracker.h:4: // On 2016/08/01 15:59:37, Mark Mentovai wrote: > Make this a blank line instead of a // so that I’m more likely to read what’s > after it. (Because nobody really reads the copyright.) Done.
LGTM (including OWNERS)
The CQ bit was checked by bcwhite@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: This issue passed the CQ dry run.
The CQ bit was checked by bcwhite@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from brucedawson@chromium.org, manzagop@chromium.org Link to the patchset: https://codereview.chromium.org/1980743002/#ps980001 (title: "addressed minor review comments; added ownership")
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
bcwhite@chromium.org changed reviewers: + asvitkine@chromium.org
+asvitkine for: chrome/browser/chrome_browser_field_trials_desktop.cc
https://chromiumcodereview.appspot.com/1980743002/diff/980001/base/debug/acti... File base/debug/activity_analyzer.h (right): https://chromiumcodereview.appspot.com/1980743002/diff/980001/base/debug/acti... base/debug/activity_analyzer.h:116: std::unique_ptr<GlobalActivityAnalyzer> CreateWithFile( static?
https://codereview.chromium.org/1980743002/diff/980001/base/debug/activity_tr... File base/debug/activity_tracker.cc (right): https://codereview.chromium.org/1980743002/diff/980001/base/debug/activity_tr... base/debug/activity_tracker.cc:42: const Feature kActivityTrackerFeature{ Move this outside the function, to the anon namespace. The FeatureList code actually verifies that the memory address of the Feature definition is the same, so in a function on the stack this can change. Now, in this case since you're only calling it once and checking for the feature in the same function, you're getting away with it. But still, I'd rather no one does it so there's no confusion on whether it's OK to do. https://codereview.chromium.org/1980743002/diff/980001/base/debug/activity_tr... base/debug/activity_tracker.cc:43: "ActivityTracking", FEATURE_DISABLED_BY_DEFAULT Wearing my privacy hat, this name be changed? It could be completely misinterpreted by someone who does not know what this is doing and can mislead people to think Chrome is tracking people's activities - whereas this is much more specific in scope. https://codereview.chromium.org/1980743002/diff/980001/chrome/browser/chrome_... File chrome/browser/chrome_browser_field_trials_desktop.cc (right): https://codereview.chromium.org/1980743002/diff/980001/chrome/browser/chrome_... chrome/browser/chrome_browser_field_trials_desktop.cc:59: // Track code activities (such as posting task, blocking on locks, and Please move this block to a helper function, which can live in this file. https://codereview.chromium.org/1980743002/diff/980001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1980743002/diff/980001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:60200: +<histogram name="UMA.ActivityTracker.ThreadTrackers.Count"> Add units= to both of these. https://codereview.chromium.org/1980743002/diff/980001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:60203: + The number of threads being tracked for activities. This value is updated "tracked for activities" is meaningless to someone not familiar with what this is doing. Can this be explained more? https://codereview.chromium.org/1980743002/diff/980001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:60210: +<histogram name="UMA.ActivityTracker.ThreadTrackers.MemLimitTrackerCount"> These histograms don't seem to be related to the UMA subsystem, unless I'm mistaken, so they shouldn't be prefixed with "UMA.".
A minor edit suggestion! https://chromiumcodereview.appspot.com/1980743002/diff/980001/base/debug/acti... File base/debug/activity_analyzer.cc (right): https://chromiumcodereview.appspot.com/1980743002/diff/980001/base/debug/acti... base/debug/activity_analyzer.cc:45: mmfile->Initialize(file_path); // Note: write access is required for snapshotting, which writes a sentinel to // ensure the snapshot's consistency. mmfile->Initialize(file_path, MemoryMappedFile::READ_WRITE);
addressed review comments
The CQ bit was checked by bcwhite@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: This issue passed the CQ dry run.
Description was changed from ========== Track thread activities in order to diagnose hangs. There are a large number of "unclean shutdowns" in Chrome with little information available as to what the various threads of Chrome were doing that prevented a proper close. The ActivityTracker class provides a low-overhead way of keeping track of what each thread is doing in a persistent manner that can be analyzed both in real- time and post-mortem. The ActivityAnalyzer class provides an interface into the persisted data for analysis. Using that data is for a future CL. Operation is controlled by an experiment. Enable manually with: --enable-features=ActivityTracking BUG=620813 ========== to ========== Track thread activities in order to diagnose hangs. There are a large number of "unclean shutdowns" in Chrome with little information available as to what the various threads of Chrome were doing that prevented a proper close. The ActivityTracker class provides a low-overhead way of keeping track of what each thread is doing in a persistent manner that can be analyzed both in real- time and post-mortem. The ActivityAnalyzer class provides an interface into the persisted data for analysis. Using that data is for a future CL. Operation is controlled by an experiment. Enable manually with: --enable-features=HangDebugging BUG=620813 ==========
https://codereview.chromium.org/1980743002/diff/980001/base/debug/activity_an... File base/debug/activity_analyzer.cc (right): https://codereview.chromium.org/1980743002/diff/980001/base/debug/activity_an... base/debug/activity_analyzer.cc:45: mmfile->Initialize(file_path); On 2016/08/03 14:36:06, manzagop wrote: > // Note: write access is required for snapshotting, which writes a sentinel to > // ensure the snapshot's consistency. > mmfile->Initialize(file_path, MemoryMappedFile::READ_WRITE); Good point. Done. https://codereview.chromium.org/1980743002/diff/980001/base/debug/activity_an... File base/debug/activity_analyzer.h (right): https://codereview.chromium.org/1980743002/diff/980001/base/debug/activity_an... base/debug/activity_analyzer.h:116: std::unique_ptr<GlobalActivityAnalyzer> CreateWithFile( On 2016/08/02 21:16:43, manzagop wrote: > static? Done. https://codereview.chromium.org/1980743002/diff/980001/base/debug/activity_tr... File base/debug/activity_tracker.cc (right): https://codereview.chromium.org/1980743002/diff/980001/base/debug/activity_tr... base/debug/activity_tracker.cc:42: const Feature kActivityTrackerFeature{ On 2016/08/02 21:18:51, Alexei Svitkine (slow) wrote: > Move this outside the function, to the anon namespace. > > The FeatureList code actually verifies that the memory address of the Feature > definition is the same, so in a function on the stack this can change. Now, in > this case since you're only calling it once and checking for the feature in the > same function, you're getting away with it. But still, I'd rather no one does it > so there's no confusion on whether it's OK to do. Done. https://codereview.chromium.org/1980743002/diff/980001/base/debug/activity_tr... base/debug/activity_tracker.cc:43: "ActivityTracking", FEATURE_DISABLED_BY_DEFAULT On 2016/08/02 21:18:51, Alexei Svitkine (slow) wrote: > Wearing my privacy hat, this name be changed? It could be completely > misinterpreted by someone who does not know what this is doing and can mislead > people to think Chrome is tracking people's activities - whereas this is much > more specific in scope. Done. https://codereview.chromium.org/1980743002/diff/980001/chrome/browser/chrome_... File chrome/browser/chrome_browser_field_trials_desktop.cc (right): https://codereview.chromium.org/1980743002/diff/980001/chrome/browser/chrome_... chrome/browser/chrome_browser_field_trials_desktop.cc:59: // Track code activities (such as posting task, blocking on locks, and On 2016/08/02 21:18:51, Alexei Svitkine (slow) wrote: > Please move this block to a helper function, which can live in this file. Done. https://codereview.chromium.org/1980743002/diff/980001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1980743002/diff/980001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:60200: +<histogram name="UMA.ActivityTracker.ThreadTrackers.Count"> On 2016/08/02 21:18:51, Alexei Svitkine (slow) wrote: > Add units= to both of these. Done. https://codereview.chromium.org/1980743002/diff/980001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:60203: + The number of threads being tracked for activities. This value is updated On 2016/08/02 21:18:51, Alexei Svitkine (slow) wrote: > "tracked for activities" is meaningless to someone not familiar with what this > is doing. Can this be explained more? Done. https://codereview.chromium.org/1980743002/diff/980001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:60210: +<histogram name="UMA.ActivityTracker.ThreadTrackers.MemLimitTrackerCount"> On 2016/08/02 21:18:51, Alexei Svitkine (slow) wrote: > These histograms don't seem to be related to the UMA subsystem, unless I'm > mistaken, so they shouldn't be prefixed with "UMA.". "Debug.*" okay?
https://codereview.chromium.org/1980743002/diff/1000001/base/debug/activity_t... File base/debug/activity_tracker.cc (right): https://codereview.chromium.org/1980743002/diff/1000001/base/debug/activity_t... base/debug/activity_tracker.cc:39: "HangDebugging", FEATURE_DISABLED_BY_DEFAULT Make the feature name match the constant name (i.e. kHangDebuggingFeature). https://codereview.chromium.org/1980743002/diff/1000001/base/debug/activity_t... base/debug/activity_tracker.cc:613: UMA_HISTOGRAM_ENUMERATION("Debug.ActivityTracker.ThreadTrackers.Count", Maybe HangDebugging.? Or HangWatcher. or something? https://codereview.chromium.org/1980743002/diff/1000001/chrome/browser/chrome... File chrome/browser/chrome_browser_field_trials_desktop.cc (right): https://codereview.chromium.org/1980743002/diff/1000001/chrome/browser/chrome... chrome/browser/chrome_browser_field_trials_desktop.cc:52: void SetupActivityTracking() { SetupHangDebugging?
https://codereview.chromium.org/1980743002/diff/1000001/base/debug/activity_t... File base/debug/activity_tracker.cc (right): https://codereview.chromium.org/1980743002/diff/1000001/base/debug/activity_t... base/debug/activity_tracker.cc:613: UMA_HISTOGRAM_ENUMERATION("Debug.ActivityTracker.ThreadTrackers.Count", On 2016/08/04 14:27:05, Alexei Svitkine (slow) wrote: > Maybe HangDebugging.? Or HangWatcher. or something? The plan is to use this "internal state representation" debug information for the many stability issues: hang, crash and unclean shutdown. How about Stability.*? Or did you mean to replace ActivityTracker? If so... InternalStateTracker?
Stability.* SGTM On Thu, Aug 4, 2016 at 10:35 AM, <manzagop@chromium.org> wrote: > > https://codereview.chromium.org/1980743002/diff/1000001/ > base/debug/activity_tracker.cc > File base/debug/activity_tracker.cc (right): > > https://codereview.chromium.org/1980743002/diff/1000001/ > base/debug/activity_tracker.cc#newcode613 > base/debug/activity_tracker.cc:613: > UMA_HISTOGRAM_ENUMERATION("Debug.ActivityTracker.ThreadTrackers.Count", > On 2016/08/04 14:27:05, Alexei Svitkine (slow) wrote: > > Maybe HangDebugging.? Or HangWatcher. or something? > > The plan is to use this "internal state representation" debug > information for the many stability issues: hang, crash and unclean > shutdown. > > How about Stability.*? Or did you mean to replace ActivityTracker? If > so... InternalStateTracker? > > https://codereview.chromium.org/1980743002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/1980743002/diff/1000001/base/debug/activity_t... File base/debug/activity_tracker.cc (right): https://codereview.chromium.org/1980743002/diff/1000001/base/debug/activity_t... base/debug/activity_tracker.cc:39: "HangDebugging", FEATURE_DISABLED_BY_DEFAULT On 2016/08/04 14:27:05, Alexei Svitkine (slow) wrote: > Make the feature name match the constant name (i.e. kHangDebuggingFeature). Done. https://codereview.chromium.org/1980743002/diff/1000001/base/debug/activity_t... base/debug/activity_tracker.cc:613: UMA_HISTOGRAM_ENUMERATION("Debug.ActivityTracker.ThreadTrackers.Count", On 2016/08/04 14:35:46, manzagop wrote: > On 2016/08/04 14:27:05, Alexei Svitkine (slow) wrote: > > Maybe HangDebugging.? Or HangWatcher. or something? > > The plan is to use this "internal state representation" debug information for > the many stability issues: hang, crash and unclean shutdown. > > How about Stability.*? Or did you mean to replace ActivityTracker? If so... > InternalStateTracker? The UMA name should match the class, should it not? I can rename all the classes to StateTracker (or InternalStateTracker) if that would avoid confusion. Pain in the butt right now but might be worth it to have less butt pain in the future.
It doesn't need to match the class - usually the suggestion is to use broader categories than class names. Having said that, I like StateTracker much better than ActivityTracker, so I would support the rename. On Thu, Aug 4, 2016 at 10:55 AM, <bcwhite@chromium.org> wrote: > > https://codereview.chromium.org/1980743002/diff/1000001/ > base/debug/activity_tracker.cc > File base/debug/activity_tracker.cc (right): > > https://codereview.chromium.org/1980743002/diff/1000001/ > base/debug/activity_tracker.cc#newcode39 > base/debug/activity_tracker.cc:39: "HangDebugging", > FEATURE_DISABLED_BY_DEFAULT > On 2016/08/04 14:27:05, Alexei Svitkine (slow) wrote: > > Make the feature name match the constant name (i.e. > kHangDebuggingFeature). > > Done. > > https://codereview.chromium.org/1980743002/diff/1000001/ > base/debug/activity_tracker.cc#newcode613 > base/debug/activity_tracker.cc:613: > UMA_HISTOGRAM_ENUMERATION("Debug.ActivityTracker.ThreadTrackers.Count", > On 2016/08/04 14:35:46, manzagop wrote: > > On 2016/08/04 14:27:05, Alexei Svitkine (slow) wrote: > > > Maybe HangDebugging.? Or HangWatcher. or something? > > > > The plan is to use this "internal state representation" debug > information for > > the many stability issues: hang, crash and unclean shutdown. > > > > How about Stability.*? Or did you mean to replace ActivityTracker? If > so... > > InternalStateTracker? > > The UMA name should match the class, should it not? I can rename all > the classes to StateTracker (or InternalStateTracker) if that would > avoid confusion. Pain in the butt right now but might be worth it to > have less butt pain in the future. > > https://codereview.chromium.org/1980743002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
> It doesn't need to match the class - usually the suggestion is to use > broader categories than class names. In this case, though, "StabilityDebugging" is a narrower category than the "ActivityTracker" class name which is a general ability. The tracker could be used for other uses than just tracking stability. > Having said that, I like StateTracker much better than ActivityTracker, so > I would support the rename. I looked down that path a bit; it doesn't work as well. PA doesn't have a strong opinion so I'm going to leave it the way it is.
lgtm % comments https://codereview.chromium.org/1980743002/diff/1000001/base/debug/activity_t... File base/debug/activity_tracker.cc (right): https://codereview.chromium.org/1980743002/diff/1000001/base/debug/activity_t... base/debug/activity_tracker.cc:39: "HangDebugging", FEATURE_DISABLED_BY_DEFAULT On 2016/08/04 14:55:36, bcwhite wrote: > On 2016/08/04 14:27:05, Alexei Svitkine (slow) wrote: > > Make the feature name match the constant name (i.e. kHangDebuggingFeature). > > Done. Note done? https://codereview.chromium.org/1980743002/diff/1000001/base/debug/activity_t... File base/debug/activity_tracker.h (right): https://codereview.chromium.org/1980743002/diff/1000001/base/debug/activity_t... base/debug/activity_tracker.h:510: }; DISALLOW_COPY_AND_ASSIGN()? Also, please add anywhere else you're missing it, such as in inner classes. https://codereview.chromium.org/1980743002/diff/1000001/chrome/browser/chrome... File chrome/browser/chrome_browser_field_trials_desktop.cc (right): https://codereview.chromium.org/1980743002/diff/1000001/chrome/browser/chrome... chrome/browser/chrome_browser_field_trials_desktop.cc:52: void SetupActivityTracking() { On 2016/08/04 14:27:05, Alexei Svitkine (slow) wrote: > SetupHangDebugging? Ping. https://codereview.chromium.org/1980743002/diff/1000001/tools/metrics/histogr... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1980743002/diff/1000001/tools/metrics/histogr... tools/metrics/histograms/histograms.xml:8262: +<histogram name="Debug.ActivityTracker.ThreadTrackers.Count" units="count"> Okay, in this case remove the Debug. prefix.
cleaned up feature instantiation; addressed review comments
The CQ bit was checked by bcwhite@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...
cleaned up feature instantiation; addressed review comments
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Patchset #27 (id:1020001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/1980743002/diff/1000001/base/debug/activity_t... File base/debug/activity_tracker.cc (right): https://codereview.chromium.org/1980743002/diff/1000001/base/debug/activity_t... base/debug/activity_tracker.cc:39: "HangDebugging", FEATURE_DISABLED_BY_DEFAULT On 2016/08/04 18:16:45, Alexei Svitkine (slow) wrote: > On 2016/08/04 14:55:36, bcwhite wrote: > > On 2016/08/04 14:27:05, Alexei Svitkine (slow) wrote: > > > Make the feature name match the constant name (i.e. kHangDebuggingFeature). > > > > Done. > > Note done? Done, just not uploaded because it became a discussion on another topic. https://codereview.chromium.org/1980743002/diff/1000001/base/debug/activity_t... File base/debug/activity_tracker.h (right): https://codereview.chromium.org/1980743002/diff/1000001/base/debug/activity_t... base/debug/activity_tracker.h:510: }; On 2016/08/04 18:16:45, Alexei Svitkine (slow) wrote: > DISALLOW_COPY_AND_ASSIGN()? > > Also, please add anywhere else you're missing it, such as in inner classes. Done. https://codereview.chromium.org/1980743002/diff/1000001/chrome/browser/chrome... File chrome/browser/chrome_browser_field_trials_desktop.cc (right): https://codereview.chromium.org/1980743002/diff/1000001/chrome/browser/chrome... chrome/browser/chrome_browser_field_trials_desktop.cc:52: void SetupActivityTracking() { On 2016/08/04 14:27:05, Alexei Svitkine (slow) wrote: > SetupHangDebugging? Done. https://codereview.chromium.org/1980743002/diff/1000001/chrome/browser/chrome... chrome/browser/chrome_browser_field_trials_desktop.cc:52: void SetupActivityTracking() { On 2016/08/04 18:16:45, Alexei Svitkine (slow) wrote: > On 2016/08/04 14:27:05, Alexei Svitkine (slow) wrote: > > SetupHangDebugging? > > Ping. Done. Was waiting to resolve other names. https://codereview.chromium.org/1980743002/diff/1000001/tools/metrics/histogr... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1980743002/diff/1000001/tools/metrics/histogr... tools/metrics/histograms/histograms.xml:8262: +<histogram name="Debug.ActivityTracker.ThreadTrackers.Count" units="count"> On 2016/08/04 18:16:45, Alexei Svitkine (slow) wrote: > Okay, in this case remove the Debug. prefix. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Ping?
Who are you pinging? (You have my l-g-t-m already.) On Aug 8, 2016 8:09 AM, <bcwhite@chromium.org> wrote: > Ping? > > https://codereview.chromium.org/1980743002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by bcwhite@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from manzagop@chromium.org, mark@chromium.org, brucedawson@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/1980743002/#ps1040001 (title: "cleaned up feature instantiation; addressed review comments")
Description was changed from ========== Track thread activities in order to diagnose hangs. There are a large number of "unclean shutdowns" in Chrome with little information available as to what the various threads of Chrome were doing that prevented a proper close. The ActivityTracker class provides a low-overhead way of keeping track of what each thread is doing in a persistent manner that can be analyzed both in real- time and post-mortem. The ActivityAnalyzer class provides an interface into the persisted data for analysis. Using that data is for a future CL. Operation is controlled by an experiment. Enable manually with: --enable-features=HangDebugging BUG=620813 ========== to ========== Track thread activities in order to diagnose hangs. There are a large number of "unclean shutdowns" in Chrome with little information available as to what the various threads of Chrome were doing that prevented a proper close. The ActivityTracker class provides a low-overhead way of keeping track of what each thread is doing in a persistent manner that can be analyzed both in real- time and post-mortem. The ActivityAnalyzer class provides an interface into the persisted data for analysis. Using that data is for a future CL. Operation is controlled by an experiment. Enable manually with: --enable-features=StabilityDebugging BUG=620813 ==========
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 ========== Track thread activities in order to diagnose hangs. There are a large number of "unclean shutdowns" in Chrome with little information available as to what the various threads of Chrome were doing that prevented a proper close. The ActivityTracker class provides a low-overhead way of keeping track of what each thread is doing in a persistent manner that can be analyzed both in real- time and post-mortem. The ActivityAnalyzer class provides an interface into the persisted data for analysis. Using that data is for a future CL. Operation is controlled by an experiment. Enable manually with: --enable-features=StabilityDebugging BUG=620813 ========== to ========== Track thread activities in order to diagnose hangs. There are a large number of "unclean shutdowns" in Chrome with little information available as to what the various threads of Chrome were doing that prevented a proper close. The ActivityTracker class provides a low-overhead way of keeping track of what each thread is doing in a persistent manner that can be analyzed both in real- time and post-mortem. The ActivityAnalyzer class provides an interface into the persisted data for analysis. Using that data is for a future CL. Operation is controlled by an experiment. Enable manually with: --enable-features=StabilityDebugging BUG=620813 ==========
Message was sent while issue was closed.
Committed patchset #27 (id:1040001)
Message was sent while issue was closed.
Description was changed from ========== Track thread activities in order to diagnose hangs. There are a large number of "unclean shutdowns" in Chrome with little information available as to what the various threads of Chrome were doing that prevented a proper close. The ActivityTracker class provides a low-overhead way of keeping track of what each thread is doing in a persistent manner that can be analyzed both in real- time and post-mortem. The ActivityAnalyzer class provides an interface into the persisted data for analysis. Using that data is for a future CL. Operation is controlled by an experiment. Enable manually with: --enable-features=StabilityDebugging BUG=620813 ========== to ========== Track thread activities in order to diagnose hangs. There are a large number of "unclean shutdowns" in Chrome with little information available as to what the various threads of Chrome were doing that prevented a proper close. The ActivityTracker class provides a low-overhead way of keeping track of what each thread is doing in a persistent manner that can be analyzed both in real- time and post-mortem. The ActivityAnalyzer class provides an interface into the persisted data for analysis. Using that data is for a future CL. Operation is controlled by an experiment. Enable manually with: --enable-features=StabilityDebugging BUG=620813 Committed: https://crrev.com/5d3470e5a33aaad936e171bee625b6b978ee3c34 Cr-Commit-Position: refs/heads/master@{#410470} ==========
Message was sent while issue was closed.
Patchset 27 (id:??) landed as https://crrev.com/5d3470e5a33aaad936e171bee625b6b978ee3c34 Cr-Commit-Position: refs/heads/master@{#410470}
Message was sent while issue was closed.
I am your sheriff today and I manually reverted this in https://codereview.chromium.org/2221343002, please shout if that was the wrong call.
Message was sent while issue was closed.
The failure is an invalid argument to Lock() so my change shouldn't be the cause unless it is corrupting the stack somehow. Manual inspection didn't reveal any way that would happen. The revert landed at 11:53 UTC and another flake occurred at 13:57 UTC so it doesn't appear to be my change. Details in https://bugs.chromium.org/p/chromium/issues/detail?id=635832
Message was sent while issue was closed.
Description was changed from ========== Track thread activities in order to diagnose hangs. There are a large number of "unclean shutdowns" in Chrome with little information available as to what the various threads of Chrome were doing that prevented a proper close. The ActivityTracker class provides a low-overhead way of keeping track of what each thread is doing in a persistent manner that can be analyzed both in real- time and post-mortem. The ActivityAnalyzer class provides an interface into the persisted data for analysis. Using that data is for a future CL. Operation is controlled by an experiment. Enable manually with: --enable-features=StabilityDebugging BUG=620813 Committed: https://crrev.com/5d3470e5a33aaad936e171bee625b6b978ee3c34 Cr-Commit-Position: refs/heads/master@{#410470} ========== to ========== Track thread activities in order to diagnose hangs. There are a large number of "unclean shutdowns" in Chrome with little information available as to what the various threads of Chrome were doing that prevented a proper close. The ActivityTracker class provides a low-overhead way of keeping track of what each thread is doing in a persistent manner that can be analyzed both in real- time and post-mortem. The ActivityAnalyzer class provides an interface into the persisted data for analysis. Using that data is for a future CL. Operation is controlled by an experiment. Enable manually with: --enable-features=StabilityDebugging BUG=620813 Committed: https://crrev.com/5d3470e5a33aaad936e171bee625b6b978ee3c34 Cr-Commit-Position: refs/heads/master@{#410470} ==========
The CQ bit was checked by bcwhite@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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by bcwhite@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: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Patchset #28 (id:1060001) has been deleted
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 bcwhite@chromium.org
The CQ bit was checked by bcwhite@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from manzagop@chromium.org, mark@chromium.org, asvitkine@chromium.org, brucedawson@chromium.org Link to the patchset: https://codereview.chromium.org/1980743002/#ps1080001 (title: "rebased")
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 ========== Track thread activities in order to diagnose hangs. There are a large number of "unclean shutdowns" in Chrome with little information available as to what the various threads of Chrome were doing that prevented a proper close. The ActivityTracker class provides a low-overhead way of keeping track of what each thread is doing in a persistent manner that can be analyzed both in real- time and post-mortem. The ActivityAnalyzer class provides an interface into the persisted data for analysis. Using that data is for a future CL. Operation is controlled by an experiment. Enable manually with: --enable-features=StabilityDebugging BUG=620813 Committed: https://crrev.com/5d3470e5a33aaad936e171bee625b6b978ee3c34 Cr-Commit-Position: refs/heads/master@{#410470} ========== to ========== Track thread activities in order to diagnose hangs. There are a large number of "unclean shutdowns" in Chrome with little information available as to what the various threads of Chrome were doing that prevented a proper close. The ActivityTracker class provides a low-overhead way of keeping track of what each thread is doing in a persistent manner that can be analyzed both in real- time and post-mortem. The ActivityAnalyzer class provides an interface into the persisted data for analysis. Using that data is for a future CL. Operation is controlled by an experiment. Enable manually with: --enable-features=StabilityDebugging BUG=620813 Committed: https://crrev.com/5d3470e5a33aaad936e171bee625b6b978ee3c34 Cr-Commit-Position: refs/heads/master@{#410470} ==========
Message was sent while issue was closed.
Committed patchset #28 (id:1080001)
Message was sent while issue was closed.
Description was changed from ========== Track thread activities in order to diagnose hangs. There are a large number of "unclean shutdowns" in Chrome with little information available as to what the various threads of Chrome were doing that prevented a proper close. The ActivityTracker class provides a low-overhead way of keeping track of what each thread is doing in a persistent manner that can be analyzed both in real- time and post-mortem. The ActivityAnalyzer class provides an interface into the persisted data for analysis. Using that data is for a future CL. Operation is controlled by an experiment. Enable manually with: --enable-features=StabilityDebugging BUG=620813 Committed: https://crrev.com/5d3470e5a33aaad936e171bee625b6b978ee3c34 Cr-Commit-Position: refs/heads/master@{#410470} ========== to ========== Track thread activities in order to diagnose hangs. There are a large number of "unclean shutdowns" in Chrome with little information available as to what the various threads of Chrome were doing that prevented a proper close. The ActivityTracker class provides a low-overhead way of keeping track of what each thread is doing in a persistent manner that can be analyzed both in real- time and post-mortem. The ActivityAnalyzer class provides an interface into the persisted data for analysis. Using that data is for a future CL. Operation is controlled by an experiment. Enable manually with: --enable-features=StabilityDebugging BUG=620813 Committed: https://crrev.com/5d3470e5a33aaad936e171bee625b6b978ee3c34 Committed: https://crrev.com/d9705964f525c278a5939b6fcd29a10f732149d0 Cr-Original-Commit-Position: refs/heads/master@{#410470} Cr-Commit-Position: refs/heads/master@{#410938} ==========
Message was sent while issue was closed.
Patchset 28 (id:??) landed as https://crrev.com/d9705964f525c278a5939b6fcd29a10f732149d0 Cr-Commit-Position: refs/heads/master@{#410938} |
