Skip to content

[devtool] make ETDumpGen use datasink #8499

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 9 commits into from
Closed

[devtool] make ETDumpGen use datasink #8499

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
e5955e7
[devtool] make ETDumpGen use datasink
Gasoonjia Feb 14, 2025
9bfa193
Update on "[devtool] make ETDumpGen use datasink"
Gasoonjia Feb 16, 2025
9ed69ae
Update on "[devtool] make ETDumpGen use datasink"
Gasoonjia Feb 17, 2025
ec95cb0
Update on "[devtool] make ETDumpGen use datasink"
Gasoonjia Feb 17, 2025
7c15e29
Update on "[devtool] make ETDumpGen use datasink"
Gasoonjia Feb 18, 2025
4328491
Update on "[devtool] make ETDumpGen use datasink"
Gasoonjia Feb 19, 2025
a25d258
Update on "[devtool] make ETDumpGen use datasink"
Gasoonjia Feb 25, 2025
2f7afb0
Update on "[devtool] make ETDumpGen use datasink"
Gasoonjia Feb 25, 2025
56f334c
Update on "[devtool] make ETDumpGen use datasink"
Gasoonjia Feb 26, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion devtools/etdump/buffer_data_sink.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,14 +25,24 @@ namespace etdump {
class BufferDataSink : public DataSinkBase {
public:
/**
* Constructs a BufferDataSink with a given buffer.
* Constructs a BufferDataSink with a given span buffer.
*
* @param[in] buffer A Span object representing the buffer where data will be
* stored.
*/
explicit BufferDataSink(::executorch::runtime::Span<uint8_t> buffer)
: debug_buffer_(buffer), offset_(0) {}

/**
Copy link
Contributor

@tarun292 tarun292 Feb 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there internal users who need this?

Copy link
Contributor Author

@Gasoonjia Gasoonjia Feb 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not right now but introducing this constructor may make our friends life easier and I'd like to make it as a primary entrance in the future

If our user contructs BufferDataSink from Span:

void* ptr = malloc(2048)
Span<uint8_t> span= Span(ptr, 2048)
BufferDataSink bds(span)

However if from the new constructor:

void* ptr = malloc(2048)
BufferDataSink bds(ptr, 2048)

More straightforward and sturcure, user doesn't need to take care about span or anything else.

Copy link
Contributor

@dbort dbort Feb 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that the Span ctor would work with syntax like

void* ptr = malloc(2048);
BufferDataSink bds({ptr, 2048});

Copy link
Contributor Author

@Gasoonjia Gasoonjia Feb 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Program syntactically yes, but user need to figure out what's span is, how to construct it, and how to make the logic shorter, which introduces lots of ramp up cost.
Put into user's shoe, they don't need to knwo what span is. A buffer data sink should be directly constructed on the user-provided memory space, and all type transforming should be happened under-the-neath.

* Constructs a BufferDataSink with a given ptr to data blob, and the size of
* data blob.
*
* @param[in] ptr A pointer to the data blob where data will be stored.
* @param[in] size The size of the data blob in bytes.
*/
BufferDataSink(void* ptr, size_t size)
: debug_buffer_((uint8_t*)ptr, size), offset_(0) {}

BufferDataSink(const BufferDataSink&) = delete;
BufferDataSink& operator=(const BufferDataSink&) = delete;
BufferDataSink(BufferDataSink&&) = default;
Expand Down
61 changes: 36 additions & 25 deletions devtools/etdump/etdump_flatcc.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

#include <cstring>

#include <executorch/devtools/etdump/buffer_data_sink.h>
#include <executorch/devtools/etdump/emitter.h>
#include <executorch/devtools/etdump/etdump_schema_flatcc_builder.h>
#include <executorch/devtools/etdump/etdump_schema_flatcc_reader.h>
Expand All @@ -29,6 +30,7 @@ using ::executorch::runtime::DelegateDebugIdType;
using ::executorch::runtime::EValue;
using ::executorch::runtime::EventTracerEntry;
using ::executorch::runtime::LoggedEValueType;
using ::executorch::runtime::Result;
using ::executorch::runtime::Span;
using ::executorch::runtime::Tag;

Expand Down Expand Up @@ -347,10 +349,10 @@ void ETDumpGen::log_intermediate_output_delegate_helper(
ET_CHECK_MSG(
(name == nullptr) ^ (delegate_debug_index == -1),
"Only name or delegate_debug_index can be valid. Check DelegateMappingBuilder documentation for more details.");
if (debug_buffer_.empty()) {
ET_CHECK_MSG(0, "Must pre-set debug buffer with set_debug_buffer()\n");
return;
}

ET_CHECK_MSG(
data_sink_,
"Must pre-set data sink before logging evalue with set_data_sink() or set_debug_buffer()\n");

check_ready_to_add_events();
int64_t string_id = name != nullptr ? create_string_entry(name) : -1;
Expand All @@ -367,7 +369,7 @@ void ETDumpGen::log_intermediate_output_delegate_helper(

// Check the type of `output` then call the corresponding logging functions
if constexpr (std::is_same<T, Tensor>::value) {
long offset = copy_tensor_to_debug_buffer(output);
long offset = write_tensor_or_raise_error(output);
etdump_Tensor_ref_t tensor_ref = add_tensor_entry(builder_, output, offset);

etdump_Value_start(builder_);
Expand All @@ -377,7 +379,7 @@ void ETDumpGen::log_intermediate_output_delegate_helper(
} else if constexpr (std::is_same<T, ArrayRef<Tensor>>::value) {
etdump_Tensor_vec_start(builder_);
for (size_t i = 0; i < output.size(); ++i) {
long offset = copy_tensor_to_debug_buffer(output[i]);
long offset = write_tensor_or_raise_error(output[i]);
etdump_Tensor_vec_push(
builder_, add_tensor_entry(builder_, output[i], offset));
}
Expand Down Expand Up @@ -497,27 +499,15 @@ ETDumpResult ETDumpGen::get_etdump_data() {
}

void ETDumpGen::set_debug_buffer(Span<uint8_t> buffer) {
Copy link
Contributor

@tarun292 tarun292 Feb 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should remove the set_debug_buffer api. Is there any reason to keep it around? Makes it a little confusing for users i think to have these 2 around.

Copy link
Contributor Author

@Gasoonjia Gasoonjia Feb 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just for BC. Currently all our users use set_debug_buffer to set the memory for debug data, and their work will be impacted if we remove that

Copy link
Contributor

@tarun292 tarun292 Feb 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually we should keep this API around i think. Only users who want to do their custom thing should know about data sinks, otherwise most users should just be able to call set_debug_buffer.

Copy link
Contributor Author

@Gasoonjia Gasoonjia Feb 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

User only need standard data dumping pipeline can use both datasink and set_debug_buffer, since we have provided BufferDataSink which is exactly the same as debug buffer pipeline. But from API design prospective, I would like to deprecate set_debug_buffer api and only focus on data sink for better structure.

debug_buffer_ = buffer;
data_sink_ = std::make_shared<BufferDataSink>(buffer);
Copy link
Contributor

@tarun292 tarun292 Feb 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious why make_shared here. This is only owned internally by this class right?

Copy link
Contributor Author

@Gasoonjia Gasoonjia Feb 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just the underlying data_sink_ is a shared_ptr;

Copy link
Contributor Author

@Gasoonjia Gasoonjia Feb 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our user may still want to use theirdata_sink after ETDumpGen lifecycle, so that I make data_sink_ as shared_ptr.

}

size_t ETDumpGen::copy_tensor_to_debug_buffer(executorch::aten::Tensor tensor) {
if (tensor.nbytes() == 0) {
return static_cast<size_t>(-1);
}
uint8_t* offset_ptr =
internal::alignPointer(debug_buffer_.data() + debug_buffer_offset_, 64);
debug_buffer_offset_ = (offset_ptr - debug_buffer_.data()) + tensor.nbytes();
ET_CHECK_MSG(
debug_buffer_offset_ <= debug_buffer_.size(),
"Ran out of space to store intermediate outputs.");
memcpy(offset_ptr, tensor.const_data_ptr(), tensor.nbytes());
return (size_t)(offset_ptr - debug_buffer_.data());
void ETDumpGen::set_data_sink(std::shared_ptr<DataSinkBase> buffer_data_sink) {
data_sink_ = buffer_data_sink;
}

void ETDumpGen::log_evalue(const EValue& evalue, LoggedEValueType evalue_type) {
if (debug_buffer_.empty()) {
return;
}
ET_CHECK_MSG(data_sink_, "Must set data sink before logging evalue\n");

check_ready_to_add_events();

Expand All @@ -529,7 +519,7 @@ void ETDumpGen::log_evalue(const EValue& evalue, LoggedEValueType evalue_type) {
switch (evalue.tag) {
case Tag::Tensor: {
executorch::aten::Tensor tensor = evalue.toTensor();
long offset = copy_tensor_to_debug_buffer(tensor);
long offset = write_tensor_or_raise_error(tensor);
etdump_Tensor_ref_t tensor_ref =
add_tensor_entry(builder_, tensor, offset);

Expand All @@ -551,7 +541,7 @@ void ETDumpGen::log_evalue(const EValue& evalue, LoggedEValueType evalue_type) {
evalue.toTensorList();
etdump_Tensor_vec_start(builder_);
for (size_t i = 0; i < tensors.size(); ++i) {
long offset = copy_tensor_to_debug_buffer(tensors[i]);
long offset = write_tensor_or_raise_error(tensors[i]);
etdump_Tensor_vec_push(
builder_, add_tensor_entry(builder_, tensors[i], offset));
}
Expand Down Expand Up @@ -636,7 +626,28 @@ bool ETDumpGen::is_static_etdump() {
}

size_t ETDumpGen::get_debug_buffer_size() const {
return debug_buffer_.size();
return ETDumpGen::get_data_sink_size();
}

size_t ETDumpGen::get_data_sink_size() const {
ET_CHECK_MSG(data_sink_, "Must set data sink before checking its size\n");
Result<size_t> ret = data_sink_->get_storage_size();
ET_CHECK_MSG(
ret.ok(),
"Failed to get storage size with error 0x%" PRIx32,
static_cast<uint32_t>(ret.error()));
return ret.get();
}

long ETDumpGen::write_tensor_or_raise_error(Tensor tensor) {
ET_CHECK_MSG(data_sink_, "Must set data sink before writing data\n");
Copy link
Contributor

@tarun292 tarun292 Feb 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit : don't need \n.

Result<size_t> ret =
data_sink_->write(tensor.const_data_ptr(), tensor.nbytes());
ET_CHECK_MSG(
ret.ok(),
"Failed to write tensor with error 0x%" PRIx32,
static_cast<uint32_t>(ret.error()));
return static_cast<long>(ret.get());
}

} // namespace etdump
Expand Down
10 changes: 7 additions & 3 deletions devtools/etdump/etdump_flatcc.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,9 @@
#pragma once

#include <cstdint>
#include <memory>

#include <executorch/devtools/etdump/data_sink_base.h>
#include <executorch/runtime/core/event_tracer.h>
#include <executorch/runtime/core/span.h>
#include <executorch/runtime/platform/platform.h>
Expand Down Expand Up @@ -141,8 +143,10 @@ class ETDumpGen : public ::executorch::runtime::EventTracer {
::executorch::runtime::DebugHandle delegate_debug_index,
const double& output) override;
void set_debug_buffer(::executorch::runtime::Span<uint8_t> buffer);
void set_data_sink(std::shared_ptr<DataSinkBase> buffer_data_sink);
ETDumpResult get_etdump_data();
size_t get_debug_buffer_size() const;
size_t get_data_sink_size() const;
size_t get_num_blocks();
bool is_static_etdump();
void reset();
Expand All @@ -158,7 +162,6 @@ class ETDumpGen : public ::executorch::runtime::EventTracer {

void check_ready_to_add_events();
int64_t create_string_entry(const char* name);
size_t copy_tensor_to_debug_buffer(executorch::aten::Tensor tensor);

/**
* Templated helper function used to log various types of intermediate output.
Expand All @@ -170,10 +173,11 @@ class ETDumpGen : public ::executorch::runtime::EventTracer {
::executorch::runtime::DebugHandle delegate_debug_index,
const T& output);

long write_tensor_or_raise_error(executorch::aten::Tensor tensor);

struct flatcc_builder* builder_;
size_t num_blocks_ = 0;
::executorch::runtime::Span<uint8_t> debug_buffer_;
size_t debug_buffer_offset_ = 0;
std::shared_ptr<DataSinkBase> data_sink_;
int bundled_input_index_ = -1;
State state_ = State::Init;
struct internal::ETDumpStaticAllocator alloc_;
Expand Down
4 changes: 3 additions & 1 deletion devtools/etdump/targets.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ def define_common_targets():

runtime.cxx_library(
name = "buffer_data_sink" + aten_suffix,
headers = [
exported_headers = [
"buffer_data_sink.h",
],
srcs = [
Expand Down Expand Up @@ -153,6 +153,8 @@ def define_common_targets():
exported_deps = [
":etdump_schema_flatcc",
":utils",
":data_sink_base" + aten_suffix,
":buffer_data_sink" + aten_suffix,
"//executorch/runtime/core:event_tracer" + aten_suffix,
"//executorch/runtime/core/exec_aten/util:scalar_type_util" + aten_suffix,
],
Expand Down
2 changes: 1 addition & 1 deletion devtools/etdump/tests/buffer_data_sink_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,5 +119,5 @@ TEST_F(BufferDataSinkTest, WriteUntilOverflow) {
// Attempting to write another tensor should raise an error
Result<size_t> ret =
data_sink_->write(tensor.const_data_ptr(), tensor.nbytes());
ASSERT_EQ(ret.error(), Error::AccessFailed);
ASSERT_EQ(ret.error(), Error::OutOfResources);
}
Loading
Loading