From 9422e0a4279de31a297c88d515157ea556027ce0 Mon Sep 17 00:00:00 2001 From: Adam Barth Date: Wed, 21 Oct 2015 15:14:19 -0700 Subject: [PATCH] Don't use a \0 to separate base and Dart trace events Instead, we can just concatenate the records together, which means you can view the generated trace file without post-processing. Also, I've made the trace file world-readable, so that you can trace a device without needing root access. --- sky/engine/core/script/dart_controller.cc | 11 +++- sky/shell/android/tracing_controller.cc | 2 - sky/shell/tracing_controller.cc | 71 +++++++++++------------ sky/shell/tracing_controller.h | 10 +++- 4 files changed, 50 insertions(+), 44 deletions(-) diff --git a/sky/engine/core/script/dart_controller.cc b/sky/engine/core/script/dart_controller.cc index 562926f1bc7..dffe265579f 100644 --- a/sky/engine/core/script/dart_controller.cc +++ b/sky/engine/core/script/dart_controller.cc @@ -180,9 +180,14 @@ static void DartController_DartStreamConsumer( } if (state == Dart_StreamConsumer_kData) { - const std::string data(reinterpret_cast(buffer), - buffer_length); - mojo::common::BlockingCopyFromString(data, *handle); + // Trim trailing null characters. + if (buffer[buffer_length - 1] == 0) + --buffer_length; + if (buffer_length) { + const std::string data(reinterpret_cast(buffer), + buffer_length); + mojo::common::BlockingCopyFromString(data, *handle); + } } } diff --git a/sky/shell/android/tracing_controller.cc b/sky/shell/android/tracing_controller.cc index bb20fbf6a18..44cd4147b90 100644 --- a/sky/shell/android/tracing_controller.cc +++ b/sky/shell/android/tracing_controller.cc @@ -16,14 +16,12 @@ namespace sky { namespace shell { static void StartTracing(JNIEnv* env, jclass clazz) { - LOG(INFO) << "Starting trace"; Shell::Shared().tracing_controller().StartTracing(); } static void StopTracing(JNIEnv* env, jclass clazz, jstring path) { base::FilePath file_path(base::android::ConvertJavaStringToUTF8(env, path)); Shell::Shared().tracing_controller().StopTracing(file_path); - LOG(INFO) << "Saving trace to " << file_path.LossyDisplayName(); } bool RegisterTracingController(JNIEnv* env) { diff --git a/sky/shell/tracing_controller.cc b/sky/shell/tracing_controller.cc index 2be8b5203e1..9ef90a4bab0 100644 --- a/sky/shell/tracing_controller.cc +++ b/sky/shell/tracing_controller.cc @@ -2,6 +2,7 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. +#include "base/files/file_util.h" #include "base/logging.h" #include "base/macros.h" #include "base/trace_event/trace_config.h" @@ -20,53 +21,55 @@ const char kBaseTraceEnd[] = "]}"; const char kSentinel[] = "\0"; TracingController::TracingController() - : view_(nullptr), picture_tracing_enabled_(false) { + : view_(nullptr), picture_tracing_enabled_(false), weak_factory_(this) { } TracingController::~TracingController() { } void TracingController::StartTracing() { - DLOG(INFO) << "Collecting Traces"; + LOG(INFO) << "Starting trace"; StartDartTracing(); StartBaseTracing(); } void TracingController::StopTracing(const base::FilePath& path) { - DLOG(INFO) << "Stopping trace collection"; + LOG(INFO) << "Saving trace to " << path.LossyDisplayName(); trace_file_ = std::unique_ptr(new base::File( path, base::File::FLAG_CREATE_ALWAYS | base::File::FLAG_WRITE)); - + base::SetPosixFilePermissions(path, base::FILE_PERMISSION_MASK); StopBaseTracing(); } void TracingController::OnDataAvailable(const void* data, size_t size) { - if (trace_file_ == nullptr) { - return; - } - - trace_file_->WriteAtCurrentPos(reinterpret_cast(data), size); + if (trace_file_) + trace_file_->WriteAtCurrentPos(reinterpret_cast(data), size); } void TracingController::OnDataComplete() { - trace_file_ = nullptr; drainer_ = nullptr; + FinalizeTraceFile(); + LOG(INFO) << "Trace complete"; } void TracingController::StartDartTracing() { - if (view_ != nullptr) { + if (view_) view_->StartDartTracing(); - } } void TracingController::StopDartTracing() { - mojo::DataPipe pipe; - drainer_ = std::unique_ptr( - new mojo::common::DataPipeDrainer(this, pipe.consumer_handle.Pass())); - if (view_ != nullptr) { + if (view_) { + if (trace_file_) + trace_file_->WriteAtCurrentPos(",", 1); + + mojo::DataPipe pipe; + drainer_ = std::unique_ptr( + new mojo::common::DataPipeDrainer(this, pipe.consumer_handle.Pass())); view_->StopDartTracing(pipe.producer_handle.Pass()); + } else { + FinalizeTraceFile(); } } @@ -80,37 +83,33 @@ void TracingController::StopBaseTracing() { base::trace_event::TraceLog* log = base::trace_event::TraceLog::GetInstance(); log->SetDisabled(); - if (trace_file_ != nullptr) { + if (trace_file_) { trace_file_->WriteAtCurrentPos(kBaseTraceStart, sizeof(kBaseTraceStart) - 1); } - log->Flush(base::Bind(&TracingController::OnBaseTraceChunk)); + log->Flush(base::Bind( + &TracingController::OnBaseTraceChunk, weak_factory_.GetWeakPtr())); +} + +void TracingController::FinalizeTraceFile() { + if (trace_file_) { + trace_file_->WriteAtCurrentPos(kBaseTraceEnd, sizeof(kBaseTraceEnd) - 1); + trace_file_ = nullptr; + } } void TracingController::OnBaseTraceChunk( const scoped_refptr& chunk, bool has_more_events) { - // Unfortunately, there does not seem to be a way to pass a user args - // reference to the callback. So we make this static and use the |Shared()| - // accessor - TracingController& controller = Shell::Shared().tracing_controller(); - - if (controller.trace_file_ != nullptr) { + if (trace_file_) { std::string& str = chunk->data(); - controller.trace_file_->WriteAtCurrentPos(str.data(), str.size()); - if (has_more_events) { - controller.trace_file_->WriteAtCurrentPos(",", 1); - } else { - controller.trace_file_->WriteAtCurrentPos(kBaseTraceEnd, - sizeof(kBaseTraceEnd) - 1); - controller.trace_file_->WriteAtCurrentPos(kSentinel, - sizeof(kSentinel) - 1); - } + trace_file_->WriteAtCurrentPos(str.data(), str.size()); + if (has_more_events) + trace_file_->WriteAtCurrentPos(",", 1); } - if (!has_more_events) { - controller.StopDartTracing(); - } + if (!has_more_events) + StopDartTracing(); } void TracingController::RegisterShellView(ShellView* view) { diff --git a/sky/shell/tracing_controller.h b/sky/shell/tracing_controller.h index f69e60b77bd..98ea4a788e0 100644 --- a/sky/shell/tracing_controller.h +++ b/sky/shell/tracing_controller.h @@ -8,6 +8,7 @@ #include "base/files/file.h" #include "base/macros.h" #include "base/memory/ref_counted_memory.h" +#include "base/memory/weak_ptr.h" #include "base/time/time.h" #include "mojo/data_pipe_utils/data_pipe_drainer.h" #include "mojo/public/cpp/system/data_pipe.h" @@ -64,9 +65,12 @@ class TracingController : public mojo::common::DataPipeDrainer::Client { void StopBaseTracing(); void OnDataAvailable(const void* data, size_t num_bytes) override; void OnDataComplete() override; - static void OnBaseTraceChunk( - const scoped_refptr& chunk, - bool has_more_events); + void FinalizeTraceFile(); + + void OnBaseTraceChunk(const scoped_refptr& chunk, + bool has_more_events); + + base::WeakPtrFactory weak_factory_; DISALLOW_COPY_AND_ASSIGN(TracingController); };