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

components/omnibox/browser/scored_history_match.cc

Index: components/omnibox/browser/scored_history_match.cc
diff --git a/components/omnibox/browser/scored_history_match.cc b/components/omnibox/browser/scored_history_match.cc
index bdde8e547430e18c6a9659cc48203cde566edf58..56e4408c077b263911ffe276ffb95258780197dd 100644
--- a/components/omnibox/browser/scored_history_match.cc
+++ b/components/omnibox/browser/scored_history_match.cc
@@ -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"
@@ -30,6 +31,14 @@ const int kDaysToPrecomputeRecencyScoresFor = 366;
 // capped at the score of the largest bucket.
 const int kMaxRawTermScore = 30;
 
+// Default topicality threshold.  See GetTopicalityScore() for details.
+float kDefaultTopicalityThreshold = 0.8;
+
+// Default relevance buckets. See GetFinalRelevancyScore() for more details on
+// these numbers.
+const char kDefaultRelevanceBucketsString[] =
+    "0.0:400,1.5:600,5.0:900,10.5:1203,15.0:1300,20.0:1399";

[Peter Kasting, 2016/12/08 01:29:21]

Nit: Since both these constants are only used in one function, I suggest moving
them there, so their definitions appear adjacent to their uses.

[Mark P, 2016/12/09 20:40:56]

I prefer meaningful, significant constants be declared in an obvious place, not
hidden in an implementation somewhere.

Also, they better match the style of the existing file by going here with the
existing two constants above.  (And those cannot be moved because they're used
as array size bounds immediately below.)

[Peter Kasting, 2016/12/10 02:31:15]

You mean, like, up here?  :)

It seems harder to read code when the constants it uses aren't locally defined,
but hidden away in a random other place in the file.

The Google style guide (
http://google.github.io/styleguide/cppguide.html#Local_Variables ) says: "Place
a function's variables in the narrowest scope possible".  I read that as
applying to these variables too.

Is this something some external consumer would be looking to read?  Would they
tend to look here rather than in GetHQPBuckets(), which seems to be the API that
actually relates to this?
As you note, these are up here because they're used here and cannot be moved. 
But I'm not convinced that establishes a "file style" that all constants are
declared up here.

Anyway, I'm not going to put my foot down and demand this, but I think the style
guide text is applicable.  :)

[Mark P, 2016/12/11 04:57:22]

Touché.
 
Seems like a reasonable interpretation.
Okay, I'm convinced.  Done.

+
 // Pre-computed information to speed up calculating recency scores.
 // |days_ago_to_recency_score| is a simple array mapping how long ago a page was
 // visited (in days) to the recency score we should assign it.  This allows easy
@@ -113,17 +122,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";
+float ScoredHistoryMatch::topicality_threshold_;
 std::vector<ScoredHistoryMatch::ScoreMaxRelevance>*
-    ScoredHistoryMatch::hqp_relevance_buckets_ = nullptr;
+    ScoredHistoryMatch::relevance_buckets_ = nullptr;
 
 ScoredHistoryMatch::ScoredHistoryMatch()
     : ScoredHistoryMatch(history::URLRow(),
@@ -261,8 +262,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 +416,13 @@ void ScoredHistoryMatch::Init() {
   allow_tld_matches_ = OmniboxFieldTrial::HQPAllowMatchInTLDValue();
   allow_scheme_matches_ = OmniboxFieldTrial::HQPAllowMatchInSchemeValue();
   num_title_words_to_allow_ = OmniboxFieldTrial::HQPNumTitleWordsToAllow();
+  topicality_threshold_ =
+      OmniboxFieldTrial::HQPExperimentalScoringEnabled()
+          ? OmniboxFieldTrial::HQPExperimentalTopicalityThreshold()
+          : kDefaultTopicalityThreshold;

[Peter Kasting, 2016/12/08 01:29:21]

Nit: Should HQPExperimentalTopicalityThreshold() return the "default threshold"
when HQPExperimentalScoringEnabled() would return false?  That would obviate the
need to define the threshold here (for now) and seems consistent with the other
values above?

[Mark P, 2016/12/09 20:40:56]

Sure.  I had some concerns about current experiment configs that this change in
interpretation would screw up, but I checked them out and they all look fine. 
Done.

 
   InitRawTermScoreToTopicalityScoreArray();
   InitDaysAgoToRecencyScoreArray();
-  InitHQPExperimentalParams();
 }
 
 float ScoredHistoryMatch::GetTopicalityScore(
@@ -617,12 +621,15 @@ 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(std::vector<ScoreMaxRelevance>, relevance_buckets,
+                         (GetHQPBuckets()));

[Peter Kasting, 2016/12/08 01:29:21]

This has the danger (that you called out in the header comments) that a test
might want to access the pointer before this method is first called, which won't
work given the way you're having tests dereference this.

I would fix this by making |relevance_buckets_| a "possibly null pointer to an
override vector" instead, like this:

// in .h
static std::vector<ScoreMaxRelevance>* relevance_buckets_override_;

// here
CR_DEFINE_STATIC_LOCAL(std::vector<ScoreMaxRelevance>,
                       default_relevance_buckets, (GetHQPBuckets()));
std::vector<ScoreMaxRelevance>* relevance_buckets = relevance_buckets_override_
    ? relevance_buckets_override_ : &default_relevance_buckets;
// Use *relevance_buckets below this point

// in tests
std::vector<ScoreMaxRelevance> relevance_buckets = ...;
base::AutoReset<std::vector<ScoreMaxRelevance>*> tmp(
    &obj.relevance_buckets_override_, &relevance_buckets);

Now the test can change the pointer value even before the first call to this
method, and everything will still work fine.

[Mark P, 2016/12/09 20:40:56]

Yeah, that's cleaner.  Done.

+  relevance_buckets_ = &relevance_buckets;
+  DCHECK(relevance_buckets.size() > 0);

[Peter Kasting, 2016/12/08 01:29:21]

Nit: DCHECK(!relevance_buckets.empty())

[Mark P, 2016/12/09 20:40:56]

Done.

+  DCHECK_EQ(relevance_buckets[0].first, 0.0);

[Peter Kasting, 2016/12/08 01:29:21]

Nit: (expected, actual)

[Mark P, 2016/12/09 20:40:56]

Done.

 
   if (topicality_score == 0)
     return 0;
@@ -642,7 +649,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 +662,53 @@ 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 =
+std::vector<ScoredHistoryMatch::ScoreMaxRelevance>
+ScoredHistoryMatch::GetHQPBuckets() {
+  std::string relevance_buckets_str(kDefaultRelevanceBucketsString);
+  if (OmniboxFieldTrial::HQPExperimentalScoringEnabled()) {
+    std::string experimental_scoring_buckets =
         OmniboxFieldTrial::HQPExperimentalScoringBuckets();
-    if (!hqp_experimental_scoring_buckets.empty())
-      hqp_relevance_buckets_str = hqp_experimental_scoring_buckets;
+    if (!experimental_scoring_buckets.empty())
+      relevance_buckets_str = 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);
+  return GetHQPBucketsFromString(relevance_buckets_str);
 }
 
 // static
-bool ScoredHistoryMatch::GetHQPBucketsFromString(
-    const std::string& buckets_str,
-    std::vector<ScoreMaxRelevance>* hqp_buckets) {
-  DCHECK(hqp_buckets != NULL);
+std::vector<ScoredHistoryMatch::ScoreMaxRelevance>
+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)) {

[Peter Kasting, 2016/12/08 01:29:21]

Nit: No {}

[Mark P, 2016/12/09 20:40:56]

Done.

+    return std::vector<ScoreMaxRelevance>();
+  }
+  std::vector<ScoreMaxRelevance> hqp_buckets;
+  for (base::StringPairs::const_iterator it = kv_pairs.begin();

[Mark P, 2016/12/07 22:39:27]

Despite the highlighting, this loop and its interior hasn't changed (except for
variable names and conversion of -> to .)

+       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;
 }