Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2548363010:
Omnibox - Refactor |relevance_buckets| to Remove Memory Leak on Exit (Closed)
|
Created:
4 years ago by Mark P Modified:
4 years ago Reviewers:
Peter Kasting CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionOmnibox - 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=
Committed: https://crrev.com/9007c3fbec7e2fb0d2f2f3f5d026046a56b71e13
Cr-Commit-Position: refs/heads/master@{#438006}
Patch Set 1 #Patch Set 2 : polish #
Total comments: 23
Patch Set 3 : pkasting's comments #Patch Set 4 : polish #
Total comments: 11
Patch Set 5 : ipeter's comments #Patch Set 6 : 0.8 -> 0.8f #
Messages
Total messages: 29 (14 generated)
Description was changed from ========== 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. BUG= ========== to ========== 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. No functional changes. BUG= ==========
Description was changed from ========== 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. No functional changes. BUG= ========== to ========== 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. 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= ==========
mpearson@chromium.org changed reviewers: + pkasting@chromium.org
There's a currently-leak variable in scored_history_match. This changelist cleans it up using CR_DEFINE_STATIC_LOCAL. It's a good way for me to make sure I understand how you want me to do this kind of thing. Once this change is reviewed and approved, I will apply the same strategy to fix the leaky variable in the other changelist. Please take a look, thanks. --mark https://codereview.chromium.org/2548363010/diff/20001/components/omnibox/brow... File components/omnibox/browser/scored_history_match.cc (right): https://codereview.chromium.org/2548363010/diff/20001/components/omnibox/brow... components/omnibox/browser/scored_history_match.cc:703: for (base::StringPairs::const_iterator it = kv_pairs.begin(); Despite the highlighting, this loop and its interior hasn't changed (except for variable names and conversion of -> to .)
https://codereview.chromium.org/2548363010/diff/20001/components/omnibox/brow... File components/omnibox/browser/scored_history_match.cc (right): https://codereview.chromium.org/2548363010/diff/20001/components/omnibox/brow... components/omnibox/browser/scored_history_match.cc:40: "0.0:400,1.5:600,5.0:900,10.5:1203,15.0:1300,20.0:1399"; Nit: Since both these constants are only used in one function, I suggest moving them there, so their definitions appear adjacent to their uses. https://codereview.chromium.org/2548363010/diff/20001/components/omnibox/brow... components/omnibox/browser/scored_history_match.cc:422: : kDefaultTopicalityThreshold; 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? https://codereview.chromium.org/2548363010/diff/20001/components/omnibox/brow... components/omnibox/browser/scored_history_match.cc:629: (GetHQPBuckets())); 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. https://codereview.chromium.org/2548363010/diff/20001/components/omnibox/brow... components/omnibox/browser/scored_history_match.cc:631: DCHECK(relevance_buckets.size() > 0); Nit: DCHECK(!relevance_buckets.empty()) https://codereview.chromium.org/2548363010/diff/20001/components/omnibox/brow... components/omnibox/browser/scored_history_match.cc:632: DCHECK_EQ(relevance_buckets[0].first, 0.0); Nit: (expected, actual) https://codereview.chromium.org/2548363010/diff/20001/components/omnibox/brow... components/omnibox/browser/scored_history_match.cc:699: if (!base::SplitStringIntoKeyValuePairs(buckets_str, ':', ',', &kv_pairs)) { Nit: No {} https://codereview.chromium.org/2548363010/diff/20001/components/omnibox/brow... File components/omnibox/browser/scored_history_match.h (right): https://codereview.chromium.org/2548363010/diff/20001/components/omnibox/brow... components/omnibox/browser/scored_history_match.h:138: static std::vector<ScoreMaxRelevance> GetHQPBuckets(); I don't love how often "std::vector<ScoreMaxRelevance>" gets named in these various files... a type alias might make sense. https://codereview.chromium.org/2548363010/diff/20001/components/omnibox/brow... components/omnibox/browser/scored_history_match.h:141: // return the results. For example, with |bucket_str| as Nit: buckets https://codereview.chromium.org/2548363010/diff/20001/components/omnibox/brow... components/omnibox/browser/scored_history_match.h:143: // (12.0, 1300), (20.0, 1399)]. It returns an empty list in the case of a Nit: list -> vector https://codereview.chromium.org/2548363010/diff/20001/components/omnibox/brow... File components/omnibox/browser/scored_history_match_unittest.cc (right): https://codereview.chromium.org/2548363010/diff/20001/components/omnibox/brow... components/omnibox/browser/scored_history_match_unittest.cc:658: ScoredHistoryMatch::relevance_buckets_->push_back(std::make_pair(10.0, 1000)); Can you do something like this to avoid all the push_backs? std::vector<ScoredHistoryMatch::ScoreMaxRelevance> relevance_buckets = {{0.0, 100}, {1.0, 200}, {4.0, 500}, {8.0, 900}, {10.0, 1000}}; base::AutoReset<std::vector<ScoredHistoryMatch::ScoreMaxRelevance>> tmp( ScoredHistoryMatch::relevance_buckets_, relevance_buckets);
Description was changed from ========== 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. 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= ========== to ========== 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= ==========
https://codereview.chromium.org/2548363010/diff/20001/components/omnibox/brow... File components/omnibox/browser/scored_history_match.cc (right): https://codereview.chromium.org/2548363010/diff/20001/components/omnibox/brow... components/omnibox/browser/scored_history_match.cc:40: "0.0:400,1.5:600,5.0:900,10.5:1203,15.0:1300,20.0:1399"; On 2016/12/08 01:29:21, Peter Kasting wrote: > Nit: Since both these constants are only used in one function, I suggest moving > them there, so their definitions appear adjacent to their uses. 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.) https://codereview.chromium.org/2548363010/diff/20001/components/omnibox/brow... components/omnibox/browser/scored_history_match.cc:422: : kDefaultTopicalityThreshold; On 2016/12/08 01:29:21, Peter Kasting wrote: > Nit: Should HQPExperimentalTopicalityThreshold() return the "default threshold" > when HQPExperimentalScoringEnabled() would return false? 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. > That would obviate the > need to define the threshold here (for now) and seems consistent with the other > values above? https://codereview.chromium.org/2548363010/diff/20001/components/omnibox/brow... components/omnibox/browser/scored_history_match.cc:629: (GetHQPBuckets())); On 2016/12/08 01:29:21, Peter Kasting wrote: > 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. Yeah, that's cleaner. Done. https://codereview.chromium.org/2548363010/diff/20001/components/omnibox/brow... components/omnibox/browser/scored_history_match.cc:631: DCHECK(relevance_buckets.size() > 0); On 2016/12/08 01:29:21, Peter Kasting wrote: > Nit: DCHECK(!relevance_buckets.empty()) Done. https://codereview.chromium.org/2548363010/diff/20001/components/omnibox/brow... components/omnibox/browser/scored_history_match.cc:632: DCHECK_EQ(relevance_buckets[0].first, 0.0); On 2016/12/08 01:29:21, Peter Kasting wrote: > Nit: (expected, actual) Done. https://codereview.chromium.org/2548363010/diff/20001/components/omnibox/brow... components/omnibox/browser/scored_history_match.cc:699: if (!base::SplitStringIntoKeyValuePairs(buckets_str, ':', ',', &kv_pairs)) { On 2016/12/08 01:29:21, Peter Kasting wrote: > Nit: No {} Done. https://codereview.chromium.org/2548363010/diff/20001/components/omnibox/brow... File components/omnibox/browser/scored_history_match.h (right): https://codereview.chromium.org/2548363010/diff/20001/components/omnibox/brow... components/omnibox/browser/scored_history_match.h:138: static std::vector<ScoreMaxRelevance> GetHQPBuckets(); On 2016/12/08 01:29:21, Peter Kasting wrote: > I don't love how often "std::vector<ScoreMaxRelevance>" gets named in these > various files... a type alias might make sense. Done. https://codereview.chromium.org/2548363010/diff/20001/components/omnibox/brow... components/omnibox/browser/scored_history_match.h:141: // return the results. For example, with |bucket_str| as On 2016/12/08 01:29:21, Peter Kasting wrote: > Nit: buckets Done. https://codereview.chromium.org/2548363010/diff/20001/components/omnibox/brow... components/omnibox/browser/scored_history_match.h:143: // (12.0, 1300), (20.0, 1399)]. It returns an empty list in the case of a On 2016/12/08 01:29:22, Peter Kasting wrote: > Nit: list -> vector Done. https://codereview.chromium.org/2548363010/diff/20001/components/omnibox/brow... File components/omnibox/browser/scored_history_match_unittest.cc (right): https://codereview.chromium.org/2548363010/diff/20001/components/omnibox/brow... components/omnibox/browser/scored_history_match_unittest.cc:658: ScoredHistoryMatch::relevance_buckets_->push_back(std::make_pair(10.0, 1000)); On 2016/12/08 01:29:22, Peter Kasting wrote: > Can you do something like this to avoid all the push_backs? > > std::vector<ScoredHistoryMatch::ScoreMaxRelevance> relevance_buckets = > {{0.0, 100}, {1.0, 200}, {4.0, 500}, {8.0, 900}, {10.0, 1000}}; > base::AutoReset<std::vector<ScoredHistoryMatch::ScoreMaxRelevance>> tmp( > ScoredHistoryMatch::relevance_buckets_, relevance_buckets); Done.
LGTM https://codereview.chromium.org/2548363010/diff/20001/components/omnibox/brow... File components/omnibox/browser/scored_history_match.cc (right): https://codereview.chromium.org/2548363010/diff/20001/components/omnibox/brow... components/omnibox/browser/scored_history_match.cc:40: "0.0:400,1.5:600,5.0:900,10.5:1203,15.0:1300,20.0:1399"; On 2016/12/09 20:40:56, Mark P wrote: > On 2016/12/08 01:29:21, Peter Kasting wrote: > > Nit: Since both these constants are only used in one function, I suggest > moving > > them there, so their definitions appear adjacent to their uses. > > I prefer meaningful, significant constants be declared in an obvious place, not > hidden in an implementation somewhere. 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? > 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.) 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. :) https://codereview.chromium.org/2548363010/diff/60001/components/omnibox/brow... File components/omnibox/browser/omnibox_field_trial.h (right): https://codereview.chromium.org/2548363010/diff/60001/components/omnibox/brow... components/omnibox/browser/omnibox_field_trial.h:281: // Returns the topicality threshold for HQP experiments. Returns 0.8 if Nit: I dunno if it would be clearer to say "returns a default value of 0.8 if..."? https://codereview.chromium.org/2548363010/diff/60001/components/omnibox/brow... File components/omnibox/browser/scored_history_match.cc (right): https://codereview.chromium.org/2548363010/diff/60001/components/omnibox/brow... components/omnibox/browser/scored_history_match.cc:626: ? relevance_buckets_override_ : &default_relevance_buckets; Nit: Bad indenting (See what git cl format would do) https://codereview.chromium.org/2548363010/diff/60001/components/omnibox/brow... components/omnibox/browser/scored_history_match.cc:680: std::string relevance_buckets_str(kDefaultRelevanceBucketsString); Nit: Prefer = to () for strings; see https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Variab... . https://codereview.chromium.org/2548363010/diff/60001/components/omnibox/brow... File components/omnibox/browser/scored_history_match.h (right): https://codereview.chromium.org/2548363010/diff/60001/components/omnibox/brow... components/omnibox/browser/scored_history_match.h:34: typedef std::vector<ScoreMaxRelevance> ScoreMaxRelevances; Nit: Prefer type alias ("using") to typedef (though if you do this, fix both here to be consistent) https://codereview.chromium.org/2548363010/diff/60001/components/omnibox/brow... components/omnibox/browser/scored_history_match.h:190: // Used for testing. A possibly null pointer to an vector. If set, Nit: an -> a
https://codereview.chromium.org/2548363010/diff/20001/components/omnibox/brow... File components/omnibox/browser/scored_history_match.cc (right): https://codereview.chromium.org/2548363010/diff/20001/components/omnibox/brow... components/omnibox/browser/scored_history_match.cc:40: "0.0:400,1.5:600,5.0:900,10.5:1203,15.0:1300,20.0:1399"; On 2016/12/10 02:31:15, Peter Kasting wrote: > On 2016/12/09 20:40:56, Mark P wrote: > > On 2016/12/08 01:29:21, Peter Kasting wrote: > > > Nit: Since both these constants are only used in one function, I suggest > > moving > > > them there, so their definitions appear adjacent to their uses. > > > > I prefer meaningful, significant constants be declared in an obvious place, > not > > hidden in an implementation somewhere. > > You mean, like, up here? :) Touché. > 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. Seems like a reasonable interpretation. > 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? > > > 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.) > > 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. :) Okay, I'm convinced. Done. https://codereview.chromium.org/2548363010/diff/60001/components/omnibox/brow... File components/omnibox/browser/omnibox_field_trial.h (right): https://codereview.chromium.org/2548363010/diff/60001/components/omnibox/brow... components/omnibox/browser/omnibox_field_trial.h:281: // Returns the topicality threshold for HQP experiments. Returns 0.8 if On 2016/12/10 02:31:15, Peter Kasting wrote: > Nit: I dunno if it would be clearer to say "returns a default value of 0.8 > if..."? I agree. Made two changes: added the mention of "default" and removed the reference to the parameter name. None of the other trials here mention the parameter name. https://codereview.chromium.org/2548363010/diff/60001/components/omnibox/brow... File components/omnibox/browser/scored_history_match.cc (right): https://codereview.chromium.org/2548363010/diff/60001/components/omnibox/brow... components/omnibox/browser/scored_history_match.cc:626: ? relevance_buckets_override_ : &default_relevance_buckets; On 2016/12/10 02:31:15, Peter Kasting wrote: > Nit: Bad indenting (See what git cl format would do) Done. https://codereview.chromium.org/2548363010/diff/60001/components/omnibox/brow... components/omnibox/browser/scored_history_match.cc:680: std::string relevance_buckets_str(kDefaultRelevanceBucketsString); On 2016/12/10 02:31:15, Peter Kasting wrote: > Nit: Prefer = to () for strings; see > https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Variab... > . Done. https://codereview.chromium.org/2548363010/diff/60001/components/omnibox/brow... File components/omnibox/browser/scored_history_match.h (right): https://codereview.chromium.org/2548363010/diff/60001/components/omnibox/brow... components/omnibox/browser/scored_history_match.h:34: typedef std::vector<ScoreMaxRelevance> ScoreMaxRelevances; On 2016/12/10 02:31:15, Peter Kasting wrote: > Nit: Prefer type alias ("using") to typedef (though if you do this, fix both > here to be consistent) Done. (Any reason for this preference? Or is it arbitrary: pick one for consistency and use it everywhere. I thought they were equivalent.) https://codereview.chromium.org/2548363010/diff/60001/components/omnibox/brow... components/omnibox/browser/scored_history_match.h:190: // Used for testing. A possibly null pointer to an vector. If set, On 2016/12/10 02:31:15, Peter Kasting wrote: > Nit: an -> a Done.
https://codereview.chromium.org/2548363010/diff/60001/components/omnibox/brow... File components/omnibox/browser/scored_history_match.h (right): https://codereview.chromium.org/2548363010/diff/60001/components/omnibox/brow... components/omnibox/browser/scored_history_match.h:34: typedef std::vector<ScoreMaxRelevance> ScoreMaxRelevances; On 2016/12/11 04:57:22, Mark P wrote: > On 2016/12/10 02:31:15, Peter Kasting wrote: > > Nit: Prefer type alias ("using") to typedef (though if you do this, fix both > > here to be consistent) > > Done. (Any reason for this preference? Or is it arbitrary: pick one for > consistency and use it everywhere. I thought they were equivalent.) Very minor differences: * "using" syntax is more generalizable (with templates), so if you're going to "pick one", that's the one that will work in more cases * "using A=B" is more instantly obvious about which type is the "original" and which is the "newly declared" type. Perhaps I'm dumb, but after all these years I still have to think carefully about that with typedefs when it's not obvious ("typedef UnfamiliarTypeX UnfamiliarTypeY").
The CQ bit was checked by mpearson@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2548363010/#ps80001 (title: "ipeter's comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by mpearson@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2548363010/#ps100001 (title: "0.8 -> 0.8f")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by mpearson@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1481589530708080,
"parent_rev": "c6b97d212ba54f0230a0d42dc265c06e79f16e33", "commit_rev":
"194e7226ed8eff24e6f86b6ea4088463077218ab"}
Message was sent while issue was closed.
Description was changed from ========== 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= ========== to ========== 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 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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= Committed: https://crrev.com/9007c3fbec7e2fb0d2f2f3f5d026046a56b71e13 Cr-Commit-Position: refs/heads/master@{#438006} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/9007c3fbec7e2fb0d2f2f3f5d026046a56b71e13 Cr-Commit-Position: refs/heads/master@{#438006}
Message was sent while issue was closed.
BTW, I forgot to ever write this before, but your CL description is technically incorrect. Using CR_DEFINE_STATIC_LOCAL doesn't result in objects being cleaned up on exit. It basically just makes it clear that this memory leak is intentional, and tools and readers don't need to freak out about it.
Message was sent while issue was closed.
On Tue, Dec 13, 2016 at 2:00 PM, <pkasting@chromium.org> wrote: > BTW, I forgot to ever write this before, but your CL description is > technically > incorrect. Using CR_DEFINE_STATIC_LOCAL doesn't result in objects being > cleaned > up on exit. It basically just makes it clear that this memory leak is > intentional, and tools and readers don't need to freak out about it. > Thanks for the clarification. I didn't know that. --mark > > https://codereview.chromium.org/2548363010/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
