-
Notifications
You must be signed in to change notification settings - Fork 522
etdumpfilter impl #9752
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
etdumpfilter impl #9752
Changes from all commits
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,95 @@ | ||
/* | ||
* 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/etdump_filter.h> | ||
|
||
#include <executorch/runtime/core/error.h> | ||
|
||
using ::executorch::runtime::DelegateDebugIntId; | ||
using ::executorch::runtime::Error; | ||
using ::executorch::runtime::kUnsetDelegateDebugIntId; | ||
|
||
namespace executorch { | ||
namespace etdump { | ||
Comment on lines
+17
to
+18
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we have C++17 -- |
||
|
||
ETDumpFilter::ETDumpFilter() = default; | ||
|
||
Result<bool> ETDumpFilter::add_regex(string_view pattern) { | ||
auto regex = std::make_unique<re2::RE2>(pattern.data()); | ||
if (!regex->ok()) { | ||
return Error::InvalidArgument; // Error during regex compilation | ||
} | ||
regex_patterns_.emplace_back(std::move(regex)); | ||
return true; | ||
} | ||
|
||
Result<bool> ETDumpFilter::set_debug_handle_range(size_t start, size_t end) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what does a successful result of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. false will not be use. |
||
if (start >= end) { | ||
return Error::InvalidArgument; // Start is greater than end | ||
} | ||
if (start < 0 || end < 0) { | ||
return Error::InvalidArgument; // Start or end is negative | ||
} | ||
range_start_ = start; | ||
range_end_ = end; | ||
return true; | ||
} | ||
|
||
Result<bool> ETDumpFilter::filter_name_(const char* name) { | ||
if (name == nullptr) { | ||
return Error::InvalidArgument; | ||
} | ||
if (regex_patterns_.empty()) { | ||
return true; | ||
} | ||
for (const auto& regex : regex_patterns_) { | ||
if (RE2::FullMatch(name, *regex)) { | ||
return true; | ||
} | ||
} | ||
return false; | ||
} | ||
|
||
Result<bool> ETDumpFilter::filter_delegate_debug_index_( | ||
DelegateDebugIntId debug_handle) { | ||
if (debug_handle == kUnsetDelegateDebugIntId) { | ||
return Error::InvalidArgument; // Delegate debug index is unset | ||
} | ||
|
||
if (range_start_ == 0 && range_end_ == 0) { | ||
return true; | ||
} | ||
|
||
if (debug_handle < range_start_ || debug_handle >= range_end_) { | ||
return false; | ||
} | ||
|
||
return true; | ||
} | ||
|
||
Result<bool> ETDumpFilter::filter( | ||
const char* name, | ||
DelegateDebugIntId delegate_debug_index) { | ||
if ((name == nullptr) == (delegate_debug_index == kUnsetDelegateDebugIntId)) { | ||
return Error::InvalidArgument; // Name and delegate debug index should be | ||
// both set or unset | ||
} | ||
|
||
if (name) { | ||
return filter_name_(name); | ||
} else { | ||
return filter_delegate_debug_index_(delegate_debug_index); | ||
} | ||
} | ||
|
||
size_t ETDumpFilter::get_n_regex() const { | ||
return regex_patterns_.size(); | ||
} | ||
|
||
} // namespace etdump | ||
} // namespace executorch |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,102 @@ | ||
/* | ||
* 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 <re2/re2.h> | ||
#include <memory> | ||
|
||
#include <executorch/runtime/core/event_tracer.h> | ||
#include <executorch/runtime/core/result.h> | ||
#include <executorch/runtime/platform/platform.h> | ||
|
||
namespace executorch::etdump { | ||
|
||
using ::executorch::aten::string_view; | ||
using ::executorch::runtime::Result; | ||
|
||
/** | ||
* ETDumpFilter is a class that filters intermediate output based on output's | ||
* name by full regex filtering, or delegate debug indices by range-based | ||
* filtering. | ||
* | ||
* Note that this filter supports up to MAX_REGEX_PATTERNS regex patterns with a | ||
* maximum length of MAX_PATTERN_LENGTH characters each. | ||
*/ | ||
|
||
class ETDumpFilter : public ::executorch::runtime::EventTracerFilterBase { | ||
public: | ||
ETDumpFilter(); | ||
~ETDumpFilter() override = default; | ||
/** | ||
* Adds a regex pattern to the filter. | ||
* | ||
* @param[in] pattern A c string representing the regex pattern to be added. | ||
* | ||
* @return A Result<bool> indicating the success or failure of adding the | ||
* regex pattern. | ||
* - True if the pattern is successfully added. | ||
* - False if the pattern could not be added or if the maximum number | ||
* of patterns is exceeded. | ||
* - An error code if number of pattern has reached to cap, or any | ||
* error occurs during regex compilation. | ||
*/ | ||
Result<bool> add_regex(string_view pattern); | ||
/** | ||
* Sets the range for the delegate debug index filtering as [start, end). | ||
* Note that this function will flush the existing range. | ||
* | ||
* @param[in] start The start of the range for filtering. | ||
* @param[in] end The end of the range for filtering. | ||
* | ||
* @return A Result<bool> indicating the success or failure of setting the | ||
* range. | ||
* - True if the range is successfully set. | ||
* - An error code if an error occurs. | ||
*/ | ||
Result<bool> set_debug_handle_range(size_t start, size_t end); | ||
|
||
/** | ||
* Filters events based on the given name or delegate debug index. | ||
* | ||
* Note that everytime only one of either the name or delegate_debug_index | ||
* should be passed in. | ||
* | ||
* @param[in] name A pointer to a string representing the `name` of the | ||
* event. If `delegate_debug_index` is not set to kUnsetDebugHandle, `name` | ||
* should be set to nullptr. | ||
* | ||
* @param[in] delegate_debug_index A DebugHandle representing the debug index | ||
* of the delegate. If `name` is not nullptr, this should be set to | ||
* kUnsetDebugHandle. | ||
* | ||
* @return A Result<bool> indicating whether the event matches the filter | ||
* criteria. | ||
* - True if the event matches the filter, or filter is unset. | ||
* - False if the event does not match or is unknown. | ||
* - An error code if an error occurs during filtering. | ||
*/ | ||
Result<bool> filter( | ||
const char* name, | ||
::executorch::runtime::DelegateDebugIntId delegate_debug_index) override; | ||
|
||
/** | ||
* Returns the number of regex patterns in the filter. | ||
*/ | ||
size_t get_n_regex() const; | ||
|
||
private: | ||
std::vector<std::unique_ptr<re2::RE2>> regex_patterns_; | ||
size_t range_start_ = 0; | ||
size_t range_end_ = 0; | ||
Result<bool> filter_name_(const char* name); | ||
Result<bool> filter_delegate_debug_index_( | ||
::executorch::runtime::DelegateDebugIntId delegate_debug_index); | ||
}; | ||
|
||
} // namespace executorch::etdump |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,120 @@ | ||
/* | ||
* 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 <gtest/gtest.h> | ||
|
||
#include <executorch/devtools/etdump/etdump_filter.h> | ||
#include <executorch/runtime/platform/runtime.h> | ||
|
||
#include <cstring> | ||
|
||
using ::executorch::etdump::ETDumpFilter; | ||
using ::executorch::runtime::Error; | ||
using ::executorch::runtime::kUnsetDelegateDebugIntId; | ||
using ::executorch::runtime::Result; | ||
|
||
class ETDumpFilterTest : public ::testing::Test { | ||
protected: | ||
ETDumpFilter filter; | ||
|
||
void SetUp() override { | ||
torch::executor::runtime_init(); | ||
} | ||
|
||
void TearDown() override {} | ||
}; | ||
|
||
TEST_F(ETDumpFilterTest, AddRegexPatternSuccess) { | ||
Result<bool> result = filter.add_regex("test.*"); | ||
EXPECT_TRUE(result.ok()); | ||
EXPECT_TRUE(result.get()); | ||
} | ||
|
||
TEST_F(ETDumpFilterTest, SetDebugHandleRangeSuccess) { | ||
Result<bool> result = filter.set_debug_handle_range(10, 20); | ||
EXPECT_TRUE(result.ok()); | ||
EXPECT_TRUE(result.get()); | ||
} | ||
|
||
TEST_F(ETDumpFilterTest, SetDebugHandleRangeFailure) { | ||
Result<bool> result = filter.set_debug_handle_range(20, 10); | ||
EXPECT_EQ(result.error(), Error::InvalidArgument); | ||
} | ||
|
||
TEST_F(ETDumpFilterTest, FilterByNameSuccess) { | ||
filter.add_regex("event.*"); | ||
Result<bool> result = filter.filter("event_name", kUnsetDelegateDebugIntId); | ||
EXPECT_TRUE(result.ok()); | ||
EXPECT_TRUE(result.get()); | ||
} | ||
|
||
TEST_F(ETDumpFilterTest, PartialMatchingFailed) { | ||
filter.add_regex("event.*"); | ||
Result<bool> result = | ||
filter.filter("non_matching_event", kUnsetDelegateDebugIntId); | ||
EXPECT_TRUE(result.ok()); | ||
EXPECT_FALSE(result.get()); | ||
} | ||
|
||
TEST_F(ETDumpFilterTest, FilterByDelegateDebugIndexSuccess) { | ||
filter.set_debug_handle_range(10, 20); | ||
Result<bool> result = filter.filter(nullptr, 15); | ||
EXPECT_TRUE(result.ok()); | ||
EXPECT_TRUE(result.get()); | ||
} | ||
|
||
TEST_F(ETDumpFilterTest, FilterByDelegateDebugIndexFailure) { | ||
filter.set_debug_handle_range(10, 20); | ||
Result<bool> result = filter.filter(nullptr, 25); | ||
EXPECT_TRUE(result.ok()); | ||
EXPECT_FALSE(result.get()); | ||
} | ||
|
||
TEST_F(ETDumpFilterTest, NaiveFilterNameInputCanSucceed) { | ||
Result<bool> result = filter.filter("any_input", kUnsetDelegateDebugIntId); | ||
EXPECT_TRUE(result.ok()); | ||
EXPECT_TRUE(result.get()); | ||
} | ||
|
||
TEST_F(ETDumpFilterTest, NaiveFilterDebugHandleInputCanSucceed) { | ||
Result<bool> result = filter.filter(nullptr, 12345); | ||
EXPECT_TRUE(result.ok()); | ||
EXPECT_TRUE(result.get()); | ||
} | ||
|
||
TEST_F(ETDumpFilterTest, IllegalInput) { | ||
filter.add_regex("pattern"); | ||
Result<bool> result = filter.filter("matching_event", 1); | ||
EXPECT_EQ(result.error(), Error::InvalidArgument); | ||
} | ||
|
||
TEST_F(ETDumpFilterTest, NoMatchFirstThenMatch) { | ||
filter.add_regex("non_matching_pattern"); | ||
Result<bool> result_1 = | ||
filter.filter("matching_event", kUnsetDelegateDebugIntId); | ||
EXPECT_TRUE(result_1.ok()); | ||
EXPECT_FALSE(result_1.get()); | ||
filter.add_regex("matching_.*"); | ||
Result<bool> result_2 = | ||
filter.filter("matching_event", kUnsetDelegateDebugIntId); | ||
EXPECT_TRUE(result_2.ok()); | ||
EXPECT_TRUE(result_2.get()); | ||
} | ||
|
||
TEST_F(ETDumpFilterTest, MatchRegexFirstThen) { | ||
filter.add_regex("matching.*"); | ||
Result<bool> result_1 = | ||
filter.filter("matching_event", kUnsetDelegateDebugIntId); | ||
EXPECT_TRUE(result_1.ok()); | ||
EXPECT_TRUE(result_1.get()); | ||
filter.add_regex("non_matching_pattern"); | ||
Result<bool> result_2 = | ||
filter.filter("matching_event", kUnsetDelegateDebugIntId); | ||
EXPECT_TRUE(result_2.ok()); | ||
EXPECT_TRUE(result_2.get()); | ||
} |
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.
Overall high level comment, i think it's reasonable that we can assume the etdump filter we provide based on regex will need malloc to be supported. If users want to use this malloc support will be required on their platform. Based on this i think it would be good to remove the limitations we have in this class such as max regex, max char len etc.
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.
If users do need a filter for their platform that isn't based on regex they can write their own one. (which will be only very few embedded users).
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.
ok in this case i agree we can make our etdump_filter support regex + malloc, just as what swolchok mentioned RE use malloc internally. Let me make an update