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 all 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
2 changes: 2 additions & 0 deletions devtools/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,8 @@ add_custom_command(
add_library(
etdump ${CMAKE_CURRENT_SOURCE_DIR}/etdump/etdump_flatcc.cpp
${CMAKE_CURRENT_SOURCE_DIR}/etdump/emitter.cpp
${CMAKE_CURRENT_SOURCE_DIR}/etdump/buffer_data_sink.cpp
${CMAKE_CURRENT_SOURCE_DIR}/etdump/buffer_data_sink.h
)

target_link_libraries(
Expand Down
6 changes: 6 additions & 0 deletions devtools/etdump/buffer_data_sink.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,12 @@ Result<BufferDataSink> BufferDataSink::create(
return BufferDataSink(buffer, alignment);
}

Result<BufferDataSink> BufferDataSink::create(
void* ptr, size_t size,
size_t alignment) noexcept {
return BufferDataSink::create({(uint8_t*)ptr, size}, alignment);
}

Result<size_t> BufferDataSink::write(const void* ptr, size_t length) {
if (length == 0) {
return offset_;
Expand Down
16 changes: 16 additions & 0 deletions devtools/etdump/buffer_data_sink.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,22 @@ class BufferDataSink : public DataSinkBase {
::executorch::runtime::Span<uint8_t> buffer,
size_t alignment = 64) noexcept;

/**
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.

* Creates a BufferDataSink with a given span buffer.
*
* @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.
* @param[in] alignment The alignment requirement for the buffer. It must be
* a power of two and greater than zero. Default is 64.
* @return A Result object containing either:
* - A BufferDataSink object if succees, or
* - An error code indicating the failure reason, if any issue
* occurs during the creation process.
*/
static ::executorch::runtime::Result<BufferDataSink> create(
void* ptr, size_t size,
size_t alignment = 64) noexcept;

// Uncopiable and unassignable to avoid double assignment and free of the
// internal buffer.
BufferDataSink(const BufferDataSink&) = delete;
Expand Down
65 changes: 39 additions & 26 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>(std::move(BufferDataSink::create(buffer).get()));
}

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::align_pointer(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 @@ -635,8 +625,31 @@ bool ETDumpGen::is_static_etdump() {
return alloc_.data != nullptr;
}

size_t ETDumpGen::get_debug_buffer_size() const {
return debug_buffer_.size();
std::shared_ptr<DataSinkBase> ETDumpGen::get_data_sink() {
return data_sink_;
}

long ETDumpGen::write_tensor_or_raise_error(Tensor tensor) {
// Previously, the function copy_tensor_to_debug_buffer returned 0xFF..F when
// given an empty tensor, which is an invalid offset for most buffers. In our
// data sink, we will return the current debug_buffer_offset for better
// clarity. We are isolating the empty tensor case here using the old logic to
// avoid any backward compatibility issues while introducing the data sink.
// Once the data sink is fully implemented, we can remove this check and apply
// the new logic to all cases.
// TODO(gasoonjia): remove this check after datasink is fully rolled out.
if (tensor.nbytes() == 0) {
return static_cast<size_t>(-1);
}

ET_CHECK_MSG(data_sink_, "Must set data sink before writing data");
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
11 changes: 7 additions & 4 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,9 +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_num_blocks();
std::shared_ptr<DataSinkBase> get_data_sink();
bool is_static_etdump();
void reset();

Expand All @@ -158,7 +161,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 +172,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
Loading
Loading