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

components/omnibox/browser/scored_history_match.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/omnibox/browser/scored_history_match.h"
 
 #include <math.h>
 
 #include <algorithm>
 #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"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_offset_string_conversions.h"
 #include "base/strings/utf_string_conversions.h"
 #include "components/bookmarks/browser/bookmark_utils.h"
 #include "components/omnibox/browser/history_url_provider.h"
 #include "components/omnibox/browser/omnibox_field_trial.h"
 #include "components/omnibox/browser/url_prefix.h"
 
 namespace {
 
 // The number of days of recency scores to precompute.
 const int kDaysToPrecomputeRecencyScoresFor = 366;
 
 // The number of raw term score buckets use; raw term scores greater this are
 // 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
 // lookups of scores without requiring math. This is initialized by
 // InitDaysAgoToRecencyScoreArray called by
 // ScoredHistoryMatch::Init().
 float days_ago_to_recency_score[kDaysToPrecomputeRecencyScoresFor];
 
 // Pre-computed information to speed up calculating topicality scores.
 // |raw_term_score_to_topicality_score| is a simple array mapping how raw terms
@@ skipping 63 matching lines @@
 // static
 bool ScoredHistoryMatch::also_do_hup_like_scoring_;
 float ScoredHistoryMatch::bookmark_value_;
 float ScoredHistoryMatch::typed_value_;
 bool ScoredHistoryMatch::fix_few_visits_bug_;
 bool ScoredHistoryMatch::frequency_uses_sum_;
 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(),
                          VisitInfoVector(),
                          base::string16(),
                          String16Vector(),
                          WordStarts(),
                          RowWordStarts(),
                          false,
                          base::Time::Max()) {
@@ skipping 117 matching lines @@
         likely_can_inline = true;
         innermost_match = (best_inlineable_prefix->num_components ==
                            best_prefix->num_components);
       }
     }
   }
 
   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
     // capable of being inlined a certain minimum score.  Some of these
     // are given a higher score that lets them be shown in inline.
     // This test here derives from the test in
     // HistoryURLProvider::PromoteMatchForInlineAutocomplete().
     const bool promote_to_inline =
         (row.typed_count() > 1) || (IsHostOnly() && (row.typed_count() == 1));
     int hup_like_score =
@@ skipping 132 matching lines @@
   initialized = true;
   also_do_hup_like_scoring_ = OmniboxFieldTrial::HQPAlsoDoHUPLikeScoring();
   bookmark_value_ = OmniboxFieldTrial::HQPBookmarkValue();
   typed_value_ = OmniboxFieldTrial::HQPTypedValue();
   max_visits_to_score_ = OmniboxFieldTrial::HQPMaxVisitsToScore();
   frequency_uses_sum_ = OmniboxFieldTrial::HQPFreqencyUsesSum();
   fix_few_visits_bug_ = OmniboxFieldTrial::HQPFixFewVisitsBug();
   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(
     const int num_terms,
     const base::string16& url,
     const WordStarts& terms_to_word_starts_offsets,
     const RowWordStarts& word_starts) {
   // A vector that accumulates per-term scores.  The strongest match--a
   // match in the hostname at a word boundary--is worth 10 points.
   // Everything else is less.  In general, a match that's not at a word
@@ skipping 178 matching lines @@
   // Use |max_visits_to_score_| as the denominator for the average regardless of
   // how many visits there were in order to penalize a match that has
   // fewer visits than kMaxVisitsToScore.
   if (fix_few_visits_bug_)
     return summed_visit_points / ScoredHistoryMatch::max_visits_to_score_;
   return visits.size() * summed_visit_points /
          ScoredHistoryMatch::max_visits_to_score_;
 }
 
 // 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;
   // Here's how to interpret intermediate_score: Suppose the omnibox
   // has one input term.  Suppose we have a URL for which the omnibox
   // input term has a single URL hostname hit at a word boundary.  (This
   // implies topicality_score = 1.0.).  Then the intermediate_score for
   // this URL will depend entirely on the frequency_score with
   // this interpretation:
   // - a single typed visit more than three months ago, no other visits -> 0.2
   // - a visit every three days, no typed visits -> 0.706
   // - a visit every day, no typed visits -> 0.916
   // - a single typed visit yesterday, no other visits -> 2.0
   // - a typed visit once a week -> 11.77
   // - a typed visit every three days -> 14.12
   // - at least ten typed visits today -> 20.0 (maximum score)
   //
   // 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
   //    12.0 to 20.0  --> 1300 to 1399
   //       >= 20.0    --> 1399
   //
   // The score maxes out at 1399 (i.e., cannot beat a good inlineable result
   // from HistoryURL provider).
   const float intermediate_score = topicality_score * frequency_score;
 
   // 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>();
   }
-  return false;
+  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 hqp_buckets;
 }