Skip to content

GitLab

T
tangled
Commit a6bcdc64 authored by oysteine's avatar oysteine Committed by Commit Bot
Browse files
Options

Revert "Reland "Perfetto: Optimization to avoid string table lookup for _END events"" 

This reverts commit c9720564.

Reverting this; looks like the increased bytes used due to stringtable indices being hashes rather than sequential numbering is causing the trace chunks to fill up quicker and negates the optimization.

BUG=903762
Original change's description:
> Reland "Perfetto: Optimization to avoid string table lookup for _END events"
> 
> This reverts commit 7acd0b52.
> 
> Reason for revert: Re-landing with crashfix
> 
> Original change's description:
> > Revert "Perfetto: Optimization to avoid string table lookup for _END events"
> > 
> > This reverts commit a7afe430.
> > 
> > Reason for revert: I suspect this may have caused crashes on https://ci.chromium.org/p/chrome/builders/luci.chrome.ci/linux-perf-fyi#
> > 
> > Original change's description:
> > > Perfetto: Optimization to avoid string table lookup for _END events
> > > 
> > > If we're emitting an _END trace event, we know that the string
> > > table entries for the name and the categories have already been
> > > emitted and so we don't need to check the string table in this
> > > case.
> > > 
> > > R=​primiano@chromium.org,skyostil@chromium.org
> > > 
> > > Change-Id: I9107bffb0b48f7b71d0a42961a78a3861ffcb76f
> > > Reviewed-on: https://chromium-review.googlesource.com/c/1258043
> > > Commit-Queue: oysteine <oysteine@chromium.org>
> > > Reviewed-by: Sami Kyöstilä <skyostil@chromium.org>
> > > Cr-Commit-Position: refs/heads/master@{#600183}
> > 
> > TBR=primiano@chromium.org,oysteine@chromium.org,skyostil@chromium.org
> > 
> > # Not skipping CQ checks because original CL landed > 1 day ago.
> > 
> > Change-Id: Ia460750fcaf338c4bf24ee7317d58b7aeede9520
> > Reviewed-on: https://chromium-review.googlesource.com/c/1293612
> > Reviewed-by: Christian Biesinger <cbiesinger@chromium.org>
> > Commit-Queue: Christian Biesinger <cbiesinger@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#601586}
> 
> TBR=cbiesinger@chromium.org,primiano@chromium.org,oysteine@chromium.org,skyostil@chromium.org
> 
> # Not skipping CQ checks because original CL landed > 1 day ago.
> 
> Change-Id: I8f3b5c21494d66fe9e6c25363627ec156f5abf12
> Reviewed-on: https://chromium-review.googlesource.com/c/1325077
> Commit-Queue: oysteine <oysteine@chromium.org>
> Reviewed-by: oysteine <oysteine@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#606308}

TBR=cbiesinger@chromium.org,primiano@chromium.org,oysteine@chromium.org,skyostil@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Change-Id: I4719cde108d34cee7223615861c1f7cfac31bcd0
Reviewed-on: https://chromium-review.googlesource.com/c/1334199Reviewed-by: default avataroysteine <oysteine@chromium.org>
Commit-Queue: oysteine <oysteine@chromium.org>
Cr-Commit-Position: refs/heads/master@{#607747}
parent b8348987
Show whitespace changes
Inline Side-by-side
Showing with 27 additions and 43 deletions
services/tracing/perfetto/json_trace_exporter.cc
View file @ a6bcdc64
......@@ -83,11 +83,9 @@ const char* GetStringFromStringTable(
const std::unordered_map<int, std::string>& string_table,
int index) {
auto it = string_table.find(index);
if (it != string_table.end()) {
return it->second.c_str();
}
DCHECK(it != string_table.end());
return "UNKNOWN";
return it->second.c_str();
}
void OutputJSONFromTraceEventProto(
......
services/tracing/public/cpp/perfetto/trace_event_data_source.cc
View file @ a6bcdc64
......@@ -7,7 +7,6 @@
#include <map>
#include <utility>
#include "base/hash.h"
#include "base/json/json_writer.h"
#include "base/memory/ref_counted_memory.h"
#include "base/no_destructor.h"
......@@ -130,35 +129,28 @@ class TraceEventDataSource::ThreadLocalEventSink {
trace_packet_handle_ = trace_writer_->NewTracePacket();
event_bundle_ =
ChromeEventBundleHandle(trace_packet_handle_->set_chrome_events());
interned_strings_.clear();
#if DCHECK_IS_ON()
hash_verifier_.clear();
#endif
string_table_.clear();
next_string_table_index_ = 0;
}
uint32_t GetStringTableIndexForString(const char* str_value) {
int GetStringTableIndexForString(const char* str_value) {
EnsureValidHandles();
uint32_t string_table_index = base::Hash(str_value, strlen(str_value));
auto it = string_table_.find(reinterpret_cast<intptr_t>(str_value));
if (it != string_table_.end()) {
CHECK_EQ(std::string(reinterpret_cast<const char*>(it->first)),
std::string(str_value));
if (interned_strings_.find(string_table_index) != interned_strings_.end()) {
#if DCHECK_IS_ON()
DCHECK_EQ(hash_verifier_[string_table_index], str_value);
#endif
return string_table_index;
return it->second;
}
interned_strings_.insert(string_table_index);
int string_table_index = ++next_string_table_index_;
string_table_[reinterpret_cast<intptr_t>(str_value)] = string_table_index;
auto* new_string_table_entry = event_bundle_->add_string_table();
new_string_table_entry->set_value(str_value);
new_string_table_entry->set_index(string_table_index);
#if DCHECK_IS_ON()
hash_verifier_[string_table_index] = str_value;
#endif
return string_table_index;
}
......@@ -191,11 +183,9 @@ class TraceEventDataSource::ThreadLocalEventSink {
EnsureValidHandles();
uint32_t name_index = 0;
uint32_t category_name_index = 0;
uint32_t arg_name_indices[base::trace_event::kTraceMaxNumArgs] = {0};
char phase = trace_event->phase();
int name_index = 0;
int category_name_index = 0;
int arg_name_indices[base::trace_event::kTraceMaxNumArgs] = {0};
// Populate any new string table parts first; has to be done before
// the add_trace_events() call (as the string table is part of the outer
......@@ -206,18 +196,10 @@ class TraceEventDataSource::ThreadLocalEventSink {
bool string_table_enabled =
!(trace_event->flags() & TRACE_EVENT_FLAG_COPY) && thread_will_flush_;
if (string_table_enabled) {
// Optimization: If it's an _END event, we know that the string table
// entries for the name and the category have already been emitted.
if (phase == TRACE_EVENT_PHASE_END) {
name_index = base::Hash(trace_event->name());
category_name_index = base::Hash(TraceLog::GetCategoryGroupName(
trace_event->category_group_enabled()));
} else {
name_index = GetStringTableIndexForString(trace_event->name());
category_name_index =
GetStringTableIndexForString(TraceLog::GetCategoryGroupName(
trace_event->category_group_enabled()));
}
for (int i = 0;
i < base::trace_event::kTraceMaxNumArgs && trace_event->arg_name(i);
......@@ -262,6 +244,7 @@ class TraceEventDataSource::ThreadLocalEventSink {
new_trace_event->set_process_id(process_id);
new_trace_event->set_thread_id(thread_id);
char phase = trace_event->phase();
new_trace_event->set_phase(phase);
for (int i = 0;
......@@ -314,6 +297,11 @@ class TraceEventDataSource::ThreadLocalEventSink {
int64_t duration = trace_event->duration().InMicroseconds();
if (duration != -1) {
new_trace_event->set_duration(duration);
} else {
// TODO(oysteine): Workaround until TRACE_EVENT_PHASE_COMPLETE can be
// split into begin/end pairs. If the duration is -1 and the
// trace-viewer will spend forever generating a warning for each event.
new_trace_event->set_duration(0);
}
if (!trace_event->thread_timestamp().is_null()) {
......@@ -365,10 +353,8 @@ class TraceEventDataSource::ThreadLocalEventSink {
const bool thread_will_flush_;
ChromeEventBundleHandle event_bundle_;
perfetto::TraceWriter::TracePacketHandle trace_packet_handle_;
std::set<uint32_t> interned_strings_;
#if DCHECK_IS_ON()
std::map<uint32_t, std::string> hash_verifier_;
#endif
std::map<intptr_t, int> string_table_;
int next_string_table_index_ = 0;
};
namespace {
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment