[d8] Make CPU profiling more thread-safe
The fuzzer had some issues with the initial CPU profiling code: * The CpuProfiler may get accessed from multiple workers/isolate/threads, which caused a use-after-free if the CpuProfiler gets disposed while some thread is still accessing it. * and profiling may not have been stopped before disposing the profiler, which caused a use-after-free by the profiling thread which did not get stopped. The first issue was caused by storing a global `CpuProfiler*` in d8, which was shared among all threads. This pointer was needed to call `CollectSample`. The solution was to load the D8Console from the isolate, and then load the `CpuProfiler*` from the D8Console. Thereby a thread always loads a thread-local `CpuProfiler*`, which is then guaranteed to exist. The second issue was solved by calling `StopProfiler` before `Dispose` if profiling was still ongoing. R=clemensb@chromium.org Bug: chromium:1394581 Change-Id: Ia17809c7b1a3d72d9da919db5c3d3d5f200c0747 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4065680 Reviewed-by: Clemens Backes <clemensb@chromium.org> Commit-Queue: Andreas Haas <ahaas@chromium.org> Cr-Commit-Position: refs/heads/main@{#84581}
This commit is contained in:
parent
af84cc0b7b
commit
e135163d66
@ -80,7 +80,12 @@ D8Console::D8Console(Isolate* isolate) : isolate_(isolate) {
|
||||
}
|
||||
|
||||
D8Console::~D8Console() {
|
||||
if (profiler_) profiler_->Dispose();
|
||||
if (profiler_) {
|
||||
if (profiler_active_) {
|
||||
profiler_->StopProfiling(String::Empty(isolate_));
|
||||
}
|
||||
profiler_->Dispose();
|
||||
}
|
||||
}
|
||||
|
||||
void D8Console::Assert(const debug::ConsoleCallArguments& args,
|
||||
@ -121,8 +126,8 @@ void D8Console::Profile(const debug::ConsoleCallArguments& args,
|
||||
const v8::debug::ConsoleContext&) {
|
||||
if (!profiler_) {
|
||||
profiler_ = CpuProfiler::New(isolate_);
|
||||
Shell::SetCpuProfiler(profiler_);
|
||||
}
|
||||
profiler_active_ = true;
|
||||
profiler_->StartProfiling(String::Empty(isolate_), CpuProfilingOptions{});
|
||||
}
|
||||
|
||||
@ -130,11 +135,11 @@ void D8Console::ProfileEnd(const debug::ConsoleCallArguments& args,
|
||||
const v8::debug::ConsoleContext&) {
|
||||
if (!profiler_) return;
|
||||
CpuProfile* profile = profiler_->StopProfiling(String::Empty(isolate_));
|
||||
profiler_active_ = false;
|
||||
if (Shell::HasOnProfileEndListener()) {
|
||||
StringOutputStream out;
|
||||
profile->Serialize(&out);
|
||||
Shell::TriggerOnProfileEndListener(isolate_, out.result());
|
||||
Shell::ResetCpuProfiler();
|
||||
} else {
|
||||
FileOutputStream out(kCpuProfileOutputFilename);
|
||||
profile->Serialize(&out);
|
||||
|
@ -19,6 +19,8 @@ class D8Console : public debug::ConsoleDelegate {
|
||||
explicit D8Console(Isolate* isolate);
|
||||
~D8Console() override;
|
||||
|
||||
CpuProfiler* profiler() const { return profiler_; }
|
||||
|
||||
private:
|
||||
void Assert(const debug::ConsoleCallArguments& args,
|
||||
const v8::debug::ConsoleContext&) override;
|
||||
@ -49,6 +51,7 @@ class D8Console : public debug::ConsoleDelegate {
|
||||
std::map<std::string, base::TimeTicks> timers_;
|
||||
base::TimeTicks default_timer_;
|
||||
CpuProfiler* profiler_{nullptr};
|
||||
bool profiler_active_{false};
|
||||
};
|
||||
|
||||
} // namespace v8
|
||||
|
12
src/d8/d8.cc
12
src/d8/d8.cc
@ -467,7 +467,6 @@ const base::TimeTicks Shell::kInitialTicks = base::TimeTicks::Now();
|
||||
Global<Function> Shell::stringify_function_;
|
||||
Global<Function> Shell::profile_end_callback_;
|
||||
Global<Context> Shell::profile_end_callback_context_;
|
||||
CpuProfiler* Shell::cpu_profiler_ = nullptr;
|
||||
base::LazyMutex Shell::workers_mutex_;
|
||||
bool Shell::allow_new_workers_ = true;
|
||||
std::unordered_set<std::shared_ptr<Worker>> Shell::running_workers_;
|
||||
@ -1766,7 +1765,7 @@ uint64_t Shell::GetTracingTimestampFromPerformanceTimestamp(
|
||||
base::TimeDelta::FromMillisecondsD(performance_timestamp);
|
||||
// See TracingController::CurrentTimestampMicroseconds().
|
||||
int64_t internal_value = (delta + kInitialTicks).ToInternalValue();
|
||||
DCHECK(internal_value >= 0);
|
||||
DCHECK_GE(internal_value, 0);
|
||||
return internal_value;
|
||||
}
|
||||
|
||||
@ -2531,10 +2530,11 @@ bool Shell::HasOnProfileEndListener() {
|
||||
|
||||
void Shell::ProfilerTriggerSample(
|
||||
const v8::FunctionCallbackInfo<v8::Value>& args) {
|
||||
if (cpu_profiler_) {
|
||||
Isolate* isolate = args.GetIsolate();
|
||||
cpu_profiler_->CollectSample(isolate);
|
||||
}
|
||||
Isolate* isolate = args.GetIsolate();
|
||||
i::Isolate* i_isolate = reinterpret_cast<i::Isolate*>(isolate);
|
||||
D8Console* console =
|
||||
reinterpret_cast<D8Console*>(i_isolate->console_delegate());
|
||||
if (console->profiler()) console->profiler()->CollectSample(isolate);
|
||||
}
|
||||
|
||||
void Shell::TriggerOnProfileEndListener(Isolate* isolate, std::string profile) {
|
||||
|
@ -608,12 +608,6 @@ class Shell : public i::AllStatic {
|
||||
static void TriggerOnProfileEndListener(Isolate* isolate,
|
||||
std::string profile);
|
||||
|
||||
static void SetCpuProfiler(CpuProfiler* cpu_profiler) {
|
||||
cpu_profiler_ = cpu_profiler;
|
||||
}
|
||||
|
||||
static void ResetCpuProfiler() { cpu_profiler_ = nullptr; }
|
||||
|
||||
static void Print(const v8::FunctionCallbackInfo<v8::Value>& args);
|
||||
static void PrintErr(const v8::FunctionCallbackInfo<v8::Value>& args);
|
||||
static void WriteStdout(const v8::FunctionCallbackInfo<v8::Value>& args);
|
||||
@ -753,7 +747,6 @@ class Shell : public i::AllStatic {
|
||||
|
||||
static Global<Function> profile_end_callback_;
|
||||
static Global<Context> profile_end_callback_context_;
|
||||
static CpuProfiler* cpu_profiler_;
|
||||
|
||||
static const char* stringify_source_;
|
||||
static CounterMap* counter_map_;
|
||||
|
Loading…
Reference in New Issue
Block a user