From 815b9d7707bce11cd2fd2cb02385765c255d073e Mon Sep 17 00:00:00 2001 From: Jeremy Apthorp Date: Tue, 28 May 2019 14:15:42 -0700 Subject: [PATCH] feat: [contentTracing] allow calling stopTracing() with no arguments (#18411) --- atom/browser/api/atom_api_content_tracing.cc | 73 ++++++++++++-------- atom/common/promise_util.h | 15 ++++ docs/api/content-tracing.md | 51 +++++++------- docs/api/structures/trace-config.md | 52 ++++++++++++-- lib/browser/api/content-tracing.js | 3 - spec/api-content-tracing-spec.js | 5 ++ 6 files changed, 137 insertions(+), 62 deletions(-) diff --git a/atom/browser/api/atom_api_content_tracing.cc b/atom/browser/api/atom_api_content_tracing.cc index c6b83ef4465d..3c4053ea9a1d 100644 --- a/atom/browser/api/atom_api_content_tracing.cc +++ b/atom/browser/api/atom_api_content_tracing.cc @@ -4,6 +4,7 @@ #include #include +#include #include "atom/common/native_mate_converters/callback.h" #include "atom/common/native_mate_converters/file_path_converter.h" @@ -12,6 +13,7 @@ #include "atom/common/promise_util.h" #include "base/bind.h" #include "base/files/file_util.h" +#include "base/optional.h" #include "base/threading/thread_restrictions.h" #include "content/public/browser/tracing_controller.h" #include "native_mate/dictionary.h" @@ -53,36 +55,44 @@ struct Converter { namespace { -using CompletionCallback = base::RepeatingCallback; +using CompletionCallback = base::OnceCallback; -scoped_refptr GetTraceDataEndpoint( - const base::FilePath& path, - const CompletionCallback& callback) { - base::FilePath result_file_path = path; - - // base::CreateTemporaryFile prevents blocking so we need to allow it - // for now since offloading this to a different sequence would require - // changing the api shape - base::ThreadRestrictions::ScopedAllowIO allow_io; - if (result_file_path.empty() && !base::CreateTemporaryFile(&result_file_path)) - LOG(ERROR) << "Creating temporary file failed"; - - return TracingController::CreateFileEndpoint( - result_file_path, base::BindRepeating(callback, result_file_path)); +base::Optional CreateTemporaryFileOnIO() { + base::FilePath temp_file_path; + if (!base::CreateTemporaryFile(&temp_file_path)) + return base::nullopt; + return base::make_optional(std::move(temp_file_path)); } -v8::Local StopRecording(v8::Isolate* isolate, - const base::FilePath& path) { - atom::util::Promise promise(isolate); +void StopTracing(atom::util::Promise promise, + base::Optional file_path) { + if (file_path) { + auto endpoint = TracingController::CreateFileEndpoint( + *file_path, base::AdaptCallbackForRepeating(base::BindOnce( + &atom::util::Promise::ResolvePromise, + std::move(promise), *file_path))); + TracingController::GetInstance()->StopTracing(endpoint); + } else { + promise.RejectWithErrorMessage( + "Failed to create temporary file for trace data"); + } +} + +v8::Local StopRecording(mate::Arguments* args) { + atom::util::Promise promise(args->isolate()); v8::Local handle = promise.GetHandle(); - // TODO(zcbenz): Remove the use of CopyablePromise when the - // CreateFileEndpoint API accepts OnceCallback. - TracingController::GetInstance()->StopTracing(GetTraceDataEndpoint( - path, - base::BindRepeating(atom::util::CopyablePromise::ResolveCopyablePromise< - const base::FilePath&>, - atom::util::CopyablePromise(promise)))); + base::FilePath path; + if (args->GetNext(&path) && !path.empty()) { + StopTracing(std::move(promise), base::make_optional(path)); + } else { + // use a temporary file. + base::PostTaskWithTraitsAndReplyWithResult( + FROM_HERE, {base::MayBlock(), base::TaskPriority::USER_VISIBLE}, + base::BindOnce(CreateTemporaryFileOnIO), + base::BindOnce(StopTracing, std::move(promise))); + } + return handle; } @@ -104,10 +114,15 @@ v8::Local StartTracing( atom::util::Promise promise(isolate); v8::Local handle = promise.GetHandle(); - // Note: This method always succeeds. - TracingController::GetInstance()->StartTracing( - trace_config, base::BindOnce(atom::util::Promise::ResolveEmptyPromise, - std::move(promise))); + if (!TracingController::GetInstance()->StartTracing( + trace_config, base::BindOnce(atom::util::Promise::ResolveEmptyPromise, + std::move(promise)))) { + // If StartTracing returns false, that means it didn't invoke its callback. + // Return an already-resolved promise and abandon the previous promise (it + // was std::move()d into the StartTracing callback and has been deleted by + // this point). + return atom::util::Promise::ResolvedPromise(isolate); + } return handle; } diff --git a/atom/common/promise_util.h b/atom/common/promise_util.h index 376b9331713e..5441fd150ce0 100644 --- a/atom/common/promise_util.h +++ b/atom/common/promise_util.h @@ -82,6 +82,21 @@ class Promise { } } + // Returns an already-resolved promise. + template + static v8::Local ResolvedPromise(v8::Isolate* isolate, + T result) { + Promise resolved(isolate); + resolved.Resolve(result); + return resolved.GetHandle(); + } + + static v8::Local ResolvedPromise(v8::Isolate* isolate) { + Promise resolved(isolate); + resolved.Resolve(); + return resolved.GetHandle(); + } + v8::Local GetHandle() const; v8::Maybe Resolve() { diff --git a/docs/api/content-tracing.md b/docs/api/content-tracing.md index bed372e08216..d150e38338c3 100644 --- a/docs/api/content-tracing.md +++ b/docs/api/content-tracing.md @@ -1,13 +1,11 @@ # contentTracing -> Collect tracing data from Chromium's content module for finding performance -bottlenecks and slow operations. +> Collect tracing data from Chromium to find performance bottlenecks and slow operations. Process: [Main](../glossary.md#main-process) -This module does not include a web interface so you need to open -`chrome://tracing/` in a Chrome browser and load the generated file to view the -result. +This module does not include a web interface. To view recorded traces, use +[trace viewer][], available at `chrome://tracing` in Chrome. **Note:** You should not use this module until the `ready` event of the app module is emitted. @@ -16,20 +14,15 @@ module is emitted. const { app, contentTracing } = require('electron') app.on('ready', () => { - const options = { - categoryFilter: '*', - traceOptions: 'record-until-full,enable-sampling' - } - - contentTracing.startRecording(options, () => { + (async () => { + await contentTracing.startRecording({ + include_categories: ['*'] + }) console.log('Tracing started') - - setTimeout(() => { - contentTracing.stopRecording('', (path) => { - console.log('Tracing data recorded to ' + path) - }) - }, 5000) - }) + await new Promise(resolve => setTimeout(resolve, 5000)) + const path = await contentTracing.stopRecording() + console.log('Tracing data recorded to ' + path) + })() }) ``` @@ -41,11 +34,13 @@ The `contentTracing` module has the following methods: Returns `Promise` - resolves with an array of category groups once all child processes have acknowledged the `getCategories` request -Get a set of category groups. The category groups can change as new code paths are reached. +Get a set of category groups. The category groups can change as new code paths +are reached. See also the [list of built-in tracing +categories](https://chromium.googlesource.com/chromium/src/+/master/base/trace_event/builtin_categories.h). ### `contentTracing.startRecording(options)` -* `options` ([TraceCategoriesAndOptions](structures/trace-categories-and-options.md) | [TraceConfig](structures/trace-config.md)) +* `options` ([TraceConfig](structures/trace-config.md) | [TraceCategoriesAndOptions](structures/trace-categories-and-options.md)) Returns `Promise` - resolved once all child processes have acknowledged the `startRecording` request. @@ -54,22 +49,26 @@ Start recording on all processes. Recording begins immediately locally and asynchronously on child processes as soon as they receive the EnableRecording request. +If a recording is already running, the promise will be immediately resolved, as +only one trace operation can be in progress at a time. + ### `contentTracing.stopRecording(resultFilePath)` -* `resultFilePath` String +* `resultFilePath` String (optional) -Returns `Promise` - resolves with a file that contains the traced data once all child processes have acknowledged the `stopRecording` request +Returns `Promise` - resolves with a path to a file that contains the traced data once all child processes have acknowledged the `stopRecording` request Stop recording on all processes. Child processes typically cache trace data and only rarely flush and send trace data back to the main process. This helps to minimize the runtime overhead of tracing since sending trace data over IPC can be an expensive operation. So, -to end tracing, we must asynchronously ask all child processes to flush any +to end tracing, Chromium asynchronously asks all child processes to flush any pending trace data. -Trace data will be written into `resultFilePath` if it is not empty or into a -temporary file. +Trace data will be written into `resultFilePath`. If `resultFilePath` is empty +or not provided, trace data will be written to a temporary file, and the path +will be returned in the promise. ### `contentTracing.getTraceBufferUsage()` @@ -77,3 +76,5 @@ Returns `Promise` - Resolves with an object containing the `value` and ` Get the maximum usage across processes of trace buffer as a percentage of the full state. + +[trace viewer]: https://github.com/catapult-project/catapult/blob/master/tracing diff --git a/docs/api/structures/trace-config.md b/docs/api/structures/trace-config.md index ad0e7047bfbe..cca937cb6a9f 100644 --- a/docs/api/structures/trace-config.md +++ b/docs/api/structures/trace-config.md @@ -1,9 +1,51 @@ # TraceConfig Object -* `included_categories` String[] (optional) -* `excluded_categories` String[] (optional) -* `memory_dump_config` Object (optional) +* `recording_mode` String (optional) - one of "record-until-full" | "record-continuously" | "record-as-much-as-possible" | "trace-to-console". Defaults to "record-until-full". +* `trace_buffer_size_in_kb` number (optional) - maximum size of the trace + recording buffer in kilobytes. Defaults to 100MB. +* `trace_buffer_size_in_events` number (optional) - maximum size of the trace + recording buffer in events. +* `enable_argument_filter` boolean (optional) - if true, filter event data + according to a whitelist of events that have been manually vetted to not + include any PII. See [the implementation in + Chromium][trace_event_args_whitelist.cc] for specifics. +* `included_categories` String[] (optional) - a list of tracing categories to + include. Can include glob-like patterns using `*` at the end of the category + name. See [tracing categories][] for the list of categories. +* `excluded_categories` String[] (optional) - a list of tracing categories to + exclude. Can include glob-like patterns using `*` at the end of the category + name. See [tracing categories][] for the list of categories. +* `included_process_ids` number[] (optional) - a list of process IDs to + include in the trace. If not specified, trace all processes. +* `histogram_names` String[] (optional) - a list of [histogram][] names to report + with the trace. +* `memory_dump_config` Object (optional) - if the + `disabled-by-default-memory-infra` category is enabled, this contains + optional additional configuration for data callection. See the [Chromium + memory-infra docs][memory-infra docs] for more information. -See an example in the [Chromium docs][1]. +An example TraceConfig that roughly matches what Chrome DevTools records: -[1]: https://chromium.googlesource.com/chromium/src/+/master/docs/memory-infra/memory_infra_startup_tracing.md#the-advanced-way +```js +{ + recording_mode: 'record-until-full', + included_categories: [ + 'devtools.timeline', + 'disabled-by-default-devtools.timeline', + 'disabled-by-default-devtools.timeline.frame', + 'disabled-by-default-devtools.timeline.stack', + 'v8.execute', + 'blink.console', + 'blink.user_timing', + 'latencyInfo', + 'disabled-by-default-v8.cpu_profiler', + 'disabled-by-default-v8.cpu_profiler.hires' + ], + excluded_categories: [ '*' ] +} +``` + +[tracing categories]: https://chromium.googlesource.com/chromium/src/+/master/base/trace_event/builtin_categories.h +[memory-infra docs]: https://chromium.googlesource.com/chromium/src/+/master/docs/memory-infra/memory_infra_startup_tracing.md#the-advanced-way +[trace_event_args_whitelist.cc]: https://chromium.googlesource.com/chromium/src/+/master/services/tracing/public/cpp/trace_event_args_whitelist.cc +[histogram]: https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histograms/README.md diff --git a/lib/browser/api/content-tracing.js b/lib/browser/api/content-tracing.js index 158769d8c1f7..64565f72db3c 100644 --- a/lib/browser/api/content-tracing.js +++ b/lib/browser/api/content-tracing.js @@ -1,5 +1,2 @@ 'use strict' -const { deprecate } = require('electron') -const contentTracing = - module.exports = process.electronBinding('content_tracing') diff --git a/spec/api-content-tracing-spec.js b/spec/api-content-tracing-spec.js index e8efeeee2572..ea4557d37d85 100644 --- a/spec/api-content-tracing-spec.js +++ b/spec/api-content-tracing-spec.js @@ -132,5 +132,10 @@ describe('contentTracing', () => { const resultFilePath = await record(/* options */ {}, /* outputFilePath */ '') expect(resultFilePath).to.be.a('string').that.is.not.empty() }) + + it('creates a temporary file when no path is passed', async function () { + const resultFilePath = await record(/* options */ {}, /* outputFilePath */ undefined) + expect(resultFilePath).to.be.a('string').that.is.not.empty() + }) }) })