Skip to content

Commit

Permalink
Initialize ThreadDelegatePosix in TracingSamplerProfiler.
Browse files Browse the repository at this point in the history
This is required to initialize information that should be
initialized in the same thread where `TracingSamplerProfiler`
is constructed. For why this is needed, please refer to
https://crbug.com/1392158#c26.

Tracing can be started and stopped at any time. However,
`base::StackSamplingProfiler` -- which is used by Tracing
for stack sampling -- does not support this use case (i.e.
we cannot reuse a `base::StackSamplingProfiler` once it has
been started and stopped).

Therefore, instead of unnecessarily initializing a
`base::StackSamplingProfiler` -- a much "heavier" class than
`base::ThreadDelegatePosix` -- or any other intermediate class
here, we choose to initialize `base::ThreadDelegatePosix`,
which is lightweight and can be initialized without adding too
many additional conditionals.

Testing: I've verified that this fixes things locally. AFAICT,
there isn't a clean way to regression-test this. We need to
create a test case which runs as a separate process, and that
runs before any other unit test that uses
`TracingSamplerProfiler`, so that our test case can initialize
the static variable needed here. Additionally, a browsertest
does not seem practical, too, because there does not seem to
be a clean way to intercept a trace while it is being recorded,
and we'll have to read in the resultant trace after unit
testing ends and manually inspect packets to find out if
stack sampling is working well.

(cherry picked from commit ac7ab36)

Bug: 1392158
Change-Id: I8e7cd0a2fcdd19eb45723d4e2932cd31e56f52f5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4110974
Reviewed-by: Eric Seckler <eseckler@chromium.org>
Reviewed-by: Kartar Singh <kartarsingh@google.com>
Commit-Queue: Tushar Agarwal <agarwaltushar@google.com>
Cr-Original-Commit-Position: refs/heads/main@{#1085507}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4131386
Commit-Queue: Kartar Singh <kartarsingh@google.com>
Cr-Commit-Position: refs/branch-heads/5481@{#105}
Cr-Branched-From: 130f3e4-refs/heads/main@{#1084008}
  • Loading branch information
Tushar Agarwal authored and Chromium LUCI CQ committed Jan 3, 2023
1 parent f6f3d56 commit 840c473
Showing 1 changed file with 14 additions and 0 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,13 @@
#include "third_party/perfetto/protos/perfetto/trace/track_event/process_descriptor.pbzero.h"
#include "third_party/perfetto/protos/perfetto/trace/track_event/thread_descriptor.pbzero.h"

#if BUILDFLAG(IS_POSIX) && !BUILDFLAG(IS_NACL) && !BUILDFLAG(IS_APPLE)
#include "base/profiler/thread_delegate_posix.h"
#define INITIALIZE_THREAD_DELEGATE_POSIX 1
#else // BUILDFLAG(IS_POSIX) && !BUILDFLAG(IS_NACL) && !BUILDFLAG(IS_APPLE)
#define INITIALIZE_THREAD_DELEGATE_POSIX 0
#endif // BUILDFLAG(IS_POSIX) && !BUILDFLAG(IS_NACL) && !BUILDFLAG(IS_APPLE)

#if ANDROID_ARM64_UNWINDING_SUPPORTED || ANDROID_CFI_UNWINDING_SUPPORTED
#include <dlfcn.h>
#include "base/debug/elf_reader.h"
Expand Down Expand Up @@ -837,6 +844,13 @@ TracingSamplerProfiler::TracingSamplerProfiler(
std::move(core_unwinders_factory_function)),
unwinder_type_(unwinder_type) {
DCHECK_NE(sampled_thread_token_.id, base::kInvalidThreadId);
#if INITIALIZE_THREAD_DELEGATE_POSIX
// Since StackSamplingProfiler is scoped to a tracing session and lives on the
// thread where `StartTracing` is called, we use `ThreadDelegatePosix` to
// initialize global data, like the thread stack base address, that has to be
// created on the profiled thread. See crbug.com/1392158#c26 for details.
base::ThreadDelegatePosix::Create(sampled_thread_token_);
#endif // INITIALIZE_THREAD_DELEGATE_POSIX
TracingSamplerProfilerDataSource::Get()->RegisterProfiler(this);
}

Expand Down

0 comments on commit 840c473

Please sign in to comment.