Skip to content

Commit

Permalink
Revert "[omnibox][ml] Remove scoring signals for history fuzzy provid…
Browse files Browse the repository at this point in the history
…er matches."

This reverts commit 6300f06.

Reason for revert: match.scoring_signals is null, causing crash

Original change's description:
> [omnibox][ml] Remove scoring signals for history fuzzy provider matches.
>
> The fuzzy provider works by running the bookmark and history quick
> providers with a "corrected" input and applying a penalty proportional
> to the correction amount to the results.  The scoring signals are
> populated according to the correct input and leads to an artificially
> inflated model output.
>
> We previously attempted to correct for this by re-applying the penalty
> after re-scoring.  This was not sufficient in demoting the fuzzy
> suggestions.
>
> This CL attempts to correct for this by clearing the
> scoring signals in the fuzzy provider, as these are not accurate for
> the actual input, resulting in fuzzy matches no longer being re-scored
> by the model.
>
> Bug: 1472374, 1405555
> Change-Id: I10bd5036179513a156c6e1ba15713ff1b12eb8fa
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4775323
> Code-Coverage: findit-for-me@appspot.gserviceaccount.com <findit-for-me@appspot.gserviceaccount.com>
> Reviewed-by: manuk hovanesian <manukh@chromium.org>
> Commit-Queue: Angela Yoeurng <yoangela@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#1184412}

Bug: 1472374, 1405555
Change-Id: Ib94480da1bc69bfce486aebec7bf041f02ff9323
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4794136
Auto-Submit: Daniel Yip <danielyip@google.com>
Owners-Override: Daniel Yip <danielyip@google.com>
Commit-Queue: Daniel Yip <danielyip@google.com>
Reviewed-by: Srinivas Sista <srinivassista@chromium.org>
Cr-Commit-Position: refs/branch-heads/5956@{#7}
Cr-Branched-From: c219341-refs/heads/main@{#1185115}
  • Loading branch information
Orin Jaworski authored and Chromium LUCI CQ committed Aug 18, 2023
1 parent f85a7ac commit a5c07ea
Show file tree
Hide file tree
Showing 5 changed files with 34 additions and 9 deletions.
12 changes: 12 additions & 0 deletions components/omnibox/browser/autocomplete_controller.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1807,6 +1807,18 @@ void AutocompleteController::OnUrlScoringModelDone(
match_itr->RecordAdditionalInfo("ml_legacy_relevance",
match_itr->relevance);
match_itr->relevance = relevance_heap.top();

// Fuzzy matches use the scoring signals for history and bookmark
// providers with the "corrected" input. This leads to an artificially
// high confidence from the model. Correct for this by re-applying the
// penalty from the history fuzzy provider.
if (match_itr->provider && match_itr->provider->type() ==
AutocompleteProvider::TYPE_HISTORY_FUZZY) {
match_itr->RecordAdditionalInfo("ml_relevance_before_penalty",
match_itr->relevance);
HistoryFuzzyProvider::ApplyRelevancePenalty(
*match_itr, match_itr->fuzzy_match_penalty);
}
}
relevance_heap.pop();
prediction_and_match_itr_heap.pop();
Expand Down
3 changes: 3 additions & 0 deletions components/omnibox/browser/autocomplete_match.cc
Original file line number Diff line number Diff line change
Expand Up @@ -258,6 +258,7 @@ AutocompleteMatch::AutocompleteMatch(const AutocompleteMatch& match)
: provider(match.provider),
relevance(match.relevance),
typed_count(match.typed_count),
fuzzy_match_penalty(match.fuzzy_match_penalty),
deletable(match.deletable),
fill_into_edit(match.fill_into_edit),
additional_text(match.additional_text),
Expand Down Expand Up @@ -318,6 +319,7 @@ AutocompleteMatch& AutocompleteMatch::operator=(
provider = std::move(match.provider);
relevance = std::move(match.relevance);
typed_count = std::move(match.typed_count);
fuzzy_match_penalty = std::move(match.fuzzy_match_penalty);
deletable = std::move(match.deletable);
fill_into_edit = std::move(match.fill_into_edit);
additional_text = std::move(match.additional_text);
Expand Down Expand Up @@ -387,6 +389,7 @@ AutocompleteMatch& AutocompleteMatch::operator=(
provider = match.provider;
relevance = match.relevance;
typed_count = match.typed_count;
fuzzy_match_penalty = match.fuzzy_match_penalty;
deletable = match.deletable;
fill_into_edit = match.fill_into_edit;
additional_text = match.additional_text;
Expand Down
4 changes: 4 additions & 0 deletions components/omnibox/browser/autocomplete_match.h
Original file line number Diff line number Diff line change
Expand Up @@ -641,6 +641,10 @@ struct AutocompleteMatch {
// set for matches from HistoryURL and HistoryQuickProvider.
int typed_count = -1;

// The percentage deducted from the relevance score by the history fuzzy
// provider. This is currently used to re-apply the penalty after ML scoring.
int fuzzy_match_penalty = 0;

// True if the user should be able to delete this match.
bool deletable = false;

Expand Down
19 changes: 10 additions & 9 deletions components/omnibox/browser/history_fuzzy_provider.cc
Original file line number Diff line number Diff line change
Expand Up @@ -567,6 +567,15 @@ size_t HistoryFuzzyProvider::EstimateMemoryUsage() const {
return res;
}

// static
void HistoryFuzzyProvider::ApplyRelevancePenalty(AutocompleteMatch& match,
int penalty) {
DCHECK_GE(penalty, 0);
DCHECK_LE(penalty, 100);
match.relevance -= match.relevance * penalty / 100;
match.fuzzy_match_penalty = penalty;
}

HistoryFuzzyProvider::~HistoryFuzzyProvider() = default;

void HistoryFuzzyProvider::DoAutocomplete() {
Expand Down Expand Up @@ -698,15 +707,7 @@ int HistoryFuzzyProvider::AddConvertedMatches(const ACMatches& matches,

// Apply relevance penalty; all corrections are equal and we only apply this
// to the most relevant result, so edit distance isn't needed.
DCHECK_GE(penalty, 0);
DCHECK_LE(penalty, 100);
match.relevance -= match.relevance * penalty / 100;

// Scoring signals are calculated in the history and bookmark providers using
// the corrected input. These scoring signals are inaccurate for the true
// input, so clear them to prevent the ml model assigning an
// artificially high confidence to this suggestion.
match.scoring_signals->Clear();
ApplyRelevancePenalty(match, penalty);

return 1;
}
Expand Down
5 changes: 5 additions & 0 deletions components/omnibox/browser/history_fuzzy_provider.h
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,11 @@ class HistoryFuzzyProvider : public HistoryProvider,
// See base/trace_event/memory_usage_estimator.h for more info.
size_t EstimateMemoryUsage() const override;

// Reduces a match's relevance score according to the penalty and records the
// penalty amount. `penalty` is the percentage that will be deducted from the
// match's relevance.
static void ApplyRelevancePenalty(AutocompleteMatch& match, int penalty);

private:
~HistoryFuzzyProvider() override;

Expand Down

0 comments on commit a5c07ea

Please sign in to comment.