Temporary commit to evaluate perf impact of prototype CPU profiler

base/profiler/cpu_profiler.h

+// Copyright 2015 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.
+
+

[Ilya Sherman, 2015/02/03 22:12:10]

nit: Spurious newline.

[Mike Wittman, 2015/02/04 01:37:10]

Done.

+#ifndef BASE_PROFILER_CPU_PROFILER_H_
+#define BASE_PROFILER_CPU_PROFILER_H_
+
+#include <map>
+
+#include "base/base_export.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/strings/string16.h"
+#include "base/synchronization/waitable_event.h"
+#include "base/timer/timer.h"
+
+#if defined(OS_WIN)
+struct _CONTEXT;
+#endif
+
+namespace base {
+
+class SamplingThread : public PlatformThread::Delegate {
+ public:
+  SamplingThread();
+  ~SamplingThread() override;
+
+  // Implementation of PlatformThread::Delegate:
+  void ThreadMain() override;
+
+  void Stop();
+
+  void GetSamples();
+
+ private:
+  bool thread_running_;
+
+  base::WaitableEvent waitable_event_;
+};

[Ilya Sherman, 2015/02/03 22:12:10]

Please move this class into a separate header + implementation file.

[Mike Wittman, 2015/02/04 01:37:10]

Can we keep this as-is for purposes of this commit? We're looking to validate
this approach by landing for only as long as it takes to get performance
measurements, and I'd prefer to limit the amount of restructuring required to do
so. I will address these concerns before any attempt to land this code
permanently.

[Ilya Sherman, 2015/02/04 23:09:57]

Ah, right, I recall now that you mentioned this was intended as temporary
commit.  That's fine by me, then.

+
+class BASE_EXPORT CpuProfiler {
+ public:
+  static void Initialize(const std::map<std::string, std::string>& params);
+  static CpuProfiler* GetInstance();
+  static bool IsPlatformSupported();
+  static void Stop();
+
+  std::string GetStringParam(const std::string& key);
+  int GetIntParam(const std::string& key);
+  int64 GetInt64Param(const std::string& key);
+
+  void OnTimer();
+
+ private:
+  CpuProfiler();
+  ~CpuProfiler();
+
+  void SetParams(const std::map<std::string, std::string>& params);
+
+  void GetThreadIds();
+
+#if defined(OS_WIN)
+  struct StackTraceEntry {
+    DWORD64 rsp;
+    DWORD64 rip;
+    HMODULE module;
+  };
+
+  int SampleThread(DWORD thread, int max_stack_size, StackTraceEntry* stack);
+
+  static int StackTrace64(_CONTEXT* context, int stack_depth,
+      StackTraceEntry* stack_trace);
+
+  static void GetNames(StackTraceEntry* stack_trace, int stack_depth);
+
+  void ResolveModules();
+
+  void ProcessStack(StackTraceEntry* stack, int stack_depth);
+
+  DWORD ui_thread_;
+  DWORD io_thread_;
+
+  int ui_thread_stack_depth_;
+  StackTraceEntry ui_thread_stack_[64];
+
+  int io_thread_stack_depth_;
+  StackTraceEntry io_thread_stack_[64];
+
+  std::map<HMODULE, base::string16> modules_;
+#endif

[Ilya Sherman, 2015/02/03 22:12:10]

I'd really prefer that you find a way to keep the Windows-specific parts of the
interface outside of the shared header.

[Mike Wittman, 2015/02/04 01:37:10]

Again, can we keep this as-is for purposes of this commit? I will address this
before any future review of these changes.

+
+  static CpuProfiler* g_instance_;
+
+  typedef std::map<std::string, std::string> Map;

[Ilya Sherman, 2015/02/03 22:12:10]

nit: I'm really not fond of this typedef -- especially given the super generic
name.  With C++11's support of the auto keyword, I think this sort of thing
shouldn't be needed anymore.

[Mike Wittman, 2015/02/04 01:37:10]

Done.

+  Map params_;
+
+  scoped_ptr<SamplingThread> sampling_thread_;
+  PlatformThreadHandle sampling_thread_handle_;
+};
+
+}

[Ilya Sherman, 2015/02/03 22:12:10]

nit: This line should be "}  // namespace base".  Please fix this throughout.

[Mike Wittman, 2015/02/04 01:37:10]

Done.

+
+

[Ilya Sherman, 2015/02/03 22:12:09]

nit: Spurious newline.

[Mike Wittman, 2015/02/04 01:37:10]

Done.

+#endif   // BASE_PROFILER_CPU_PROFILER_H_