Skip to content

GitLab

T
tangled
Commit 9007c3fb authored by mpearson's avatar mpearson Committed by Commit bot
Browse files
Options

Omnibox - Refactor |relevance_buckets| to Remove Memory Leak on Exit 

A static class variable was new-ed with no opportunity for a delete call.
Instead, convert this to using CR_DEFINE_STATIC_LOCAL so it's properly
deleted on exit.

Removes ExperimentalScoringEnabled() field trial function and
changed the default value for the topicality threshold when
the field trial setting is erroneous or partially-specified.
I checked that removing this logic change does not affect any currently- or
recently-running experiments.

Other than changing the semantics of the field trial configs,
there should be no functional changes.

In the process, remove most references to HQP in variable names and
comments.
These references are unnecessary.
All scoring in these files are for the HQP.

BUG=

Review-Url: https://codereview.chromium.org/2548363010
Cr-Commit-Position: refs/heads/master@{#438006}
parent ada812f0
Hide whitespace changes
Inline Side-by-side
Showing with 113 additions and 172 deletions
components/omnibox/browser/omnibox_field_trial.cc
View file @ 9007c3fb
......@@ -371,33 +371,21 @@ void OmniboxFieldTrial::GetSuggestPollingStrategy(bool* from_last_keystroke,
}
}
bool OmniboxFieldTrial::HQPExperimentalScoringEnabled() {
return variations::GetVariationParamValue(
kBundledExperimentFieldTrialName,
kHQPExperimentalScoringEnabledParam) == "true";
}
std::string OmniboxFieldTrial::HQPExperimentalScoringBuckets() {
if (!HQPExperimentalScoringEnabled())
return "";
return variations::GetVariationParamValue(
kBundledExperimentFieldTrialName,
kHQPExperimentalScoringBucketsParam);
}
float OmniboxFieldTrial::HQPExperimentalTopicalityThreshold() {
if (!HQPExperimentalScoringEnabled())
return -1;
std::string topicality_threhold_str =
variations::GetVariationParamValue(
kBundledExperimentFieldTrialName,
kHQPExperimentalScoringTopicalityThresholdParam);
std::string topicality_threshold_str = variations::GetVariationParamValue(
kBundledExperimentFieldTrialName,
kHQPExperimentalScoringTopicalityThresholdParam);
double topicality_threshold;
if (!base::StringToDouble(topicality_threhold_str, &topicality_threshold))
return -1;
if (topicality_threshold_str.empty() ||
!base::StringToDouble(topicality_threshold_str, &topicality_threshold))
return 0.8f;
return static_cast<float>(topicality_threshold);
}
......@@ -580,8 +568,6 @@ const char OmniboxFieldTrial::kHUPNewScoringVisitedCountScoreBucketsParam[] =
const char OmniboxFieldTrial::kHUPNewScoringVisitedCountUseDecayFactorParam[] =
"VisitedCountUseDecayFactor";
const char OmniboxFieldTrial::kHQPExperimentalScoringEnabledParam[] =
"HQPExperimentalScoringEnabled";
const char OmniboxFieldTrial::kHQPExperimentalScoringBucketsParam[] =
"HQPExperimentalScoringBuckets";
const char
......
components/omnibox/browser/omnibox_field_trial.h
View file @ 9007c3fb
......@@ -270,22 +270,15 @@ class OmniboxFieldTrial {
// For HQP scoring related experiments to control the topicality and scoring
// ranges of relevancy scores.
// Returns true if HQP experimental scoring is enabled. Returns false if
// |kHQPExperimentalScoringEnabledParam| is not specified in the field trial.
static bool HQPExperimentalScoringEnabled();
// Returns the scoring buckets for HQP experiments. Returns empty string
// in case |kHQPExperimentalScoringBucketsParam| or
// |kHQPExperimentalScoringEnabledParam| is not specified in the
// field trial. Scoring buckets are stored in string form giving mapping from
// (topicality_score, frequency_score) to final relevance score.
// Please see GetRelevancyScore() under
// chrome/browser/history::ScoredHistoryMatch for details.
// Returns the scoring buckets for HQP experiments. Returns an empty string
// if scoring buckets are not specified in the field trial. Scoring buckets
// are stored in string form giving mapping from (topicality_score,
// frequency_score) to final relevance score. Please see GetRelevancyScore()
// under chrome/browser/history::ScoredHistoryMatch for details.
static std::string HQPExperimentalScoringBuckets();
// Returns the topicality threshold for HQP experiments. Returns -1 if
// |kHQPExperimentalScoringTopicalityThresholdParam| or
// |kHQPExperimentalScoringEnabledParam| is not specified in the field trial.
// Returns the topicality threshold for HQP experiments. Returns a default
// value of 0.8 if no threshold is specified in the field trial.
static float HQPExperimentalTopicalityThreshold();
// ---------------------------------------------------------
......@@ -411,7 +404,6 @@ class OmniboxFieldTrial {
static const char kHUPNewScoringVisitedCountUseDecayFactorParam[];
// Parameter names used by the HQP experimental scoring experiments.
static const char kHQPExperimentalScoringEnabledParam[];
static const char kHQPExperimentalScoringBucketsParam[];
static const char kHQPExperimentalScoringTopicalityThresholdParam[];
......
components/omnibox/browser/scored_history_match.cc
View file @ 9007c3fb
......@@ -10,6 +10,7 @@
#include <vector>
#include "base/logging.h"
#include "base/macros.h"
#include "base/numerics/safe_conversions.h"
#include "base/strings/string_number_conversions.h"
#include "base/strings/string_split.h"
......@@ -113,17 +114,9 @@ size_t ScoredHistoryMatch::max_visits_to_score_;
bool ScoredHistoryMatch::allow_tld_matches_;
bool ScoredHistoryMatch::allow_scheme_matches_;
size_t ScoredHistoryMatch::num_title_words_to_allow_;
bool ScoredHistoryMatch::hqp_experimental_scoring_enabled_;
// Default topicality threshold. See GetTopicalityScore() for details.
float ScoredHistoryMatch::topicality_threshold_ = 0.8f;
// Default HQP relevance buckets. See GetFinalRelevancyScore() for more details
// on these numbers.
char ScoredHistoryMatch::hqp_relevance_buckets_str_[] =
"0.0:400,1.5:600,5.0:900,10.5:1203,15.0:1300,20.0:1399";
std::vector<ScoredHistoryMatch::ScoreMaxRelevance>*
ScoredHistoryMatch::hqp_relevance_buckets_ = nullptr;
float ScoredHistoryMatch::topicality_threshold_;
ScoredHistoryMatch::ScoreMaxRelevances*
ScoredHistoryMatch::relevance_buckets_override_ = nullptr;
ScoredHistoryMatch::ScoredHistoryMatch()
: ScoredHistoryMatch(history::URLRow(),
......@@ -261,8 +254,8 @@ ScoredHistoryMatch::ScoredHistoryMatch(
const float topicality_score = GetTopicalityScore(
terms_vector.size(), url, terms_to_word_starts_offsets, word_starts);
const float frequency_score = GetFrequency(now, is_url_bookmarked, visits);
raw_score = base::saturated_cast<int>(GetFinalRelevancyScore(
topicality_score, frequency_score, *hqp_relevance_buckets_));
raw_score = base::saturated_cast<int>(
GetFinalRelevancyScore(topicality_score, frequency_score));
if (also_do_hup_like_scoring_ && likely_can_inline) {
// HistoryURL-provider-like scoring gives any match that is
......@@ -415,10 +408,11 @@ void ScoredHistoryMatch::Init() {
allow_tld_matches_ = OmniboxFieldTrial::HQPAllowMatchInTLDValue();
allow_scheme_matches_ = OmniboxFieldTrial::HQPAllowMatchInSchemeValue();
num_title_words_to_allow_ = OmniboxFieldTrial::HQPNumTitleWordsToAllow();
topicality_threshold_ =
OmniboxFieldTrial::HQPExperimentalTopicalityThreshold();
InitRawTermScoreToTopicalityScoreArray();
InitDaysAgoToRecencyScoreArray();
InitHQPExperimentalParams();
}
float ScoredHistoryMatch::GetTopicalityScore(
......@@ -617,12 +611,17 @@ float ScoredHistoryMatch::GetFrequency(const base::Time& now,
}
// static
float ScoredHistoryMatch::GetFinalRelevancyScore(
float topicality_score,
float frequency_score,
const std::vector<ScoreMaxRelevance>& hqp_relevance_buckets) {
DCHECK(hqp_relevance_buckets.size() > 0);
DCHECK_EQ(hqp_relevance_buckets[0].first, 0.0);
float ScoredHistoryMatch::GetFinalRelevancyScore(float topicality_score,
float frequency_score) {
// |relevance_buckets| gives a mapping from intemerdiate score to the final
// relevance score.
CR_DEFINE_STATIC_LOCAL(ScoreMaxRelevances, default_relevance_buckets,
(GetHQPBuckets()));
ScoreMaxRelevances* relevance_buckets = relevance_buckets_override_
? relevance_buckets_override_
: &default_relevance_buckets;
DCHECK(!relevance_buckets->empty());
DCHECK_EQ(0.0, (*relevance_buckets)[0].first);
if (topicality_score == 0)
return 0;
......@@ -642,7 +641,7 @@ float ScoredHistoryMatch::GetFinalRelevancyScore(
//
// The below code maps intermediate_score to the range [0, 1399].
// For example:
// HQP default scoring buckets: "0.0:400,1.5:600,12.0:1300,20.0:1399"
// The default scoring buckets: "0.0:400,1.5:600,12.0:1300,20.0:1399"
// We will linearly interpolate the scores between:
// 0 to 1.5 --> 400 to 600
// 1.5 to 12.0 --> 600 to 1300
......@@ -655,76 +654,52 @@ float ScoredHistoryMatch::GetFinalRelevancyScore(
// Find the threshold where intermediate score is greater than bucket.
size_t i = 1;
for (; i < hqp_relevance_buckets.size(); ++i) {
const ScoreMaxRelevance& hqp_bucket = hqp_relevance_buckets[i];
if (intermediate_score >= hqp_bucket.first) {
for (; i < relevance_buckets->size(); ++i) {
const ScoreMaxRelevance& bucket = (*relevance_buckets)[i];
if (intermediate_score >= bucket.first) {
continue;
}
const ScoreMaxRelevance& previous_bucket = hqp_relevance_buckets[i - 1];
const float slope = ((hqp_bucket.second - previous_bucket.second) /
(hqp_bucket.first - previous_bucket.first));
const ScoreMaxRelevance& previous_bucket = (*relevance_buckets)[i - 1];
const float slope = ((bucket.second - previous_bucket.second) /
(bucket.first - previous_bucket.first));
return (previous_bucket.second +
(slope * (intermediate_score - previous_bucket.first)));
}
// It will reach this stage when the score is > highest bucket score.
// Return the highest bucket score.
return hqp_relevance_buckets[i - 1].second;
return (*relevance_buckets)[i - 1].second;
}
// static
void ScoredHistoryMatch::InitHQPExperimentalParams() {
// These are default HQP relevance scoring buckets.
// See GetFinalRelevancyScore() for details.
std::string hqp_relevance_buckets_str = std::string(
hqp_relevance_buckets_str_);
// Fetch the experiment params if they are any.
hqp_experimental_scoring_enabled_ =
OmniboxFieldTrial::HQPExperimentalScoringEnabled();
if (hqp_experimental_scoring_enabled_) {
// Add the topicality threshold from experiment params.
float hqp_experimental_topicality_threhold =
OmniboxFieldTrial::HQPExperimentalTopicalityThreshold();
topicality_threshold_ = hqp_experimental_topicality_threhold;
// Add the HQP experimental scoring buckets.
std::string hqp_experimental_scoring_buckets =
OmniboxFieldTrial::HQPExperimentalScoringBuckets();
if (!hqp_experimental_scoring_buckets.empty())
hqp_relevance_buckets_str = hqp_experimental_scoring_buckets;
}
// Parse the hqp_relevance_buckets_str string once and store them in vector
// which is easy to access.
hqp_relevance_buckets_ =
new std::vector<ScoredHistoryMatch::ScoreMaxRelevance>();
bool is_valid_bucket_str = GetHQPBucketsFromString(hqp_relevance_buckets_str,
hqp_relevance_buckets_);
DCHECK(is_valid_bucket_str);
std::vector<ScoredHistoryMatch::ScoreMaxRelevance>
ScoredHistoryMatch::GetHQPBuckets() {
// Start with the default buckets and override them if appropriate.
std::string relevance_buckets_str =
"0.0:400,1.5:600,5.0:900,10.5:1203,15.0:1300,20.0:1399";
std::string experimental_scoring_buckets =
OmniboxFieldTrial::HQPExperimentalScoringBuckets();
if (!experimental_scoring_buckets.empty())
relevance_buckets_str = experimental_scoring_buckets;
return GetHQPBucketsFromString(relevance_buckets_str);
}
// static
bool ScoredHistoryMatch::GetHQPBucketsFromString(
const std::string& buckets_str,
std::vector<ScoreMaxRelevance>* hqp_buckets) {
DCHECK(hqp_buckets != NULL);
ScoredHistoryMatch::ScoreMaxRelevances
ScoredHistoryMatch::GetHQPBucketsFromString(const std::string& buckets_str) {
DCHECK(!buckets_str.empty());
base::StringPairs kv_pairs;
if (base::SplitStringIntoKeyValuePairs(buckets_str, ':', ',', &kv_pairs)) {
for (base::StringPairs::const_iterator it = kv_pairs.begin();
it != kv_pairs.end(); ++it) {
ScoreMaxRelevance bucket;
bool is_valid_intermediate_score =
base::StringToDouble(it->first, &bucket.first);
DCHECK(is_valid_intermediate_score);
bool is_valid_hqp_score = base::StringToInt(it->second, &bucket.second);
DCHECK(is_valid_hqp_score);
hqp_buckets->push_back(bucket);
}
return true;
if (!base::SplitStringIntoKeyValuePairs(buckets_str, ':', ',', &kv_pairs))
return ScoreMaxRelevances();
ScoreMaxRelevances hqp_buckets;
for (base::StringPairs::const_iterator it = kv_pairs.begin();
it != kv_pairs.end(); ++it) {
ScoreMaxRelevance bucket;
bool is_valid_intermediate_score =
base::StringToDouble(it->first, &bucket.first);
DCHECK(is_valid_intermediate_score);
bool is_valid_hqp_score = base::StringToInt(it->second, &bucket.second);
DCHECK(is_valid_hqp_score);
hqp_buckets.push_back(bucket);
}
return false;
return hqp_buckets;
}
components/omnibox/browser/scored_history_match.h
View file @ 9007c3fb
......@@ -24,9 +24,14 @@ class ScoredHistoryMatchTest;
struct ScoredHistoryMatch : public history::HistoryMatch {
// ScoreMaxRelevance maps from an intermediate-score to the maximum
// final-relevance score given to a URL for this intermediate score.
// This is used to store the score ranges of HQP relevance buckets.
// This is used to store the score ranges of relevance buckets.
// Please see GetFinalRelevancyScore() for details.
typedef std::pair<double, int> ScoreMaxRelevance;
using ScoreMaxRelevance = std::pair<double, int>;
// A sorted vector of ScoreMaxRelevance entries, used by taking a score and
// interpolating between consecutive buckets. See GetFinalRelevancyScore()
// for details.
using ScoreMaxRelevances = std::vector<ScoreMaxRelevance>;
// Required for STL, we don't use this directly.
ScoredHistoryMatch();
......@@ -128,29 +133,22 @@ struct ScoredHistoryMatch : public history::HistoryMatch {
const bool bookmarked,
const VisitInfoVector& visits) const;
// Combines the two component scores into a final score that's
// an appropriate value to use as a relevancy score. Scoring buckets are
// specified through |hqp_relevance_buckets|. Please see the function
// implementation for more details.
static float GetFinalRelevancyScore(
float topicality_score,
float frequency_score,
const std::vector<ScoreMaxRelevance>& hqp_relevance_buckets);
// Initializes the HQP experimental params: |hqp_relevance_buckets_|
// to default buckets. If hqp experimental scoring is enabled, it
// fetches the |hqp_experimental_scoring_enabled_|, |topicality_threshold_|
// and |hqp_relevance_buckets_| from omnibox field trials.
static void InitHQPExperimentalParams();
// Helper function to parse the string containing the scoring buckets.
// For example,
// String: "0.0:400,1.5:600,12.0:1300,20.0:1399"
// Buckets: vector[(0.0, 400),(1.5,600),(12.0,1300),(20.0,1399)]
// Returns false, in case if it fail to parse the string.
static bool GetHQPBucketsFromString(
const std::string& buckets_str,
std::vector<ScoreMaxRelevance>* hqp_buckets);
// Combines the two component scores into a final score that's an appropriate
// value to use as a relevancy score.
static float GetFinalRelevancyScore(float topicality_score,
float frequency_score);
// Helper function that returns the string containing the scoring buckets
// (either the default ones or ones specified in an experiment).
static ScoreMaxRelevances GetHQPBuckets();
// Helper function to parse the string containing the scoring buckets and
// return the results. For example, with |buckets_str| as
// "0.0:400,1.5:600,12.0:1300,20.0:1399", it returns [(0.0, 400), (1.5, 600),
// (12.0, 1300), (20.0, 1399)]. It returns an empty vector in the case of a
// malformed |buckets_str|.
static ScoreMaxRelevances GetHQPBucketsFromString(
const std::string& buckets_str);
// If true, assign raw scores to be max(whatever it normally would be, a
// score that's similar to the score HistoryURL provider would assign).
......@@ -184,22 +182,15 @@ struct ScoredHistoryMatch : public history::HistoryMatch {
// Words beyond this number are ignored.
static size_t num_title_words_to_allow_;
// True, if hqp experimental scoring is enabled.
static bool hqp_experimental_scoring_enabled_;
// |topicality_threshold_| is used to control the topicality scoring.
// If |topicality_threshold_| > 0, then URLs with topicality-score < threshold
// are given topicality score of 0. By default it is initalized to -1.
// If |topicality_threshold_| > 0, then URLs with topicality-score less than
// the threshold are given topicality score of 0.
static float topicality_threshold_;
// |hqp_relevance_buckets_str_| is used to control the hqp score ranges.
// It is the string representation of |hqp_relevance_buckets_|.
static char hqp_relevance_buckets_str_[];
// |hqp_relevance_buckets_| gives mapping from (topicality*frequency)
// to the final relevance scoring. Please see GetFinalRelevancyScore()
// for more details and scoring method.
static std::vector<ScoreMaxRelevance>* hqp_relevance_buckets_;
// Used for testing. A possibly null pointer to a vector. If set,
// overrides the static local variable |relevance_buckets| declared in
// GetFinalRelevancyScore().
static ScoreMaxRelevances* relevance_buckets_override_;
};
typedef std::vector<ScoredHistoryMatch> ScoredHistoryMatches;
......
components/omnibox/browser/scored_history_match_unittest.cc
View file @ 9007c3fb
......@@ -647,54 +647,51 @@ TEST_F(ScoredHistoryMatchTest, GetTopicalityScore) {
// Test the function GetFinalRelevancyScore().
TEST_F(ScoredHistoryMatchTest, GetFinalRelevancyScore) {
// hqp_relevance_buckets = "0.0:100,1.0:200,4.0:500,8.0:900,10.0:1000";
std::vector<ScoredHistoryMatch::ScoreMaxRelevance> hqp_buckets;
hqp_buckets.push_back(std::make_pair(0.0, 100));
hqp_buckets.push_back(std::make_pair(1.0, 200));
hqp_buckets.push_back(std::make_pair(4.0, 500));
hqp_buckets.push_back(std::make_pair(8.0, 900));
hqp_buckets.push_back(std::make_pair(10.0, 1000));
// relevance_buckets = "0.0:100,1.0:200,4.0:500,8.0:900,10.0:1000";
ScoredHistoryMatch::ScoreMaxRelevances relevance_buckets = {
{0.0, 100}, {1.0, 200}, {4.0, 500}, {8.0, 900}, {10.0, 1000}};
base::AutoReset<ScoredHistoryMatch::ScoreMaxRelevances*> tmp(
&ScoredHistoryMatch::relevance_buckets_override_, &relevance_buckets);
// Check when topicality score is zero.
float topicality_score = 0.0;
float frequency_score = 10.0;
// intermediate_score = 0.0 * 10.0 = 0.0.
EXPECT_EQ(0, ScoredHistoryMatch::GetFinalRelevancyScore(
topicality_score, frequency_score, hqp_buckets));
EXPECT_EQ(0, ScoredHistoryMatch::GetFinalRelevancyScore(topicality_score,
frequency_score));
// Check when intermediate score falls at the border range.
topicality_score = 0.4f;
frequency_score = 10.0f;
// intermediate_score = 0.5 * 10.0 = 4.0.
EXPECT_EQ(500, ScoredHistoryMatch::GetFinalRelevancyScore(
topicality_score, frequency_score, hqp_buckets));
EXPECT_EQ(500, ScoredHistoryMatch::GetFinalRelevancyScore(topicality_score,
frequency_score));
// Checking the score that falls into one of the buckets.
topicality_score = 0.5f;
frequency_score = 10.0f;
// intermediate_score = 0.5 * 10.0 = 5.0.
EXPECT_EQ(600, // 500 + (((900 - 500)/(8 -4)) * 1) = 600.
ScoredHistoryMatch::GetFinalRelevancyScore(
topicality_score, frequency_score, hqp_buckets));
ScoredHistoryMatch::GetFinalRelevancyScore(topicality_score,
frequency_score));
// Never give the score greater than maximum specified.
topicality_score = 0.5f;
frequency_score = 22.0f;
// intermediate_score = 0.5 * 22.0 = 11.0
EXPECT_EQ(1000, ScoredHistoryMatch::GetFinalRelevancyScore(
topicality_score, frequency_score, hqp_buckets));
EXPECT_EQ(1000, ScoredHistoryMatch::GetFinalRelevancyScore(topicality_score,
frequency_score));
}
// Test the function GetHQPBucketsFromString().
TEST_F(ScoredHistoryMatchTest, GetHQPBucketsFromString) {
std::string buckets_str = "0.0:400,1.5:600,12.0:1300,20.0:1399";
std::vector<ScoredHistoryMatch::ScoreMaxRelevance> hqp_buckets;
EXPECT_TRUE(
ScoredHistoryMatch::GetHQPBucketsFromString(buckets_str, &hqp_buckets));
std::vector<ScoredHistoryMatch::ScoreMaxRelevance> hqp_buckets =
ScoredHistoryMatch::GetHQPBucketsFromString(buckets_str);
EXPECT_THAT(hqp_buckets, ElementsAre(Pair(0.0, 400), Pair(1.5, 600),
Pair(12.0, 1300), Pair(20.0, 1399)));
// invalid string.
// Test using an invalid string.
buckets_str = "0.0,400,1.5,600";
EXPECT_FALSE(
ScoredHistoryMatch::GetHQPBucketsFromString(buckets_str, &hqp_buckets));
hqp_buckets = ScoredHistoryMatch::GetHQPBucketsFromString(buckets_str);
EXPECT_TRUE(hqp_buckets.empty());
}
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