Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit c589dc40 authored by Ady Abraham's avatar Ady Abraham
Browse files

SF: cleanup FlagManager

We currently use FlagManager just for boolean flags.
Remove the templated functions in favot of simpler ones.
Add macros for code reuse.

Test: presubmit
Change-Id: Iba892d981960b653466f55a52ed74a16138da75c
parent 53b0c49e
Loading
Loading
Loading
Loading
+52 −62
Original line number Diff line number Diff line
@@ -28,7 +28,6 @@

namespace android {
static constexpr const char* kExperimentNamespace = "surface_flinger_native_boot";
static constexpr const int64_t kDemoFlag = -1;

std::unique_ptr<FlagManager> FlagManager::mInstance;
std::once_flag FlagManager::mOnce;
@@ -36,40 +35,8 @@ std::once_flag FlagManager::mOnce;
FlagManager::FlagManager(ConstructorTag) {}
FlagManager::~FlagManager() = default;

FlagManager& FlagManager::getInstance() {
    std::call_once(mOnce, [&] {
        LOG_ALWAYS_FATAL_IF(mInstance, "Instance already created");
        mInstance = std::make_unique<FlagManager>(ConstructorTag{});
    });

    return *mInstance;
}

void FlagManager::dump(std::string& result) const {
    base::StringAppendF(&result, "FlagManager values: \n");
    base::StringAppendF(&result, "demo_flag: %" PRId64 "\n", demo_flag());
    base::StringAppendF(&result, "use_adpf_cpu_hint: %s\n", use_adpf_cpu_hint() ? "true" : "false");
    base::StringAppendF(&result, "use_skia_tracing: %s\n", use_skia_tracing() ? "true" : "false");
}

namespace {
template <typename T>
std::optional<T> doParse(const char* str);

template <>
[[maybe_unused]] std::optional<int32_t> doParse(const char* str) {
    int32_t ret;
    return base::ParseInt(str, &ret) ? std::make_optional(ret) : std::nullopt;
}

template <>
[[maybe_unused]] std::optional<int64_t> doParse(const char* str) {
    int64_t ret;
    return base::ParseInt(str, &ret) ? std::make_optional(ret) : std::nullopt;
}

template <>
[[maybe_unused]] std::optional<bool> doParse(const char* str) {
std::optional<bool> parseBool(const char* str) {
    base::ParseBoolResult parseResult = base::ParseBool(str);
    switch (parseResult) {
        case base::ParseBoolResult::kTrue:
@@ -80,44 +47,67 @@ template <>
            return std::nullopt;
    }
}

void dumpFlag(std::string& result, const char* name, std::function<bool()> getter) {
    base::StringAppendF(&result, "%s: %s\n", name, getter() ? "true" : "false");
}

} // namespace

std::string FlagManager::getServerConfigurableFlag(const std::string& experimentFlagName) const {
    return server_configurable_flags::GetServerConfigurableFlag(kExperimentNamespace,
                                                                experimentFlagName, "");
const FlagManager& FlagManager::getInstance() {
    return getMutableInstance();
}

FlagManager& FlagManager::getMutableInstance() {
    std::call_once(mOnce, [&] {
        LOG_ALWAYS_FATAL_IF(mInstance, "Instance already created");
        mInstance = std::make_unique<FlagManager>(ConstructorTag{});
    });

    return *mInstance;
}

template int32_t FlagManager::getValue<int32_t>(const std::string&, std::optional<int32_t>,
                                                int32_t) const;
template int64_t FlagManager::getValue<int64_t>(const std::string&, std::optional<int64_t>,
                                                int64_t) const;
template bool FlagManager::getValue<bool>(const std::string&, std::optional<bool>, bool) const;
template <typename T>
T FlagManager::getValue(const std::string& experimentFlagName, std::optional<T> systemPropertyOpt,
                        T defaultValue) const {
    // System property takes precedence over the experiment config server value.
    if (systemPropertyOpt.has_value()) {
        return *systemPropertyOpt;
void FlagManager::markBootCompleted() {
    mBootCompleted = true;
}
    std::string str = getServerConfigurableFlag(experimentFlagName);
    return str.empty() ? defaultValue : doParse<T>(str.c_str()).value_or(defaultValue);

void FlagManager::dump(std::string& result) const {
#define DUMP_FLAG(name) dumpFlag(result, #name, std::bind(&FlagManager::name, this));

    base::StringAppendF(&result, "FlagManager values: \n");
    DUMP_FLAG(use_adpf_cpu_hint);
    DUMP_FLAG(use_skia_tracing);

#undef DUMP_FLAG
}

int64_t FlagManager::demo_flag() const {
    std::optional<int64_t> sysPropVal = std::nullopt;
    return getValue("DemoFeature__demo_flag", sysPropVal, kDemoFlag);
std::optional<bool> FlagManager::getBoolProperty(const char* property) const {
    return parseBool(base::GetProperty(property, "").c_str());
}

bool FlagManager::use_adpf_cpu_hint() const {
    std::optional<bool> sysPropVal =
            doParse<bool>(base::GetProperty("debug.sf.enable_adpf_cpu_hint", "").c_str());
    return getValue("AdpfFeature__adpf_cpu_hint", sysPropVal, false);
bool FlagManager::getServerConfigurableFlag(const char* experimentFlagName) const {
    const auto value = server_configurable_flags::GetServerConfigurableFlag(kExperimentNamespace,
                                                                            experimentFlagName, "");
    const auto res = parseBool(value.c_str());
    return res.has_value() && res.value();
}

bool FlagManager::use_skia_tracing() const {
    std::optional<bool> sysPropVal =
            doParse<bool>(base::GetProperty(PROPERTY_SKIA_ATRACE_ENABLED, "").c_str());
    return getValue("SkiaTracingFeature__use_skia_tracing", sysPropVal, false);
#define FLAG_MANAGER_SERVER_FLAG(name, syspropOverride, serverFlagName)                     \
    bool FlagManager::name() const {                                                        \
        LOG_ALWAYS_FATAL_IF(!mBootCompleted,                                                \
                            "Can't read %s before boot completed as it is server writable", \
                            __func__);                                                      \
        const auto debugOverride = getBoolProperty(syspropOverride);                        \
        if (debugOverride.has_value()) return debugOverride.value();                        \
        return getServerConfigurableFlag(serverFlagName);                                   \
    }

FLAG_MANAGER_SERVER_FLAG(test_flag, "", "")
FLAG_MANAGER_SERVER_FLAG(use_adpf_cpu_hint, "debug.sf.enable_adpf_cpu_hint",
                         "AdpfFeature__adpf_cpu_hint")
FLAG_MANAGER_SERVER_FLAG(use_skia_tracing, PROPERTY_SKIA_ATRACE_ENABLED,
                         "SkiaTracingFeature__use_skia_tracing")

#undef FLAG_MANAGER_SERVER_FLAG

} // namespace android
+12 −12
Original line number Diff line number Diff line
@@ -30,30 +30,30 @@ private:
    struct ConstructorTag {};

public:
    static FlagManager& getInstance();
    static const FlagManager& getInstance();
    static FlagManager& getMutableInstance();

    FlagManager(ConstructorTag);

    virtual ~FlagManager();
    void dump(std::string& result) const;

    int64_t demo_flag() const;
    void markBootCompleted();
    void dump(std::string& result) const;

    bool test_flag() const;
    bool use_adpf_cpu_hint() const;

    bool use_skia_tracing() const;

protected:
    // overridden for unit tests
    virtual std::optional<bool> getBoolProperty(const char*) const;
    virtual bool getServerConfigurableFlag(const char*) const;

private:
    friend class FlagManagerTest;
    friend class TestableFlagManager;

    FlagManager(const FlagManager&) = delete;

    // Wrapper for mocking in test.
    virtual std::string getServerConfigurableFlag(const std::string& experimentFlagName) const;

    template <typename T>
    T getValue(const std::string& experimentFlagName, std::optional<T> systemPropertyOpt,
               T defaultValue) const;
    std::atomic_bool mBootCompleted;

    static std::unique_ptr<FlagManager> mInstance;
    static std::once_flag mOnce;
+1 −0
Original line number Diff line number Diff line
@@ -689,6 +689,7 @@ void SurfaceFlinger::bootFinished() {
        return;
    }
    mBootFinished = true;
    FlagManager::getMutableInstance().markBootCompleted();
    if (mStartPropertySetThread->join() != NO_ERROR) {
        ALOGE("Join StartPropertySetThread failed!");
    }
+35 −94
Original line number Diff line number Diff line
@@ -14,130 +14,71 @@
 * limitations under the License.
 */

#include <cstdint>
#undef LOG_TAG
#define LOG_TAG "FlagManagerTest"

#include "FlagManager.h"

#include <android-base/properties.h>
#include <gmock/gmock.h>
#include <gtest/gtest.h>
#include <log/log.h>
#include <server_configurable_flags/get_flags.h>
#include <optional>

namespace android {

using testing::Return;

class MockFlagManager : public FlagManager {
class TestableFlagManager : public FlagManager {
public:
    MockFlagManager() = default;
    ~MockFlagManager() = default;
    TestableFlagManager() : FlagManager(ConstructorTag{}) { markBootCompleted(); }
    ~TestableFlagManager() = default;

    MOCK_METHOD(std::string, getServerConfigurableFlag, (const std::string& experimentFlagName),
                (const, override));
    MOCK_METHOD(std::optional<bool>, getBoolProperty, (const char*), (const, override));
    MOCK_METHOD(bool, getServerConfigurableFlag, (const char*), (const, override));

    void markBootIncomplete() { mBootCompleted = false; }
};

class FlagManagerTest : public testing::Test {
public:
    FlagManagerTest();
    ~FlagManagerTest() override;
    std::unique_ptr<MockFlagManager> mFlagManager;

    template <typename T>
    T getValue(const std::string& experimentFlagName, std::optional<T> systemPropertyOpt,
               T defaultValue);
};

FlagManagerTest::FlagManagerTest() {
    FlagManagerTest() {
        const ::testing::TestInfo* const test_info =
                ::testing::UnitTest::GetInstance()->current_test_info();
        ALOGD("**** Setting up for %s.%s\n", test_info->test_case_name(), test_info->name());
    mFlagManager = std::make_unique<MockFlagManager>();
    }

FlagManagerTest::~FlagManagerTest() {
    ~FlagManagerTest() override {
        const ::testing::TestInfo* const test_info =
                ::testing::UnitTest::GetInstance()->current_test_info();
        ALOGD("**** Tearing down after %s.%s\n", test_info->test_case_name(), test_info->name());
    }

template <typename T>
T FlagManagerTest::getValue(const std::string& experimentFlagName,
                            std::optional<T> systemPropertyOpt, T defaultValue) {
    return mFlagManager->getValue(experimentFlagName, systemPropertyOpt, defaultValue);
}

namespace {
TEST_F(FlagManagerTest, getValue_bool_default) {
    EXPECT_CALL(*mFlagManager, getServerConfigurableFlag).Times(1).WillOnce(Return(""));
    const bool defaultValue = false;
    std::optional<bool> systemPropertyValue = std::nullopt;
    const bool result = FlagManagerTest::getValue("test_flag", systemPropertyValue, defaultValue);
    ASSERT_EQ(result, defaultValue);
}
    TestableFlagManager mFlagManager;
};

TEST_F(FlagManagerTest, getValue_bool_sysprop) {
    const bool defaultValue = false;
    std::optional<bool> systemPropertyValue = std::make_optional(true);
    const bool result = FlagManagerTest::getValue("test_flag", systemPropertyValue, defaultValue);
    ASSERT_EQ(result, true);
TEST_F(FlagManagerTest, isSingleton) {
    EXPECT_EQ(&FlagManager::getInstance(), &FlagManager::getInstance());
}

TEST_F(FlagManagerTest, getValue_bool_experiment) {
    EXPECT_CALL(*mFlagManager, getServerConfigurableFlag).Times(1).WillOnce(Return("1"));
    const bool defaultValue = false;
    std::optional<bool> systemPropertyValue = std::nullopt;
    const bool result = FlagManagerTest::getValue("test_flag", systemPropertyValue, defaultValue);
    ASSERT_EQ(result, true);
TEST_F(FlagManagerTest, creashesIfQueriedBeforeBoot) {
    mFlagManager.markBootIncomplete();
    EXPECT_DEATH(FlagManager::getInstance().use_adpf_cpu_hint(), "");
}

TEST_F(FlagManagerTest, getValue_int32_default) {
    EXPECT_CALL(*mFlagManager, getServerConfigurableFlag).Times(1).WillOnce(Return(""));
    int32_t defaultValue = 30;
    std::optional<int32_t> systemPropertyValue = std::nullopt;
    int32_t result = FlagManagerTest::getValue("test_flag", systemPropertyValue, defaultValue);
    ASSERT_EQ(result, defaultValue);
}
TEST_F(FlagManagerTest, returnsOverride) {
    EXPECT_CALL(mFlagManager, getBoolProperty).WillOnce(Return(true));
    EXPECT_EQ(true, mFlagManager.test_flag());

TEST_F(FlagManagerTest, getValue_int32_sysprop) {
    int32_t defaultValue = 30;
    std::optional<int32_t> systemPropertyValue = std::make_optional(10);
    int32_t result = FlagManagerTest::getValue("test_flag", systemPropertyValue, defaultValue);
    ASSERT_EQ(result, 10);
    EXPECT_CALL(mFlagManager, getBoolProperty).WillOnce(Return(false));
    EXPECT_EQ(false, mFlagManager.test_flag());
}

TEST_F(FlagManagerTest, getValue_int32_experiment) {
    EXPECT_CALL(*mFlagManager, getServerConfigurableFlag).Times(1).WillOnce(Return("50"));
    std::int32_t defaultValue = 30;
    std::optional<std::int32_t> systemPropertyValue = std::nullopt;
    std::int32_t result = FlagManagerTest::getValue("test_flag", systemPropertyValue, defaultValue);
    ASSERT_EQ(result, 50);
}
TEST_F(FlagManagerTest, returnsValue) {
    EXPECT_CALL(mFlagManager, getBoolProperty).WillRepeatedly(Return(std::nullopt));

TEST_F(FlagManagerTest, getValue_int64_default) {
    EXPECT_CALL(*mFlagManager, getServerConfigurableFlag).Times(1).WillOnce(Return(""));
    int64_t defaultValue = 30;
    std::optional<int64_t> systemPropertyValue = std::nullopt;
    int64_t result = getValue("flag_name", systemPropertyValue, defaultValue);
    ASSERT_EQ(result, defaultValue);
}
    EXPECT_CALL(mFlagManager, getServerConfigurableFlag).WillOnce(Return(true));
    EXPECT_EQ(true, mFlagManager.test_flag());

TEST_F(FlagManagerTest, getValue_int64_sysprop) {
    int64_t defaultValue = 30;
    std::optional<int64_t> systemPropertyValue = std::make_optional(10);
    int64_t result = getValue("flag_name", systemPropertyValue, defaultValue);
    ASSERT_EQ(result, 10);
    EXPECT_CALL(mFlagManager, getServerConfigurableFlag).WillOnce(Return(false));
    EXPECT_EQ(false, mFlagManager.test_flag());
}

TEST_F(FlagManagerTest, getValue_int64_experiment) {
    EXPECT_CALL(*mFlagManager, getServerConfigurableFlag).Times(1).WillOnce(Return("50"));
    int64_t defaultValue = 30;
    std::optional<int64_t> systemPropertyValue = std::nullopt;
    int64_t result = getValue("flag_name", systemPropertyValue, defaultValue);
    ASSERT_EQ(result, 50);
}
} // namespace
} // namespace android