Skip to content

Commit

Permalink
tp: make NotifyEndOfFile return a base::Status
Browse files Browse the repository at this point in the history
This CL changes the public API of trace processor to return a
base::Status from the NotifyEndOfFile method. This allows communicating
errors which happen post all the trace file being pushed into TP (e.g.
with gzip streams, we can return errors that the stream was not
sucessfully parsed).

In the future, this method will be made [[nodiscard]] but for now just
keep it a normal method to not break back-compat.

Change-Id: I24ae97465f25c79ae0e340ca01a0b77cefb87a70
  • Loading branch information
LalitMaganti committed Jul 26, 2024
1 parent 439b00c commit 1e10193
Show file tree
Hide file tree
Showing 45 changed files with 187 additions and 148 deletions.
3 changes: 3 additions & 0 deletions CHANGELOG
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,9 @@ Unreleased:
`android.memory.heap_graph.dominator_tree`. This is to allow for the
addition of more modules related to heap graphs.
Trace Processor:
* Change `NotifyEndOfFile` method to return a Status object. For backwards
compatibility, this value can be ignored but in the future a [[nodiscard]]
annotation will be added.
* Added `CREATE PERFETTO INDEX` to add sqlite-like indexes to Perfetto
tables. Has the same API as `CREATE INDEX`.
UI:
Expand Down
16 changes: 6 additions & 10 deletions include/perfetto/trace_processor/trace_processor_storage.h
Original file line number Diff line number Diff line change
Expand Up @@ -17,18 +17,17 @@
#ifndef INCLUDE_PERFETTO_TRACE_PROCESSOR_TRACE_PROCESSOR_STORAGE_H_
#define INCLUDE_PERFETTO_TRACE_PROCESSOR_TRACE_PROCESSOR_STORAGE_H_

#include <stdint.h>
#include <cstdint>

#include <memory>

#include "perfetto/base/export.h"
#include "perfetto/base/status.h"
#include "perfetto/trace_processor/basic_types.h"
#include "perfetto/trace_processor/status.h"
#include "perfetto/trace_processor/trace_blob.h"
#include "perfetto/trace_processor/trace_blob_view.h"

namespace perfetto {
namespace trace_processor {
namespace perfetto::trace_processor {

// Coordinates the loading of traces from an arbitrary source.
class PERFETTO_EXPORT_COMPONENT TraceProcessorStorage {
Expand Down Expand Up @@ -58,13 +57,10 @@ class PERFETTO_EXPORT_COMPONENT TraceProcessorStorage {
virtual void Flush() = 0;

// Calls Flush and finishes all of the actions required for parsing the trace.
// Should only be called once: in v28, calling this function multiple times
// will simply log an error but in subsequent versions, this will become
// undefined behaviour.
virtual void NotifyEndOfFile() = 0;
// Calling this function multiple times is undefined behaviour.
virtual base::Status NotifyEndOfFile() = 0;
};

} // namespace trace_processor
} // namespace perfetto
} // namespace perfetto::trace_processor

#endif // INCLUDE_PERFETTO_TRACE_PROCESSOR_TRACE_PROCESSOR_STORAGE_H_
5 changes: 2 additions & 3 deletions src/trace_processor/forwarding_trace_parser.cc
Original file line number Diff line number Diff line change
Expand Up @@ -143,12 +143,11 @@ base::Status ForwardingTraceParser::Parse(TraceBlobView blob) {
if (!reader_) {
RETURN_IF_ERROR(Init(blob));
}

return reader_->Parse(std::move(blob));
}

void ForwardingTraceParser::NotifyEndOfFile() {
reader_->NotifyEndOfFile();
base::Status ForwardingTraceParser::NotifyEndOfFile() {
return reader_ ? reader_->NotifyEndOfFile() : base::OkStatus();
}

} // namespace trace_processor
Expand Down
2 changes: 1 addition & 1 deletion src/trace_processor/forwarding_trace_parser.h
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ class ForwardingTraceParser : public ChunkedTraceReader {

// ChunkedTraceReader implementation
base::Status Parse(TraceBlobView) override;
void NotifyEndOfFile() override;
[[nodiscard]] base::Status NotifyEndOfFile() override;

private:
base::Status Init(const TraceBlobView&);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,9 @@
#include "src/trace_processor/importers/android_bugreport/chunked_line_reader.h"

#include <cstddef>
#include <cstdint>
#include <cstring>
#include <utility>

#include "perfetto/base/logging.h"
#include "perfetto/base/status.h"
#include "perfetto/ext/base/status_or.h"
#include "perfetto/ext/base/string_view.h"
Expand Down Expand Up @@ -101,9 +99,10 @@ base::Status ChunkedLineReader::Parse(TraceBlobView data) {
return base::OkStatus();
}

void ChunkedLineReader::NotifyEndOfFile() {
base::Status ChunkedLineReader::NotifyEndOfFile() {
EndOfStream(base::StringView(reinterpret_cast<const char*>(buffer_.data()),
buffer_.size()));
return base::OkStatus();
}

} // namespace perfetto::trace_processor
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ namespace perfetto ::trace_processor {
class ChunkedLineReader : public ChunkedTraceReader {
public:
base::Status Parse(TraceBlobView) final;
void NotifyEndOfFile() final;
base::Status NotifyEndOfFile() final;

// Called for each line in the input. Each line is terminated by a '\n'
// character. The new line character will be included the `line`.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ class ChunkedTraceReader {
virtual base::Status Parse(TraceBlobView) = 0;

// Called after the last Parse() call.
virtual void NotifyEndOfFile() = 0;
[[nodiscard]] virtual base::Status NotifyEndOfFile() = 0;
};

} // namespace perfetto::trace_processor
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
#include <limits>

#include "perfetto/base/logging.h"
#include "perfetto/base/status.h"
#include "perfetto/ext/base/string_view.h"
#include "perfetto/trace_processor/trace_blob.h"
#include "src/trace_processor/importers/common/cpu_tracker.h"
Expand Down Expand Up @@ -848,7 +849,9 @@ void FuchsiaTraceTokenizer::RegisterProvider(uint32_t provider_id,
providers_[provider_id] = std::move(provider);
}

void FuchsiaTraceTokenizer::NotifyEndOfFile() {}
base::Status FuchsiaTraceTokenizer::NotifyEndOfFile() {
return base::OkStatus();
}

} // namespace trace_processor
} // namespace perfetto
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ class FuchsiaTraceTokenizer : public ChunkedTraceReader {

// ChunkedTraceReader implementation
util::Status Parse(TraceBlobView) override;
void NotifyEndOfFile() override;
base::Status NotifyEndOfFile() override;

private:
struct ProviderInfo {
Expand Down
39 changes: 24 additions & 15 deletions src/trace_processor/importers/gzip/gzip_trace_parser.cc
Original file line number Diff line number Diff line change
Expand Up @@ -16,18 +16,24 @@

#include "src/trace_processor/importers/gzip/gzip_trace_parser.h"

#include <cstdint>
#include <cstring>
#include <memory>
#include <string>
#include <utility>

#include "perfetto/base/logging.h"
#include "perfetto/base/status.h"
#include "perfetto/ext/base/string_utils.h"
#include "perfetto/ext/base/string_view.h"
#include "perfetto/trace_processor/trace_blob.h"
#include "perfetto/trace_processor/trace_blob_view.h"
#include "src/trace_processor/forwarding_trace_parser.h"
#include "src/trace_processor/importers/common/chunked_trace_reader.h"
#include "src/trace_processor/util/gzip_utils.h"
#include "src/trace_processor/util/status_macros.h"

namespace perfetto {
namespace trace_processor {
namespace perfetto::trace_processor {

namespace {

Expand All @@ -43,11 +49,11 @@ GzipTraceParser::GzipTraceParser(std::unique_ptr<ChunkedTraceReader> reader)

GzipTraceParser::~GzipTraceParser() = default;

util::Status GzipTraceParser::Parse(TraceBlobView blob) {
base::Status GzipTraceParser::Parse(TraceBlobView blob) {
return ParseUnowned(blob.data(), blob.size());
}

util::Status GzipTraceParser::ParseUnowned(const uint8_t* data, size_t size) {
base::Status GzipTraceParser::ParseUnowned(const uint8_t* data, size_t size) {
const uint8_t* start = data;
size_t len = size;

Expand All @@ -72,7 +78,7 @@ util::Status GzipTraceParser::ParseUnowned(const uint8_t* data, size_t size) {

// Our default uncompressed buffer size is 32MB as it allows for good
// throughput.
constexpr size_t kUncompressedBufferSize = 32 * 1024 * 1024;
constexpr size_t kUncompressedBufferSize = 32ul * 1024 * 1024;

needs_more_input_ = false;
decompressor_.Feed(start, len);
Expand All @@ -88,12 +94,12 @@ util::Status GzipTraceParser::ParseUnowned(const uint8_t* data, size_t size) {
kUncompressedBufferSize - bytes_written_);
ret = result.ret;
if (ret == ResultCode::kError)
return util::ErrStatus("Failed to decompress trace chunk");
return base::ErrStatus("Failed to decompress trace chunk");

if (ret == ResultCode::kNeedsMoreInput) {
PERFETTO_DCHECK(result.bytes_written == 0);
needs_more_input_ = true;
return util::OkStatus();
return base::OkStatus();
}
bytes_written_ += result.bytes_written;

Expand All @@ -103,19 +109,22 @@ util::Status GzipTraceParser::ParseUnowned(const uint8_t* data, size_t size) {
RETURN_IF_ERROR(inner_->Parse(TraceBlobView(std::move(blob))));
}
}
return util::OkStatus();
return base::OkStatus();
}

void GzipTraceParser::NotifyEndOfFile() {
base::Status GzipTraceParser::NotifyEndOfFile() {
// TODO(lalitm): this should really be an error returned to the caller but
// due to historical implementation, NotifyEndOfFile does not return a
// util::Status.
PERFETTO_DCHECK(!needs_more_input_);
// base::Status.
if (needs_more_input_) {
return base::ErrStatus("GZIP stream incomplete, trace is likely corrupt");
}
PERFETTO_DCHECK(!buffer_);

if (inner_)
inner_->NotifyEndOfFile();
if (!inner_) {
base::OkStatus();
}
return inner_->NotifyEndOfFile();
}

} // namespace trace_processor
} // namespace perfetto
} // namespace perfetto::trace_processor
2 changes: 1 addition & 1 deletion src/trace_processor/importers/gzip/gzip_trace_parser.h
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ class GzipTraceParser : public ChunkedTraceReader {

// ChunkedTraceReader implementation
base::Status Parse(TraceBlobView) override;
void NotifyEndOfFile() override;
base::Status NotifyEndOfFile() override;

base::Status ParseUnowned(const uint8_t*, size_t);

Expand Down
7 changes: 5 additions & 2 deletions src/trace_processor/importers/json/json_trace_tokenizer.cc
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
#include <memory>

#include "perfetto/base/build_config.h"
#include "perfetto/base/status.h"
#include "perfetto/ext/base/string_utils.h"

#include "perfetto/trace_processor/trace_blob_view.h"
Expand Down Expand Up @@ -666,8 +667,10 @@ base::Status JsonTraceTokenizer::HandleSystemTraceEvent(const char* start,
return SetOutAndReturn(next, out);
}

void JsonTraceTokenizer::NotifyEndOfFile() {
PERFETTO_DCHECK(position_ == TracePosition::kEof);
base::Status JsonTraceTokenizer::NotifyEndOfFile() {
return position_ == TracePosition::kEof
? base::OkStatus()
: base::ErrStatus("JSON trace file is incomplete");
}

} // namespace trace_processor
Expand Down
2 changes: 1 addition & 1 deletion src/trace_processor/importers/json/json_trace_tokenizer.h
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ class JsonTraceTokenizer : public ChunkedTraceReader {

// ChunkedTraceReader implementation.
base::Status Parse(TraceBlobView) override;
void NotifyEndOfFile() override;
base::Status NotifyEndOfFile() override;

private:
// Enum which tracks which type of JSON trace we are dealing with.
Expand Down
4 changes: 3 additions & 1 deletion src/trace_processor/importers/ninja/ninja_log_parser.cc
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

#include "src/trace_processor/importers/ninja/ninja_log_parser.h"

#include "perfetto/base/status.h"
#include "perfetto/ext/base/string_splitter.h"
#include "perfetto/ext/base/string_utils.h"
#include "src/trace_processor/importers/common/process_tracker.h"
Expand Down Expand Up @@ -112,7 +113,7 @@ util::Status NinjaLogParser::Parse(TraceBlobView blob) {

// This is called after the last Parse() call. At this point all |jobs_| have
// been populated.
void NinjaLogParser::NotifyEndOfFile() {
base::Status NinjaLogParser::NotifyEndOfFile() {
std::sort(jobs_.begin(), jobs_.end(),
[](const Job& x, const Job& y) { return x.start_ms < y.start_ms; });

Expand Down Expand Up @@ -175,6 +176,7 @@ void NinjaLogParser::NotifyEndOfFile() {
ctx_->slice_tracker->Scoped(start_ns, worker->track_id, StringId::Null(),
name_id, dur_ns);
}
return base::OkStatus();
}

} // namespace trace_processor
Expand Down
2 changes: 1 addition & 1 deletion src/trace_processor/importers/ninja/ninja_log_parser.h
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ class NinjaLogParser : public ChunkedTraceReader {

// ChunkedTraceReader implementation
base::Status Parse(TraceBlobView) override;
void NotifyEndOfFile() override;
base::Status NotifyEndOfFile() override;

private:
struct Job {
Expand Down
4 changes: 3 additions & 1 deletion src/trace_processor/importers/perf/perf_data_tokenizer.cc
Original file line number Diff line number Diff line change
Expand Up @@ -442,6 +442,8 @@ base::Status PerfDataTokenizer::ParseFeature(uint8_t feature_id,
return base::OkStatus();
}

void PerfDataTokenizer::NotifyEndOfFile() {}
base::Status PerfDataTokenizer::NotifyEndOfFile() {
return base::OkStatus();
}

} // namespace perfetto::trace_processor::perf_importer
2 changes: 1 addition & 1 deletion src/trace_processor/importers/perf/perf_data_tokenizer.h
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ class PerfDataTokenizer : public ChunkedTraceReader {

// ChunkedTraceReader implementation
base::Status Parse(TraceBlobView) override;
void NotifyEndOfFile() override;
base::Status NotifyEndOfFile() override;

private:
enum class ParsingState {
Expand Down
12 changes: 7 additions & 5 deletions src/trace_processor/importers/proto/proto_importer_module.h
Original file line number Diff line number Diff line change
Expand Up @@ -17,20 +17,22 @@
#ifndef SRC_TRACE_PROCESSOR_IMPORTERS_PROTO_PROTO_IMPORTER_MODULE_H_
#define SRC_TRACE_PROCESSOR_IMPORTERS_PROTO_PROTO_IMPORTER_MODULE_H_

#include <cstdint>
#include <optional>
#include <string>

#include "perfetto/base/logging.h"
#include "perfetto/base/status.h"
#include "perfetto/trace_processor/ref_counted.h"
#include "src/trace_processor/importers/common/trace_parser.h"
#include "src/trace_processor/importers/proto/packet_sequence_state_generation.h"

namespace perfetto {

namespace protos {
namespace pbzero {
namespace protos::pbzero {
class TraceConfig_Decoder;
class TracePacket_Decoder;
} // namespace pbzero
} // namespace protos
} // namespace protos::pbzero

namespace trace_processor {

Expand All @@ -53,7 +55,7 @@ class TraceProcessorContext;
class ModuleResult {
public:
// Allow auto conversion from util::Status to Handled / Error result.
ModuleResult(base::Status status)
ModuleResult(const base::Status& status)
: ignored_(false),
error_(status.ok() ? std::nullopt
: std::make_optional(status.message())) {}
Expand Down
5 changes: 4 additions & 1 deletion src/trace_processor/importers/proto/proto_trace_reader.cc
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@

#include "perfetto/base/build_config.h"
#include "perfetto/base/logging.h"
#include "perfetto/base/status.h"
#include "perfetto/ext/base/flat_hash_map.h"
#include "perfetto/ext/base/string_view.h"
#include "perfetto/ext/base/utils.h"
Expand Down Expand Up @@ -736,7 +737,9 @@ void ProtoTraceReader::ParseTraceStats(ConstBytes blob) {
}
}

void ProtoTraceReader::NotifyEndOfFile() {}
base::Status ProtoTraceReader::NotifyEndOfFile() {
return base::OkStatus();
}

} // namespace trace_processor
} // namespace perfetto
2 changes: 1 addition & 1 deletion src/trace_processor/importers/proto/proto_trace_reader.h
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ class ProtoTraceReader : public ChunkedTraceReader {

// ChunkedTraceReader implementation.
util::Status Parse(TraceBlobView) override;
void NotifyEndOfFile() override;
base::Status NotifyEndOfFile() override;

using SyncClockSnapshots = base::FlatHashMap<
int64_t,
Expand Down
Loading

0 comments on commit 1e10193

Please sign in to comment.