[logger] Start cleaning up Logger class

- Use unique ptrs for owned objects
- Remove friendship with CpuProfiler and replace with public API
- Remove unused method LogFailure()
- Remove StopProfiler() which was only used by LogFailure() (removed)
  and one test, which can use StopProfilerThread() instead
- Remove 'paused' state which was only used by the above
- Remove 'engage' state. There is no reason we need this as along as
  users keep track of Engage/Disengage calls

Drive-by cleanup:
- Remove import of log.h from profile-generator.h
- Remove unnecessary includes of log.h

Change-Id: Ifc4ca156bef038c40953f8361ffea17788e3a59b
Reviewed-on: https://chromium-review.googlesource.com/c/1424338
Commit-Queue: Peter Marshall <petermarshall@chromium.org>
Reviewed-by: Michael Starzinger <mstarzinger@chromium.org>
Reviewed-by: Jakob Gruber <jgruber@chromium.org>
Cr-Commit-Position: refs/heads/master@{#58957}

src/compiler/wasm-compiler.cc

@@ -35,7 +35,7 @@
 #include "src/heap/factory.h"
 #include "src/interface-descriptors.h"
 #include "src/isolate-inl.h"
-#include "src/log-inl.h"
+#include "src/log.h"
 #include "src/objects/heap-number.h"
 #include "src/optimized-compilation-info.h"
 #include "src/tracing/trace-event.h"

src/debug/debug.cc

@@ -27,7 +27,6 @@
 #include "src/interpreter/bytecode-array-iterator.h"
 #include "src/interpreter/interpreter.h"
 #include "src/isolate-inl.h"
-#include "src/log.h"
 #include "src/message-template.h"
 #include "src/objects/api-callbacks-inl.h"
 #include "src/objects/debug-objects-inl.h"

src/interpreter/interpreter.cc

@@ -14,7 +14,6 @@
 #include "src/counters-inl.h"
 #include "src/interpreter/bytecode-generator.h"
 #include "src/interpreter/bytecodes.h"
-#include "src/log.h"
 #include "src/objects-inl.h"
 #include "src/objects/shared-function-info.h"
 #include "src/objects/slots.h"

src/log.cc

@@ -780,9 +780,6 @@ class Profiler: public base::Thread {
 
   // Inserts collected profiling data into buffer.
   void Insert(v8::TickSample* sample) {
-    if (paused_)
-      return;
-
     if (Succ(head_) == static_cast<int>(base::Relaxed_Load(&tail_))) {
       overflow_ = true;
     } else {
@@ -794,10 +791,6 @@ class Profiler: public base::Thread {
 
   void Run() override;
 
-  // Pause and Resume TickSample data collection.
-  void Pause() { paused_ = true; }
-  void Resume() { paused_ = false; }
-
  private:
   // Waits for a signal and removes profiling data.
   bool Remove(v8::TickSample* sample) {
@@ -824,14 +817,8 @@ class Profiler: public base::Thread {
   // Semaphore used for buffer synchronization.
   base::Semaphore buffer_semaphore_;
 
-  // Tells whether profiler is engaged, that is, processing thread is stated.
-  bool engaged_;
-
   // Tells whether worker thread should continue running.
   base::Atomic32 running_;
-
-  // Tells whether we are currently recording tick samples.
-  bool paused_;
 };
 
 
@@ -843,12 +830,11 @@ class Ticker: public sampler::Sampler {
  public:
   Ticker(Isolate* isolate, int interval_microseconds)
       : sampler::Sampler(reinterpret_cast<v8::Isolate*>(isolate)),
-        profiler_(nullptr),
-        sampling_thread_(new SamplingThread(this, interval_microseconds)) {}
+        sampling_thread_(
+            base::make_unique<SamplingThread>(this, interval_microseconds)) {}
 
   ~Ticker() override {
     if (IsActive()) Stop();
-    delete sampling_thread_;
   }
 
   void SetProfiler(Profiler* profiler) {
@@ -873,8 +859,8 @@ class Ticker: public sampler::Sampler {
   }
 
  private:
-  Profiler* profiler_;
-  SamplingThread* sampling_thread_;
+  Profiler* profiler_ = nullptr;
+  std::unique_ptr<SamplingThread> sampling_thread_;
 };
 
 //
@@ -885,18 +871,12 @@ Profiler::Profiler(Isolate* isolate)
       isolate_(isolate),
       head_(0),
       overflow_(false),
-      buffer_semaphore_(0),
-      engaged_(false),
-      paused_(false) {
+      buffer_semaphore_(0) {
   base::Relaxed_Store(&tail_, 0);
   base::Relaxed_Store(&running_, 0);
 }
 
-
 void Profiler::Engage() {
-  if (engaged_) return;
-  engaged_ = true;
-
   std::vector<base::OS::SharedLibraryAddress> addresses =
       base::OS::GetSharedLibraryAddresses();
   for (const auto& address : addresses) {
@@ -917,8 +897,6 @@ void Profiler::Engage() {
 
 
 void Profiler::Disengage() {
-  if (!engaged_) return;
-
   // Stop receiving ticks.
   isolate_->logger()->ticker_->ClearProfiler();
 
@@ -927,8 +905,6 @@ void Profiler::Disengage() {
   // the thread to terminate.
   base::Relaxed_Store(&running_, 0);
   v8::TickSample sample;
-  // Reset 'paused_' flag, otherwise semaphore may not be signalled.
-  Resume();
   Insert(&sample);
   Join();
 
@@ -952,15 +928,9 @@ void Profiler::Run() {
 
 Logger::Logger(Isolate* isolate)
     : isolate_(isolate),
-      ticker_(nullptr),
-      profiler_(nullptr),
       log_events_(nullptr),
       is_logging_(false),
       log_(nullptr),
-      perf_basic_logger_(nullptr),
-      perf_jit_logger_(nullptr),
-      ll_logger_(nullptr),
-      jit_logger_(nullptr),
       is_initialized_(false),
       existing_code_logger_(isolate) {}
 
@@ -1427,13 +1397,13 @@ void CodeLinePosEvent(JitLogger* jit_logger, Address code_start,
 void Logger::CodeLinePosInfoRecordEvent(Address code_start,
                                         ByteArray source_position_table) {
   SourcePositionTableIterator iter(source_position_table);
-  CodeLinePosEvent(jit_logger_, code_start, iter);
+  CodeLinePosEvent(jit_logger_.get(), code_start, iter);
 }
 
 void Logger::CodeLinePosInfoRecordEvent(
     Address code_start, Vector<const byte> source_position_table) {
   SourcePositionTableIterator iter(source_position_table);
-  CodeLinePosEvent(jit_logger_, code_start, iter);
+  CodeLinePosEvent(jit_logger_.get(), code_start, iter);
 }
 
 void Logger::CodeNameEvent(Address addr, int pos, const char* code_name) {
@@ -1722,21 +1692,6 @@ void Logger::MapDetails(Map map) {
   msg.WriteToLogFile();
 }
 
-void Logger::StopProfiler() {
-  if (!log_->IsEnabled()) return;
-  if (profiler_ != nullptr) {
-    profiler_->Pause();
-    is_logging_ = false;
-    RemoveCodeEventListener(this);
-  }
-}
-
-// This function can be called when Log's mutex is acquired,
-// either from main or Profiler's thread.
-void Logger::LogFailure() {
-  StopProfiler();
-}
-
 static void AddFunctionAndCode(SharedFunctionInfo sfi, AbstractCode code_object,
                                Handle<SharedFunctionInfo>* sfis,
                                Handle<AbstractCode>* code_objects, int offset) {
@@ -1929,21 +1884,21 @@ bool Logger::SetUp(Isolate* isolate) {
   log_ = new Log(this, log_file_name.str().c_str());
 
   if (FLAG_perf_basic_prof) {
-    perf_basic_logger_ = new PerfBasicLogger(isolate);
-    AddCodeEventListener(perf_basic_logger_);
+    perf_basic_logger_.reset(new PerfBasicLogger(isolate));
+    AddCodeEventListener(perf_basic_logger_.get());
   }
 
   if (FLAG_perf_prof) {
-    perf_jit_logger_ = new PerfJitLogger(isolate);
-    AddCodeEventListener(perf_jit_logger_);
+    perf_jit_logger_.reset(new PerfJitLogger(isolate));
+    AddCodeEventListener(perf_jit_logger_.get());
   }
 
   if (FLAG_ll_prof) {
-    ll_logger_ = new LowLevelLogger(isolate, log_file_name.str().c_str());
-    AddCodeEventListener(ll_logger_);
+    ll_logger_.reset(new LowLevelLogger(isolate, log_file_name.str().c_str()));
+    AddCodeEventListener(ll_logger_.get());
   }
 
-  ticker_ = new Ticker(isolate, FLAG_prof_sampling_interval);
+  ticker_.reset(new Ticker(isolate, FLAG_prof_sampling_interval));
 
   if (Log::InitLogAtStart()) {
     is_logging_ = true;
@@ -1952,7 +1907,7 @@ bool Logger::SetUp(Isolate* isolate) {
   timer_.Start();
 
   if (FLAG_prof_cpp) {
-    profiler_ = new Profiler(isolate);
+    profiler_.reset(new Profiler(isolate));
     is_logging_ = true;
     profiler_->Engage();
   }
@@ -1968,14 +1923,13 @@ bool Logger::SetUp(Isolate* isolate) {
 void Logger::SetCodeEventHandler(uint32_t options,
                                  JitCodeEventHandler event_handler) {
   if (jit_logger_) {
-    RemoveCodeEventListener(jit_logger_);
-    delete jit_logger_;
-    jit_logger_ = nullptr;
+    RemoveCodeEventListener(jit_logger_.get());
+    jit_logger_.reset();
   }
 
   if (event_handler) {
-    jit_logger_ = new JitLogger(isolate_, event_handler);
-    AddCodeEventListener(jit_logger_);
+    jit_logger_.reset(new JitLogger(isolate_, event_handler));
+    AddCodeEventListener(jit_logger_.get());
     if (options & kJitCodeEventEnumExisting) {
       HandleScope scope(isolate_);
       LogCodeObjects();
@@ -1984,15 +1938,12 @@ void Logger::SetCodeEventHandler(uint32_t options,
   }
 }
 
-sampler::Sampler* Logger::sampler() {
-  return ticker_;
-}
+sampler::Sampler* Logger::sampler() { return ticker_.get(); }
 
 void Logger::StopProfilerThread() {
   if (profiler_ != nullptr) {
     profiler_->Disengage();
-    delete profiler_;
-    profiler_ = nullptr;
+    profiler_.reset();
   }
 }
 
@@ -2003,31 +1954,26 @@ FILE* Logger::TearDown() {
   // Stop the profiler thread before closing the file.
   StopProfilerThread();
 
-  delete ticker_;
-  ticker_ = nullptr;
+  ticker_.reset();
 
   if (perf_basic_logger_) {
-    RemoveCodeEventListener(perf_basic_logger_);
-    delete perf_basic_logger_;
-    perf_basic_logger_ = nullptr;
+    RemoveCodeEventListener(perf_basic_logger_.get());
+    perf_basic_logger_.reset();
   }
 
   if (perf_jit_logger_) {
-    RemoveCodeEventListener(perf_jit_logger_);
-    delete perf_jit_logger_;
-    perf_jit_logger_ = nullptr;
+    RemoveCodeEventListener(perf_jit_logger_.get());
+    perf_jit_logger_.reset();
   }
 
   if (ll_logger_) {
-    RemoveCodeEventListener(ll_logger_);
-    delete ll_logger_;
-    ll_logger_ = nullptr;
+    RemoveCodeEventListener(ll_logger_.get());
+    ll_logger_.reset();
   }
 
   if (jit_logger_) {
-    RemoveCodeEventListener(jit_logger_);
-    delete jit_logger_;
-    jit_logger_ = nullptr;
+    RemoveCodeEventListener(jit_logger_.get());
+    jit_logger_.reset();
   }
 
   return log_->Close();

src/log.h

@@ -257,14 +257,13 @@ class Logger : public CodeEventListener {
     return is_logging_;
   }
 
+  // Used by CpuProfiler. TODO(petermarshall): Untangle
+  void set_is_logging(bool new_value) { is_logging_ = new_value; }
+
   bool is_listening_to_code_events() override {
     return is_logging() || jit_logger_ != nullptr;
   }
 
-  // Stop collection of profiling data.
-  // When data collection is paused, CPU Tick events are discarded.
-  void StopProfiler();
-
   void LogExistingFunction(Handle<SharedFunctionInfo> shared,
                            Handle<AbstractCode> code);
   // Logs all compiled functions found in the heap.
@@ -280,9 +279,6 @@ class Logger : public CodeEventListener {
   V8_INLINE static CodeEventListener::LogEventsAndTags ToNativeByScript(
       CodeEventListener::LogEventsAndTags, Script);
 
-  // Callback from Log, stops profiling in case of insufficient resources.
-  void LogFailure();
-
   // Used for logging stubs found in the snapshot.
   void LogCodeObject(Object code_object);
 
@@ -321,12 +317,12 @@ class Logger : public CodeEventListener {
   Isolate* isolate_;
 
   // The sampler used by the profiler and the sliding state window.
-  Ticker* ticker_;
+  std::unique_ptr<Ticker> ticker_;
 
   // When the statistical profile is active, profiler_
   // points to a Profiler, that handles collection
   // of samples.
-  Profiler* profiler_;
+  std::unique_ptr<Profiler> profiler_;
 
   // An array of log events names.
   const char* const* log_events_;
@@ -342,10 +338,10 @@ class Logger : public CodeEventListener {
 
   bool is_logging_;
   Log* log_;
-  PerfBasicLogger* perf_basic_logger_;
-  PerfJitLogger* perf_jit_logger_;
-  LowLevelLogger* ll_logger_;
-  JitLogger* jit_logger_;
+  std::unique_ptr<PerfBasicLogger> perf_basic_logger_;
+  std::unique_ptr<PerfJitLogger> perf_jit_logger_;
+  std::unique_ptr<LowLevelLogger> ll_logger_;
+  std::unique_ptr<JitLogger> jit_logger_;
   std::set<int> logged_source_code_;
   uint32_t next_source_info_id_ = 0;
 
@@ -356,8 +352,6 @@ class Logger : public CodeEventListener {
   ExistingCodeLogger existing_code_logger_;
 
   base::ElapsedTimer timer_;
-
-  friend class CpuProfiler;
 };
 
 #define TIMER_EVENTS_LIST(V)     \

src/profiler/cpu-profiler.cc

@@ -14,7 +14,7 @@
 #include "src/deoptimizer.h"
 #include "src/frames-inl.h"
 #include "src/locked-queue-inl.h"
-#include "src/log-inl.h"
+#include "src/log.h"
 #include "src/profiler/cpu-profiler-inl.h"
 #include "src/vm-state-inl.h"
 
@@ -369,8 +369,8 @@ void CpuProfiler::StartProcessorIfNotStarted() {
   }
   Logger* logger = isolate_->logger();
   // Disable logging when using the new implementation.
-  saved_is_logging_ = logger->is_logging_;
-  logger->is_logging_ = false;
+  saved_is_logging_ = logger->is_logging();
+  logger->set_is_logging(false);
 
   bool codemap_needs_initialization = false;
   if (!generator_) {
@@ -423,7 +423,7 @@ void CpuProfiler::StopProcessor() {
   logger->RemoveCodeEventListener(profiler_listener_.get());
   processor_->StopSynchronously();
   processor_.reset();
-  logger->is_logging_ = saved_is_logging_;
+  logger->set_is_logging(saved_is_logging_);
 }
 
 

src/profiler/profile-generator.cc

@@ -88,20 +88,22 @@ base::LazyDynamicInstance<CodeEntry,
     CodeEntry::kUnresolvedEntry = LAZY_DYNAMIC_INSTANCE_INITIALIZER;
 
 CodeEntry* CodeEntry::ProgramEntryCreateTrait::Create() {
-  return new CodeEntry(Logger::FUNCTION_TAG, CodeEntry::kProgramEntryName);
+  return new CodeEntry(CodeEventListener::FUNCTION_TAG,
+                       CodeEntry::kProgramEntryName);
 }
 
 CodeEntry* CodeEntry::IdleEntryCreateTrait::Create() {
-  return new CodeEntry(Logger::FUNCTION_TAG, CodeEntry::kIdleEntryName);
+  return new CodeEntry(CodeEventListener::FUNCTION_TAG,
+                       CodeEntry::kIdleEntryName);
 }
 
 CodeEntry* CodeEntry::GCEntryCreateTrait::Create() {
-  return new CodeEntry(Logger::BUILTIN_TAG,
+  return new CodeEntry(CodeEventListener::BUILTIN_TAG,
                        CodeEntry::kGarbageCollectorEntryName);
 }
 
 CodeEntry* CodeEntry::UnresolvedEntryCreateTrait::Create() {
-  return new CodeEntry(Logger::FUNCTION_TAG,
+  return new CodeEntry(CodeEventListener::FUNCTION_TAG,
                        CodeEntry::kUnresolvedFunctionName);
 }
 

src/profiler/profile-generator.h

@@ -16,7 +16,8 @@
 
 #include "include/v8-profiler.h"
 #include "src/allocation.h"
-#include "src/log.h"
+#include "src/builtins/builtins.h"
+#include "src/code-events.h"
 #include "src/profiler/strings-storage.h"
 #include "src/source-position.h"
 
@@ -194,7 +195,7 @@ class CodeEntry {
   static base::LazyDynamicInstance<CodeEntry, UnresolvedEntryCreateTrait>::type
       kUnresolvedEntry;
 
-  using TagField = BitField<Logger::LogEventsAndTags, 0, 8>;
+  using TagField = BitField<CodeEventListener::LogEventsAndTags, 0, 8>;
   using BuiltinIdField = BitField<Builtins::Name, 8, 23>;
   using UsedField = BitField<bool, 31, 1>;
 
@@ -337,6 +338,7 @@ class ProfileTree {
   DISALLOW_COPY_AND_ASSIGN(ProfileTree);
 };
 
+class CpuProfiler;
 
 class CpuProfile {
  public:

test/cctest/test-log-stack-tracer.cc

@@ -33,7 +33,6 @@
 #include "src/api-inl.h"
 #include "src/disassembler.h"
 #include "src/isolate.h"
-#include "src/log.h"
 #include "src/objects-inl.h"
 #include "src/v8.h"
 #include "src/vm-state-inl.h"

test/cctest/test-log.cc

@@ -458,7 +458,7 @@ TEST(EquivalenceOfLoggingAndTraversal) {
         "  obj.test =\n"
         "    (function a(j) { return function b() { return j; } })(100);\n"
         "})(this);");
-    logger.logger()->StopProfiler();
+    logger.logger()->StopProfilerThread();
     CcTest::PreciseCollectAllGarbage();
     logger.StringEvent("test-logging-done", "");