-
Notifications
You must be signed in to change notification settings - Fork 706
[1/N] Add BackendOptions class #11389
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
a28899d
b96d413
ee878bd
cad4964
b5aa9da
dbb4aa5
2fda6a1
d459100
772dd8b
943fe96
2c3a74b
747e037
ecab2ed
c72c199
c73309f
2cc4efe
8c35e52
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,188 @@ | ||
| /* | ||
| * 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/error.h> | ||
| #include <executorch/runtime/core/span.h> | ||
| #include <cstddef> | ||
| #include <cstring> | ||
| #include <variant> | ||
|
|
||
| namespace executorch { | ||
| namespace runtime { | ||
|
|
||
| // Strongly-typed option key template | ||
| template <typename T> | ||
| struct OptionKey { | ||
| using value_type = T; | ||
| const char* key; | ||
cccclai marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| constexpr explicit OptionKey(const char* k) : key(k) {} | ||
| }; | ||
|
|
||
| // All string keys and values must have static storage duration (string | ||
| // literals, static const char arrays, or global constants). The BackendOptions | ||
| // class does NOT take ownership of strings. | ||
| using OptionValue = std::variant<bool, int, const char*>; | ||
| static constexpr size_t kMaxOptionKeyLength = 64; | ||
|
|
||
cccclai marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| struct BackendOption { | ||
| // key is the name of the backend option, like num_threads, enable_profiling, | ||
| // etc | ||
| char key[kMaxOptionKeyLength]; | ||
| // value is the value of the backend option, like 4, true, etc | ||
| OptionValue value; | ||
| }; | ||
|
|
||
| /** | ||
| * A template class for storing and managing backend-specific configuration | ||
| * options. | ||
| * | ||
| * This class provides a type-safe way to store key-value pairs for backend | ||
| * configuration, with compile-time capacity limits and runtime type checking. | ||
| * It supports bool, int, and const char* value types. | ||
| * | ||
| * @tparam MaxCapacity The maximum number of options that can be stored | ||
| */ | ||
| template <size_t MaxCapacity> | ||
| class BackendOptions { | ||
| public: | ||
| /** | ||
| * Default constructor - initializes with zero options. | ||
| */ | ||
| BackendOptions() : size_(0) {} | ||
|
|
||
cccclai marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| /** | ||
| * Returns a const view of all stored options as a Span. | ||
| * | ||
| * @return A const Span containing all BackendOption entries | ||
| */ | ||
| executorch::runtime::Span<BackendOption> view() const { | ||
|
||
| return executorch::runtime::Span<BackendOption>(options_, size_); | ||
| } | ||
|
|
||
| /** | ||
| * Returns a mutable view of all stored options as a Span. | ||
| * | ||
| * @return A mutable Span containing all BackendOption entries | ||
| */ | ||
| executorch::runtime::Span<BackendOption> view() { | ||
|
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. maybe make it explicit that this is a mutable view. 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. Turns out that we pretty much need it to be mutable, because the way options work is we create one options, and option, then pass it to set_option/get_option. I remove the const one. |
||
| return executorch::runtime::Span<BackendOption>(options_, size_); | ||
| } | ||
|
|
||
| /** | ||
| * Sets a boolean option value for the given key. | ||
| * If the key already exists, updates its value. Otherwise, adds a new option. | ||
| * | ||
| * @tparam N The length of the key string (automatically deduced) | ||
| * @param key The option key (must be a string literal or array) | ||
| * @param value The boolean value to set | ||
| * @return Error::Ok on success, Error::InvalidArgument if storage is full | ||
| */ | ||
| template <size_t N> | ||
| Error set_option(const char (&key)[N], bool value) noexcept { | ||
|
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. Do we need N? It will just create N different methods for many possible lengths. My suggestion is to remove N as template param and expect callsites to be something like |
||
| ET_CHECK_MSG(N <= kMaxOptionKeyLength, "Option key is too long"); | ||
|
||
| return set_option_impl(key, value); | ||
| } | ||
|
|
||
| /** | ||
| * Sets an integer option value for the given key. | ||
| * If the key already exists, updates its value. Otherwise, adds a new option. | ||
| * | ||
| * @tparam N The length of the key string (automatically deduced) | ||
| * @param key The option key (must be a string literal or array) | ||
| * @param value The integer value to set | ||
| * @return Error::Ok on success, Error::InvalidArgument if storage is full | ||
| */ | ||
| template <size_t N> | ||
| Error set_option(const char (&key)[N], int value) noexcept { | ||
| ET_CHECK_MSG(N <= kMaxOptionKeyLength, "Option key is too long"); | ||
|
||
| return set_option_impl(key, value); | ||
| } | ||
|
|
||
| /** | ||
| * Sets a string option value for the given key. | ||
| * If the key already exists, updates its value. Otherwise, adds a new option. | ||
| * | ||
| * Note: The string value must have static storage duration. This class does | ||
| * NOT take ownership of the string - it only stores the pointer. | ||
| * | ||
| * @tparam N The length of the key string (automatically deduced) | ||
| * @param key The option key (must be a string literal or array) | ||
| * @param value The string value to set (must have static storage duration) | ||
| * @return Error::Ok on success, Error::InvalidArgument if storage is full | ||
| */ | ||
| template <size_t N> | ||
| Error set_option(const char (&key)[N], const char* value) noexcept { | ||
| ET_CHECK_MSG(N <= kMaxOptionKeyLength, "Option key is too long"); | ||
|
||
| return set_option_impl(key, value); | ||
| } | ||
|
|
||
| /** | ||
| * Retrieves an option value by key and type. | ||
| * | ||
| * @tparam T The expected type of the option value (bool, int, or const char*) | ||
| * @tparam KeyLen The length of the key string (automatically deduced) | ||
| * @param key The option key to look up | ||
| * @param out Reference to store the retrieved value | ||
| * @return Error::Ok if found and type matches, Error::NotFound if key doesn't | ||
| * exist, Error::InvalidArgument if type doesn't match | ||
| */ | ||
| template <typename T, size_t KeyLen> | ||
| Error get_option(const char (&key)[KeyLen], T& out) const { | ||
|
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. same as before on keylen |
||
| ET_CHECK_MSG(KeyLen <= kMaxOptionKeyLength, "Option key is too long"); | ||
|
|
||
| for (size_t i = 0; i < size_; ++i) { | ||
| if (std::strcmp(options_[i].key, key) == 0) { | ||
| if (auto* val = std::get_if<T>(&options_[i].value)) { | ||
| out = *val; | ||
| return Error::Ok; | ||
| } | ||
| return Error::InvalidArgument; | ||
| } | ||
| } | ||
| return Error::NotFound; | ||
| } | ||
|
|
||
| private: | ||
| BackendOption options_[MaxCapacity]{}; // Storage for backend options | ||
| size_t size_; // Current number of options | ||
|
|
||
| /** | ||
| * Internal implementation for setting option values. | ||
| * Handles both updating existing options and adding new ones. | ||
| * | ||
| * @tparam T The type of the value (bool, int, or const char*) | ||
| * @param key The option key | ||
| * @param value The value to set | ||
| * @return Error::Ok on success, Error::InvalidArgument if storage is full | ||
| */ | ||
| template <typename T> | ||
| Error set_option_impl(const char* key, T value) { | ||
| // Update existing if found | ||
| for (size_t i = 0; i < size_; ++i) { | ||
| if (strcmp(options_[i].key, key) == 0) { | ||
| options_[i].value = value; | ||
| return Error::Ok; | ||
| } | ||
| } | ||
| // Add new option if space available | ||
| if (size_ < MaxCapacity) { | ||
| BackendOption new_option; | ||
| strncpy(new_option.key, key, kMaxOptionKeyLength - 1); | ||
| new_option.key[kMaxOptionKeyLength - 1] = '\0'; | ||
|
||
| new_option.value = value; | ||
| options_[size_++] = new_option; | ||
| return Error::Ok; | ||
| } | ||
| // Return error when full | ||
| return Error::InvalidArgument; | ||
| } | ||
| }; | ||
|
|
||
| } // namespace runtime | ||
| } // namespace executorch | ||
| 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/runtime/backend/options.h> | ||
| #include <executorch/runtime/platform/runtime.h> | ||
|
|
||
| #include <gtest/gtest.h> | ||
|
|
||
| using namespace ::testing; | ||
| using executorch::runtime::BackendOptions; | ||
| using executorch::runtime::Error; | ||
|
|
||
| class BackendOptionsTest : public ::testing::Test { | ||
| protected: | ||
| void SetUp() override { | ||
| // Since these tests cause ET_LOG to be called, the PAL must be initialized | ||
| // first. | ||
| executorch::runtime::runtime_init(); | ||
| } | ||
| BackendOptions<5> options; // Capacity of 5 for testing limits | ||
| }; | ||
|
|
||
| // Test basic string functionality | ||
| TEST_F(BackendOptionsTest, HandlesStringOptions) { | ||
| // Set and retrieve valid string | ||
| options.set_option("backend_type", "GPU"); | ||
| const char* result = nullptr; | ||
| EXPECT_EQ(options.get_option("backend_type", result), Error::Ok); | ||
| EXPECT_STREQ(result, "GPU"); | ||
|
|
||
| // Update existing key | ||
| options.set_option("backend_type", "CPU"); | ||
| EXPECT_EQ(options.get_option("backend_type", result), Error::Ok); | ||
| EXPECT_STREQ(result, "CPU"); | ||
| } | ||
|
|
||
| // Test boolean options | ||
| TEST_F(BackendOptionsTest, HandlesBoolOptions) { | ||
| options.set_option("debug", true); | ||
| bool debug = false; | ||
| EXPECT_EQ(options.get_option("debug", debug), Error::Ok); | ||
| EXPECT_TRUE(debug); | ||
|
|
||
| // Test false value | ||
| options.set_option("verbose", false); | ||
| EXPECT_EQ(options.get_option("verbose", debug), Error::Ok); | ||
| EXPECT_FALSE(debug); | ||
| } | ||
|
|
||
| // Test integer options | ||
| TEST_F(BackendOptionsTest, HandlesIntOptions) { | ||
| options.set_option("num_threads", 256); | ||
| int num_threads = 0; | ||
| EXPECT_EQ(options.get_option("num_threads", num_threads), Error::Ok); | ||
| EXPECT_EQ(num_threads, 256); | ||
| } | ||
|
|
||
| // Test error conditions | ||
| TEST_F(BackendOptionsTest, HandlesErrors) { | ||
| // Non-existent key | ||
| bool dummy_bool; | ||
| EXPECT_EQ(options.get_option("missing", dummy_bool), Error::NotFound); | ||
|
|
||
| // Type mismatch | ||
| options.set_option("threshold", 100); | ||
| const char* dummy_str = nullptr; | ||
| EXPECT_EQ(options.get_option("threshold", dummy_str), Error::InvalidArgument); | ||
|
|
||
| // Null value handling | ||
| options.set_option("nullable", static_cast<const char*>(nullptr)); | ||
| EXPECT_EQ(options.get_option("nullable", dummy_str), Error::Ok); | ||
| EXPECT_EQ(dummy_str, nullptr); | ||
| } | ||
|
|
||
| // Test type-specific keys | ||
| TEST_F(BackendOptionsTest, EnforcesKeyTypes) { | ||
| // Same key name - later set operations overwrite earlier ones | ||
| options.set_option("flag", true); | ||
| options.set_option("flag", 123); // Overwrites the boolean entry | ||
|
|
||
| bool bval; | ||
| int ival; | ||
|
|
||
| // Boolean get should fail - type was overwritten to INT | ||
| EXPECT_EQ(options.get_option("flag", bval), Error::InvalidArgument); | ||
|
|
||
| // Integer get should succeed with correct value | ||
| EXPECT_EQ(options.get_option("flag", ival), Error::Ok); | ||
| EXPECT_EQ(ival, 123); | ||
| } | ||
|
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. Add tests for mutability of the options now that you have mutable view 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. Added |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,16 @@ | ||
| load("@fbsource//xplat/executorch/build:runtime_wrapper.bzl", "runtime") | ||
|
|
||
| def define_common_targets(): | ||
| """Defines targets that should be shared between fbcode and xplat. | ||
|
|
||
| The directory containing this targets.bzl file should also contain both | ||
| TARGETS and BUCK files that call this function. | ||
| """ | ||
| pass | ||
| runtime.cxx_test( | ||
| name = "backend_options_test", | ||
| srcs = ["backend_options_test.cpp"], | ||
| deps = [ | ||
| "//executorch/runtime/core:core", | ||
| "//executorch/runtime/backend:interface", | ||
| ], | ||
| ) |
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.
I dont think we need OptionKey as a separate class. Just use
const char *in theBackendOptionclass.However, the thing that worries me the most is by owning
const char*we are giving users a footgun. They can allocate pointer to the backend option name on stack, pass the pointer around and it will go out-of-scope. If this results in segfault that might be your best option. Worst can lead to undefined behaviors.I would suggest that we just store
const char[kMaxLegthOfBackendOptionName].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.
You resolved this comment but I dont see it being addressed
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.
Looks like you addressed but kept
OptionKey. please remove