Skip to content

GitLab

T
tangled
Commit 14fa1538 authored by liberato@chromium.org's avatar liberato@chromium.org Committed by Commit Bot
Browse files
Options

Switch to finch for setting media::Tuneable values. 

Change-Id: I151f491c0e3729cff3a20c4b8ec9fd628ce5898b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2343630
Auto-Submit: Frank Liberato <liberato@chromium.org>
Commit-Queue: Dale Curtis <dalecurtis@chromium.org>
Reviewed-by: default avatarDale Curtis <dalecurtis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#796466}
parent d1e82342
Hide whitespace changes
Inline Side-by-side
Showing with 18 additions and 142 deletions
media/base/tuneable.cc
View file @ 14fa1538
......@@ -15,85 +15,37 @@
namespace {
// Random seed for all tuneables.
static std::string* GetRandomSeedPtr() {
static base::NoDestructor<std::string> s_random_seed;
return &(*s_random_seed);
}
// Generate a pseudorandom number that depends only on the random seed provided
// to SetRandomSeedForTuneables and the name provided as an argument.
//
// We cast |T| to / from int for the non-specialized implementation, because the
// underlying parameters sent by finch are ints anyway. One can't really do
// too much better. Since specific types must be declared explicitly for the
// Tuneable specializations anyway (see the bottom of this file), there's no
// chance of somebody picking something we haven't thought of and getting an
// unexpected specialization of GenerateRandom.
template <typename T>
T GenerateRandom(const char* name, T minimum_value, T maximum_value) {
return static_cast<T>(GenerateRandom<int>(
name, static_cast<int>(minimum_value), static_cast<int>(maximum_value)));
}
template <>
int GenerateRandom<int>(const char* name,
int minimum_value,
int maximum_value) {
// It's okay if this isn't terribly random.
auto name_hash =
base::PersistentHash(std::string(name) + *GetRandomSeedPtr());
// Limit to the range [min, max].
// TODO(liberato): I think that this is biased.
return name_hash % (maximum_value - minimum_value + 1) + minimum_value;
}
template <>
base::TimeDelta GenerateRandom<base::TimeDelta>(const char* name,
base::TimeDelta minimum_value,
base::TimeDelta maximum_value) {
return base::TimeDelta::FromMilliseconds(GenerateRandom<int>(
name, minimum_value.InMilliseconds(), maximum_value.InMilliseconds()));
}
// Get the finch parameter `name`_`suffix`, and clamp it to the given values.
// Return `default_value` if there is no parameter, or if the experiment is off.
// Get the finch parameter `name`, and clamp it to the given values. Return
// `default_value` if there is no parameter, or if the experiment is off.
template <typename T>
T GetParam(const char* name,
const char* suffix,
T minimum_value,
T default_value,
T maximum_value) {
return static_cast<T>(GetParam<int>(
name, suffix, static_cast<int>(minimum_value),
static_cast<int>(default_value), static_cast<int>(maximum_value)));
return static_cast<T>(GetParam<int>(name, static_cast<int>(minimum_value),
static_cast<int>(default_value),
static_cast<int>(maximum_value)));
}
template <>
int GetParam<int>(const char* name,
const char* suffix,
int minimum_value,
int default_value,
int maximum_value) {
// TODO: "media_" # `name` ? Seems like a good idea, since finch params are
// not local to any finch feature. For now, we let consumers do this.
std::string param_name = std::string(name) + std::string(suffix);
return base::ClampToRange(
base::FeatureParam<int>(&::media::kMediaOptimizer, param_name.c_str(),
default_value)
base::FeatureParam<int>(&::media::kMediaOptimizer, name, default_value)
.Get(),
minimum_value, maximum_value);
}
template <>
base::TimeDelta GetParam<base::TimeDelta>(const char* name,
const char* suffix,
base::TimeDelta minimum_value,
base::TimeDelta default_value,
base::TimeDelta maximum_value) {
return base::TimeDelta::FromMilliseconds(GetParam<int>(
name, suffix, minimum_value.InMilliseconds(),
default_value.InMilliseconds(), maximum_value.InMilliseconds()));
name, minimum_value.InMilliseconds(), default_value.InMilliseconds(),
maximum_value.InMilliseconds()));
}
} // namespace
......@@ -105,32 +57,14 @@ Tuneable<T>::Tuneable(const char* name,
T minimum_value,
T default_value,
T maximum_value) {
// Fetch the finch-provided range, clamped to the min, max and defaulted to
// the hardcoded default. This way, if it's unset, min == max == default.
T finch_minimum =
GetParam<T>(name, "_min", minimum_value, default_value, maximum_value);
T finch_maximum =
GetParam<T>(name, "_max", minimum_value, default_value, maximum_value);
if (finch_minimum > finch_maximum) {
// Bad parameters. They're all in range, so we could pick any, but we
// assume that the finch range has no meaning and just use the (hopefully)
// saner default.
t_ = default_value;
return;
}
t_ = GenerateRandom<T>(name, finch_minimum, finch_maximum);
// Fetch the finch-provided value, clamped to the min, max and defaulted to
// the hardcoded default if it's unset.
t_ = GetParam<T>(name, minimum_value, default_value, maximum_value);
}
template <typename T>
Tuneable<T>::~Tuneable() = default;
void MEDIA_EXPORT
SetRandomSeedForTuneables(const base::UnguessableToken& seed) {
*GetRandomSeedPtr() = seed.ToString();
}
// All allowed Tuneable types. Be sure that GenerateRandom() and GetParam()
// do something sane for any type you add.
template class MEDIA_EXPORT Tuneable<int>;
......
media/base/tuneable_unittest.cc
View file @ 14fa1538
......@@ -27,31 +27,20 @@ class TuneableTest : public ::testing::Test {
// params[kTuneableInt10] = "10";
// Set some tuneables to fixed values.
SetFinchParameters(kTuneableIntSetToNot123, 124, 124);
SetFinchParameters(kTuneableInt0, 0, 0);
SetFinchParameters(kTuneableInt5, 5, 5);
SetFinchParameters(kTuneableInt10, 10, 10);
SetFinchParameters(kTuneableIntSetToNot123, 124);
SetFinchParameters(kTuneableInt0, 0);
SetFinchParameters(kTuneableInt5, 5);
SetFinchParameters(kTuneableInt10, 100);
// TimeDelta should be given in milliseconds.
SetFinchParameters(kTuneableTimeDeltaFiveSeconds, 5000, 5000);
// Let some vary via finch trial.
SetFinchParameters(kTuneableInt5To10, 5, 10);
// Create 100 identical tuneables, except for their names.
for (int i = 0; i < 100; i++) {
SetFinchParameters(GetNameForNumberedTuneable(k100Tuneables, i).c_str(),
1, 100);
}
SetFinchParameters(kTuneableTimeDeltaFiveSeconds, 5000);
scoped_feature_list_.InitAndEnableFeatureWithParameters(kMediaOptimizer,
params_);
}
// Set the finch-chosen parameters for tuneable `name`.
void SetFinchParameters(const char* name, int min_value, int max_value) {
std::string min_name = std::string(name) + "_min";
params_[min_name] = base::NumberToString(min_value);
std::string max_name = std::string(name) + "_max";
params_[max_name] = base::NumberToString(max_value);
void SetFinchParameters(const char* name, int value) {
params_[name] = base::NumberToString(value);
}
// Return the tuneable name for the `x`-th numbered tuneable.
......@@ -76,11 +65,6 @@ class TuneableTest : public ::testing::Test {
static constexpr const char* kTuneableInt10 = "t_int_10";
static constexpr const char* kTuneableTimeDeltaFiveSeconds = "t_time_5s";
static constexpr const char* kTuneableInt5To10 = "t_int_5to10";
// Initialize 100 of these with different names.
static constexpr const char* k100Tuneables = "XX_tuneable";
DISALLOW_COPY_AND_ASSIGN(TuneableTest);
};
......@@ -145,46 +129,4 @@ TEST_F(TuneableTest, TimeDeltaIsSpecifiedInMilliseconds) {
EXPECT_EQ(t.value(), base::TimeDelta::FromSeconds(5));
}
TEST_F(TuneableTest, MultipleTuneablesGetTheSameRandomValue) {
// Multiple copies of the same tuneable should get the same value.
Tuneable<int> t0(kTuneableInt5To10, 0, 2, 100);
Tuneable<int> t1(kTuneableInt5To10, 0, 2, 100);
EXPECT_GE(t0.value(), 0);
EXPECT_LE(t0.value(), 100);
EXPECT_EQ(t0.value(), t1.value());
}
TEST_F(TuneableTest, DifferentSeedsProduceDifferentValues) {
// Also verify that they stay bounded.
SetRandomSeedForTuneables(base::UnguessableToken::Create());
Tuneable<int> t0(kTuneableInt5To10, 0, 2, 100);
bool found_different = false;
for (int i = 1; i < 100; i++) {
SetRandomSeedForTuneables(base::UnguessableToken::Create());
Tuneable<int> t1(kTuneableInt5To10, 0, 2, 100);
EXPECT_GE(t1.value(), 0);
EXPECT_LE(t1.value(), 100);
if (t1.value() != t0.value())
found_different = true;
}
EXPECT_TRUE(found_different);
}
TEST_F(TuneableTest, DifferentNamesProduceDifferentValues) {
// For the same seed, we expect different parameter names to sometimes get
// different values.
Tuneable<int> t0(GetNameForNumberedTuneable(k100Tuneables, 0).c_str(), 0, 50,
100);
bool found_different = false;
for (int i = 1; i < 100; i++) {
Tuneable<int> t1(GetNameForNumberedTuneable(k100Tuneables, i).c_str(), 0,
50, 100);
EXPECT_GE(t1.value(), 0);
EXPECT_LE(t1.value(), 100);
if (t1.value() != t0.value())
found_different = true;
}
EXPECT_TRUE(found_different);
}
} // namespace media
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment