Track thread activities in order to diagnose hangs.

base/debug/activity_tracker.h

+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef BASE_METRICS_ACTIVITY_TRACKER_H_
+#define BASE_METRICS_ACTIVITY_TRACKER_H_
+
+#include <atomic>
+#include <memory>
+
+#include "base/base_export.h"
+#include "base/feature_list.h"
+#include "base/files/file_path.h"
+#include "base/metrics/persistent_memory_allocator.h"
+#include "base/synchronization/lock.h"
+#include "base/threading/thread_checker.h"
+#include "base/threading/thread_local_storage.h"
+
+namespace base {
+
+struct PendingTask;
+
+class Lock;
+class MemoryMappedFile;
+
+namespace debug {

[Sigurður Ásgeirsson, 2016/05/20 13:12:05]

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?

[manzagop (departed), 2016/05/20 13:55:42]

+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.

[bcwhite, 2016/05/20 19:19:19]

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.
Done.
I'll keep it in mind.

[bcwhite, 2016/05/20 19:19:19]

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.

[manzagop (departed), 2016/05/20 20:24:21]

Great!

+
+// Enables the global activity tracker according to a field trial setting.
+BASE_EXPORT void SetupGlobalActivityTrackerFieldTrial();
+
+
+// This class manages tracking a stack of activities for a single thread in
+// a persistent manner. However, in order to support an operational mode where
+// another thread is analyzing this data in real-time, atomic operations are
+// used where necessary to guarantee a consistent view from the outside.
+class BASE_EXPORT ThreadActivityTracker {

[Sigurður Ásgeirsson, 2016/05/20 13:12:05]

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?

[bcwhite, 2016/05/20 19:19:19]

Correct.  A ThreadChecker ensures this.
I believe it does.  If the final result ends otherwise, I'll move it into the
.cc file at that time.

+ public:
+  enum ActivityType : uint8_t {
+    ACT_TASK,
+    ACT_LOCK,
+    ACT_EVENT,
+  };
+
+  struct StackEntry {

[Sigurður Ásgeirsson, 2016/05/20 13:12:05]

nit: the fields warrant a little bit of documentation?

[bcwhite, 2016/05/20 19:19:19]

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.  ;-)

+    int64_t time_ticks;
+    uint8_t activity_type;

[Sigurður Ásgeirsson, 2016/05/20 13:12:05]

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.

[bcwhite, 2016/05/20 19:19:19]

I agree.  That'll simplify some other places, too, where there are too many
parameters being passed.

+    intptr_t source_address;
+    intptr_t method_address;
+    uint64_t sequence_id;
+  };
+
+  class BASE_EXPORT ScopedActivity {
+   public:
+    ScopedActivity(ThreadActivityTracker* tracker,
+                   const void* source,
+                   ActivityType activity,
+                   intptr_t method,
+                   uint64_t sequence)
+        : tracker_(tracker), source_(source) {
+      if (tracker_)
+        tracker_->RecordStart(source, activity, method, sequence);
+    }
+    ~ScopedActivity() {
+      if (tracker_)
+        tracker_->RecordFinish(source_);
+    }
+
+   private:
+    ThreadActivityTracker* const tracker_;
+    const void* const source_;
+  };
+
+  // A ThreadActivityTracker runs on top of memory that is managed externally.

[manzagop (departed), 2016/05/20 18:19:30]

Mention the precondition on the size (large enough for header and a few
entries)?

[bcwhite, 2016/05/20 19:19:19]

Done.

+  ThreadActivityTracker(void* base, size_t size);
+  virtual ~ThreadActivityTracker();
+
+  // Indicate that a method of the given (arbitrary) identifier has started.
+  void RecordStart(const void* source,

[Sigurður Ásgeirsson, 2016/05/20 13:12:04]

RecordXXX implies tracing, but here we're maintaining a stack.
PushActivity/PopActivity?

[bcwhite, 2016/05/20 19:19:19]

Done.

+                   ActivityType activity,
+                   intptr_t method,
+                   uint64_t sequence);
+
+  // Indicate that a method of the given (arbitrary) identifier has finished.
+  void RecordFinish(const void* source);
+
+  // Gets a copy of the current stack contents. The return value is the current
+  // depth of the stack which may be greater than the number of StackEntry
+  // records returned. If so, the returned stack has the "base" of the stack
+  // with later entries omitted.
+  uint32_t CopyStack(std::vector<StackEntry>* stack);

[Sigurður Ásgeirsson, 2016/05/20 13:12:05]

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.

[bcwhite, 2016/05/20 19:19:19]

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.
Done.

+
+  // Returns whether the current data is valid or not. Fetching a copy of the

[manzagop (departed), 2016/05/20 18:19:30]

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?

[bcwhite, 2016/05/20 19:19:19]

Done.

+  // stack will return nothing if the data is not valid.
+  bool is_valid() { return valid_; }
+
+  // Calculates the memory size required for a given stack depth.

[manzagop (departed), 2016/05/20 18:19:30]

nit: clarify this includes the header, eg "required by the tracker".

[bcwhite, 2016/05/20 19:19:19]

Done.

+  static size_t SizeForStackDepth(int stack_depth);
+
+ private:
+  friend class Iterator;

[manzagop (departed), 2016/05/20 18:19:30]

Is this needed?

[bcwhite, 2016/05/20 19:19:19]

No.  The iterator went away in favor of CopyStack().

+
+  struct Header;
+
+  Header* const header_;
+  StackEntry* const stack_;
+  const uint32_t slots_;

[manzagop (departed), 2016/05/20 18:19:30]

nit: max_stack_depth_ or stack_slots_ clearer?

[bcwhite, 2016/05/20 19:19:19]

Done.  The "depth" can actually be greater; it's just that no entries will be
stored for them.

+
+  bool valid_ = false;
+
+  base::ThreadChecker thread_checker_;
+
+  DISALLOW_COPY_AND_ASSIGN(ThreadActivityTracker);
+};
+
+class BASE_EXPORT GlobalActivityTracker {

[Sigurður Ásgeirsson, 2016/05/20 13:12:05]

so one of these guys manages one per thread of the other guys?
Depending on experiment state there's zero or one of these?

[bcwhite, 2016/05/20 19:19:19]

Correct.

+ public:
+  class BASE_EXPORT ScopedThreadActivity
+      : public ThreadActivityTracker::ScopedActivity {
+   public:
+    ScopedThreadActivity(const void* source,
+                         ThreadActivityTracker::ActivityType activity,
+                         intptr_t method,
+                         uint64_t sequence)
+        : ThreadActivityTracker::ScopedActivity(GetOrCreateTracker(),
+                                                source,
+                                                activity,
+                                                method,
+                                                sequence) {}
+
+   private:
+    static ThreadActivityTracker* GetOrCreateTracker() {
+      GlobalActivityTracker* global_tracker = Get();
+      if (!global_tracker)
+        return nullptr;
+      return global_tracker->GetOrCreateTrackerForCurrentThread();
+    }
+  };
+
+  ~GlobalActivityTracker();
+
+  static void CreateWithAllocator(
+      std::unique_ptr<PersistentMemoryAllocator> allocator,
+      int stack_depth);
+
+  static void CreateWithLocalMemory(size_t size,
+                                    uint64_t id,
+                                    StringPiece name,
+                                    int stack_depth);
+
+  static void CreateWithFile(const FilePath& file_path,
+                             size_t size,
+                             uint64_t id,
+                             StringPiece name,
+                             int stack_depth);
+
+  // Gets the global activity-tracker or null if none exists.
+  static GlobalActivityTracker* Get() { return g_tracker_; }
+
+  // Gets the thread's activity-tracker, assuming it already exists. This
+  // is inline for performance reasons. Ownership remains with the global
+  // tracker.
+  ThreadActivityTracker* GetTrackerForCurrentThread() {
+    void* tracker = this_thread_tracker_.Get();
+    DCHECK(tracker);
+    return reinterpret_cast<ThreadActivityTracker*>(tracker);
+  }
+
+  // Gets the thread's activity-tracker or creates one if none exists. This
+  // is inline for performance reasons. Ownership remains with the global
+  // tracker.
+  ThreadActivityTracker* GetOrCreateTrackerForCurrentThread() {

[Sigurður Ásgeirsson, 2016/05/20 13:12:04]

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.

[bcwhite, 2016/05/20 19:19:19]

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.

[Sigurður Ásgeirsson, 2016/05/24 14:11:50]

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.

[bcwhite, 2016/05/26 15:35:39]

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.

+    void* tracker = this_thread_tracker_.Get();
+    if (tracker)
+      return reinterpret_cast<ThreadActivityTracker*>(tracker);
+    return CreateTrackerForCurrentThread();
+  }
+
+  // Creates an activity-tracker for the current thread.
+  ThreadActivityTracker* CreateTrackerForCurrentThread();
+
+  // Releases the activity-tracker for the current thread (for testing only).
+  void ReleaseTrackerForCurrentThreadForTesting();
+
+ private:
+  class ManagedActivityTracker : public ThreadActivityTracker {
+   public:
+    ManagedActivityTracker(PersistentMemoryAllocator::Reference mem_reference,
+                           void* base,
+                           size_t size);
+    ~ManagedActivityTracker() override;
+
+   private:
+    const PersistentMemoryAllocator::Reference mem_reference_;
+    void* const mem_base_;
+  };
+
+  GlobalActivityTracker(std::unique_ptr<PersistentMemoryAllocator> allocator,
+                        int stack_depth);
+
+  // Returns the memory used by an activity-tracker managed by this class.
+  void ReturnTrackerMemory(ManagedActivityTracker* tracker,
+                           PersistentMemoryAllocator::Reference mem_reference,
+                           void* mem_base);
+
+  static void OnTLSDestroy(void* value);
+
+  std::unique_ptr<PersistentMemoryAllocator> allocator_;
+  const size_t stack_memory_;

[manzagop (departed), 2016/05/20 20:24:22]

nit: stack_memory_size_?

[bcwhite, 2016/05/26 15:35:39]

Done.

+
+  base::ThreadLocalStorage::Slot this_thread_tracker_;
+
+  Lock lock_;
+  std::set<ManagedActivityTracker*> thread_trackers_;
+  std::vector<PersistentMemoryAllocator::Reference> available_memories_;
+
+  static GlobalActivityTracker* g_tracker_;
+};
+
+class BASE_EXPORT ScopedTaskActivity

[Sigurður Ásgeirsson, 2016/05/20 13:12:04]

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.

[bcwhite, 2016/05/20 19:19:19]

Acknowledged.

+    : public GlobalActivityTracker::ScopedThreadActivity {
+ public:
+  ScopedTaskActivity(const base::PendingTask& task);
+};
+
+}  // namespace debug
+}  // namespace base
+
+#endif  // BASE_METRICS_ACTIVITY_TRACKER_H_