-
Notifications
You must be signed in to change notification settings - Fork 713
[devtool] introduce datasink class to etdump #8496
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.
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
Changes from 9 commits
90e6322
9597b5f
5da4e5e
9c23038
b06769e
5af97e3
3a77f70
0f8db1e
a40bfd7
d8fe0e3
2899286
f2fbce9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,50 @@ | ||
| /* | ||
| * Copyright (c) Meta Platforms, Inc. and affiliates. | ||
| * All rights reserved. | ||
| * | ||
| * This source code is licensed under the BSD-style license found in the | ||
| * LICENSE file in the root directory of this source tree. | ||
| */ | ||
|
|
||
| #include <executorch/devtools/etdump/buffer_data_sink.h> | ||
| #include <executorch/devtools/etdump/utils.h> | ||
|
|
||
| using ::executorch::runtime::Error; | ||
| using ::executorch::runtime::Result; | ||
|
|
||
| namespace executorch { | ||
| namespace etdump { | ||
|
|
||
| Result<size_t> BufferDataSink::write(const void* ptr, size_t length) { | ||
| if (length == 0) { | ||
| return static_cast<size_t>(-1); | ||
| } | ||
|
|
||
| uint8_t* last_data_end = debug_buffer_.data() + offset_; | ||
|
|
||
| // The beginning of the next data blob must be aligned to the alignment | ||
| uint8_t* cur_data_begin = internal::align_pointer(last_data_end, alighment_); | ||
| uint8_t* cur_data_end = cur_data_begin + length; | ||
|
|
||
| // Raise OutOfResources error if the end of current data blob is out of range. | ||
| if (cur_data_end > debug_buffer_.data() + debug_buffer_.size()) { | ||
| return Error::OutOfResources; | ||
| } | ||
|
|
||
dbort marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| memcpy(cur_data_begin, ptr, length); | ||
|
|
||
| offset_ = cur_data_end - debug_buffer_.data(); | ||
|
|
||
| return (size_t)(cur_data_begin - debug_buffer_.data()); | ||
| } | ||
|
|
||
| Result<size_t> BufferDataSink::get_storage_size() const { | ||
| return debug_buffer_.size(); | ||
| } | ||
|
|
||
| size_t BufferDataSink::get_used_bytes() const { | ||
| return offset_; | ||
| } | ||
|
|
||
| } // namespace etdump | ||
| } // namespace executorch | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,88 @@ | ||||||||||||||
| /* | ||||||||||||||
| * Copyright (c) Meta Platforms, Inc. and affiliates. | ||||||||||||||
| * All rights reserved. | ||||||||||||||
| * | ||||||||||||||
| * This source code is licensed under the BSD-style license found in the | ||||||||||||||
| * LICENSE file in the root directory of this source tree. | ||||||||||||||
| */ | ||||||||||||||
|
|
||||||||||||||
| #pragma once | ||||||||||||||
|
|
||||||||||||||
| #include <executorch/devtools/etdump/data_sink_base.h> | ||||||||||||||
| #include <executorch/runtime/core/exec_aten/exec_aten.h> | ||||||||||||||
| #include <executorch/runtime/core/span.h> | ||||||||||||||
|
|
||||||||||||||
| namespace executorch { | ||||||||||||||
| namespace etdump { | ||||||||||||||
|
|
||||||||||||||
| /** | ||||||||||||||
| * BufferDataSink is a concrete implementation of the DataSinkBase class, | ||||||||||||||
| * designed to store debug data in a pre-allocated, user-owned buffer. This | ||||||||||||||
| * class provides methods to write raw data and tensor data into the buffer, | ||||||||||||||
| * ensuring proper alignment and managing padding as needed. It is the standard | ||||||||||||||
| * DataSink used by ETDumpGen. | ||||||||||||||
Gasoonjia marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||||||||
| */ | ||||||||||||||
| class BufferDataSink : public DataSinkBase { | ||||||||||||||
| public: | ||||||||||||||
| /** | ||||||||||||||
| * Constructs a BufferDataSink with a given 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), alighment_(64) {} | ||||||||||||||
|
||||||||||||||
| : debug_buffer_(buffer), offset_(0), alighment_(64) {} | |
| : debug_buffer_(buffer), offset_(0), alignment_(64) {} |
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May as well add an alignment param. And please document that it must be a power of 2
| explicit BufferDataSink(::executorch::runtime::Span<uint8_t> buffer) | |
| : debug_buffer_(buffer), offset_(0), alighment_(64) {} | |
| explicit BufferDataSink(::executorch::runtime::Span<uint8_t> buffer, size_t alignment = 64) | |
| : debug_buffer_(buffer), offset_(0), alignment_(alignment) {} |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,70 @@ | ||
| /* | ||
| * Copyright (c) Meta Platforms, Inc. and affiliates. | ||
| * All rights reserved. | ||
| * | ||
| * This source code is licensed under the BSD-style license found in the | ||
| * LICENSE file in the root directory of this source tree. | ||
| */ | ||
|
|
||
| #pragma once | ||
|
|
||
| #include <executorch/runtime/core/exec_aten/exec_aten.h> | ||
| #include <executorch/runtime/core/result.h> | ||
|
|
||
| namespace executorch { | ||
| namespace etdump { | ||
|
|
||
| /** | ||
| * DataSinkBase is an abstract class that users can inherit and implement | ||
| * to customize the storage and management of debug data in ETDumpGen. This | ||
| * class provides a basic and essential interface for writing datablob to a | ||
| * user-defined storage, retrieving storage capacity, and tracking the amount of | ||
| * data stored. | ||
| */ | ||
| class DataSinkBase { | ||
| public: | ||
| /** | ||
| * Virtual destructor to ensure proper cleanup of derived classes. | ||
| */ | ||
| virtual ~DataSinkBase() = default; | ||
| /** | ||
| * Write data into the debug storage. This method should be implemented | ||
| * by derived classes to handle the specifics of data storage. | ||
| * | ||
| * This function should return the offset of the starting location of the | ||
| * data within the debug storage if the write operation succeeds, or an | ||
| * Error code if any issue occurs during the write process. | ||
| * | ||
| * @param[in] ptr A pointer to the data to be written into the storage. | ||
| * @param[in] length The size of the data in bytes. | ||
| * @return A Result object containing either: | ||
| * - The offset of the starting location of the data within the | ||
| * debug storage, which will be recorded in the corresponding | ||
| * metadata of ETDump, or | ||
| * - An error code indicating the failure reason, if any issue | ||
| * occurs during the write process. | ||
| */ | ||
| virtual ::executorch::runtime::Result<size_t> write( | ||
| const void* ptr, | ||
| size_t length) = 0; | ||
| /** | ||
| * Get the maximum capacity of the debug storage in bytes. | ||
| * Should return Error::NotSupported if the capacity is not available | ||
|
||
| * (e.g. unbounded storage like internet or file) | ||
| * | ||
| * @return A Result object containing either: | ||
| * - The total size of the debug storage, or | ||
| * - An error code indicating the failure reason, if any issue | ||
| * occurs when retrieving the storage capacity. | ||
| */ | ||
| virtual ::executorch::runtime::Result<size_t> get_storage_size() const = 0; | ||
| /** | ||
| * Get the number of bytes currently used in the debug storage. | ||
| * | ||
| * @return The amount of data currently stored in bytes. | ||
| */ | ||
| virtual size_t get_used_bytes() const = 0; | ||
| }; | ||
|
|
||
| } // namespace etdump | ||
| } // namespace executorch | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From an earlier comment:
@dbort:
@Gasoonjia:
What is "BP"? A behavior change in this new function will not affect anyone, because it is not used yet. If some future caller needs this
(size_t)-1behavior for some reason, then they can deal with that themselves: they know when they're writing zero bytes. I'd prefer not to land potentially-dangerous logic like this, because it might never be fixed.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sry that's a typo. I wanted to type "BC", which refers to "Backward-Compatibility" issue.
This is not a new function. It has existed in the ETDumpGen class (https://github.com/pytorch/executorch/blob/main/devtools/etdump/etdump_flatcc.cpp#L511) for writing tensors into user-owned buffer, and our
buffer_data_sinkextracted that part of logic into our class for maintaining the current behavior.Therefore, I would like to reproduce the existing logic first. I know it is not good enough. Will work on it in the following diffs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But this is a new function that no-one uses yet. If
ETDumpGen::copy_tensor_to_debug_buffer()calls this, then you can isolate the old behavior there, likeThis way, the new method can behave properly from the start, and other users/subclasses of DataSinkBase don't need to duplicate the old behavior.