I figured out some of this.  On my toy history, I have a search query from history that's highly scoring for this input.  We call Classify() on these type of search queries.
https://cs.chromium.org/chromium/src/components/omnibox/browser/search_provider.cc?l=1199

This explains some of the extra calls (that end with 1 result, as that search query string ends up matching one result from URL history).

Also, I see somewhat different results when I test interactively in the omnibox.  chrome://omnibox/ is different.  The remaining concerns I have:

Typing "c" in results in 3 calls (4, 4, 1).  I can understand two of those three.

Having "c " in the omnibox, focusing in the middle, and pressing space, results in 2 calls (4, 4).  These two calls are due to HQP breaking on the cursor position.  If there is a space adjacent to the cursor position, searching a second time for what is effectively the same string is unnecessary / inefficient.  I have filed this as a  bug 670003 .

So the only extra call I don't understand is the case mentioned above.  This extra call only happens on single-character inputs, not multiple ones.  Details to follow.

One call has the straightforward, expected call stack:
#0  URLIndexPrivateData::HistoryIdSetToScoredMatches (this, history_id_set=std::__debug::set with 4 elements = {...},
    lower_raw_string="c\000", template_url_service=0x2fe56a77a420, bookmark_model=0x2fe56a77a820, scored_items=0x7fffffff7600)
    at ../../components/omnibox/browser/url_index_private_data.cc:719
URLIndexPrivateData::HistoryItemsForTerms (this, original_search_string="c\000", cursor_position=1,
    max_matches=3, bookmark_model=0x2fe56a77a820, template_url_service=0x2fe56a77a420)
    at ../../components/omnibox/browser/url_index_private_data.cc:229
InMemoryURLIndex::HistoryItemsForTerms (this, term_string="c\000", cursor_position=1, max_matches=3)
    at ../../components/omnibox/browser/in_memory_url_index.cc:130
HistoryQuickProvider::DoAutocomplete (this,
    at ../../components/omnibox/browser/history_quick_provider.cc:70
HistoryQuickProvider::Start (this, input=..., minimal_changes=false)
    at ../../components/omnibox/browser/history_quick_provider.cc:61
AutocompleteController::Start (this, input=...)
    at ../../components/omnibox/browser/autocomplete_controller.cc:292
OmniboxController::StartAutocomplete (this, input=...)
    at ../../components/omnibox/browser/omnibox_controller.cc:39
OmniboxEditModel::StartAutocomplete (this, has_selected_text=false, prevent_inline_autocomplete=false)
    at ../../components/omnibox/browser/omnibox_edit_model.cc:403
OmniboxViewViews::UpdatePopup (this, at ../../chrome/browser/ui/views/omnibox/omnibox_view_views.cc:447
OmniboxEditModel::OnAfterPossibleChange (this, state_changes=..., allow_keyword_ui_change=true)
    at ../../components/omnibox/browser/omnibox_edit_model.cc:1159
OmniboxViewViews::OnAfterPossibleChange (this, allow_keyword_ui_change=true)
    at ../../chrome/browser/ui/views/omnibox/omnibox_view_views.cc:513
OmniboxViewViews::OnAfterUserAction (this, sender=0x2fe56b11de38)
    at ../../chrome/browser/ui/views/omnibox/omnibox_view_views.cc:972
views::Textfield::OnAfterUserAction (this, at ../../ui/views/controls/textfield/textfield.cc:1945
views::Textfield::DoInsertChar (this, ch=99) at ../../ui/views/controls/textfield/textfield.cc:1551
OmniboxViewViews::DoInsertChar (this, ch=99)

The other call (the one that happens chronologically first) has the below call stack.

The key difference seems to be in OmniboxViewViews::UpdatePopup().
https://cs.chromium.org/chromium/src/chrome/browser/ui/views/omnibox/omnibox_view_views.cc?l=444-452

At the beginning of this function, we call model()->SetInputInProgress(true). This filters down through a complex series of calls and eventually hits OmniboxEditModel::GetInfoForCurrentText(), which calls Classify(), which starts autocompletion.  (Side comment: this round of autocompletion gets called with the cursor_position is npos.)

Then, later in the function, we call model()->StartAutocomplete() directly.

#0  URLIndexPrivateData::HistoryIdSetToScoredMatches (this, history_id_set=std::__debug::set with 4 elements = {...},
    lower_raw_string="c\000", template_url_service=0x2fe56a77a420, bookmark_model=0x2fe56a77a820, scored_items=0x7fffffff6330)
    at ../../components/omnibox/browser/url_index_private_data.cc:719
URLIndexPrivateData::HistoryItemsForTerms (this, original_search_string="c\000",
    cursor_position=18446744073709551615, max_matches=3, bookmark_model=0x2fe56a77a820, template_url_service=0x2fe56a77a420)
    at ../../components/omnibox/browser/url_index_private_data.cc:229
InMemoryURLIndex::HistoryItemsForTerms (this, term_string="c\000",
    cursor_position=18446744073709551615, max_matches=3) at ../../components/omnibox/browser/in_memory_url_index.cc:130
HistoryQuickProvider::DoAutocomplete (this,
    at ../../components/omnibox/browser/history_quick_provider.cc:70
HistoryQuickProvider::Start (this, input=..., minimal_changes=false)
    at ../../components/omnibox/browser/history_quick_provider.cc:61
AutocompleteController::Start (this, input=...)
    at ../../components/omnibox/browser/autocomplete_controller.cc:292
AutocompleteClassifier::Classify (this, text="c\000", prefer_keyword=false,
    allow_exact_keyword_match=true,
    page_classification=metrics::OmniboxEventProto_PageClassification_INSTANT_NTP_WITH_OMNIBOX_AS_STARTING_FOCUS, match=0x7fffffff7fa0,
    alternate_nav_url=0x0) at ../../components/omnibox/browser/autocomplete_classifier.cc:65
OmniboxEditModel::GetInfoForCurrentText (this, match=0x7fffffff7fa0, alternate_nav_url=0x0)
    at ../../components/omnibox/browser/omnibox_edit_model.cc:1280
OmniboxEditModel::CurrentMatch (this, alternate_nav_url=0x0)
    at ../../components/omnibox/browser/omnibox_edit_model.cc:194
OmniboxEditModel::CurrentTextType (this, at ../../components/omnibox/browser/omnibox_edit_model.cc:274
OmniboxView::GetVectorIcon (this, at ../../components/omnibox/browser/omnibox_view.cc:87
LocationBarView::RefreshLocationIcon (this,
    at ../../chrome/browser/ui/views/location_bar/location_bar_view.cc:759
LocationBarView::OnChanged (this,
    at ../../chrome/browser/ui/views/location_bar/location_bar_view.cc:1244
LocationBarView::Update (this, contents=0x0)
    at ../../chrome/browser/ui/views/location_bar/location_bar_view.cc:706
LocationBarView::UpdateWithoutTabRestore (this,
    at ../../chrome/browser/ui/views/location_bar/location_bar_view.cc:717
ChromeOmniboxEditController::OnInputInProgress (this, in_progress=true)
    at ../../chrome/browser/ui/omnibox/chrome_omnibox_edit_controller.cc:40
OmniboxEditModel::SetInputInProgress (this, in_progress=true)
    at ../../components/omnibox/browser/omnibox_edit_model.cc:359
OmniboxViewViews::UpdatePopup (this, at ../../chrome/browser/ui/views/omnibox/omnibox_view_views.cc:441
OmniboxEditModel::OnAfterPossibleChange (this, state_changes=..., allow_keyword_ui_change=true)
    at ../../components/omnibox/browser/omnibox_edit_model.cc:1159
OmniboxViewViews::OnAfterPossibleChange (this, allow_keyword_ui_change=true)
    at ../../chrome/browser/ui/views/omnibox/omnibox_view_views.cc:513
OmniboxViewViews::OnAfterUserAction (this, sender=0x2fe56b11de38)
    at ../../chrome/browser/ui/views/omnibox/omnibox_view_views.cc:972
views::Textfield::OnAfterUserAction (this, at ../../ui/views/controls/textfield/textfield.cc:1945
views::Textfield::DoInsertChar (this, ch=99) at ../../ui/views/controls/textfield/textfield.cc:1551
OmniboxViewViews::DoInsertChar (this, ch=99)

Assigning to pkasting@ regarding comments #1-2.  Is it expected that switching the omnibox from no input in progress to input in progress results in a call to Classify()?  (Then, immediately after, we get a proper call to StartAutocomplete.)

This happens upon typing the first character into the omnibox or pasting a multi-character string into an omnibox that does not already have input in progress.

Mark, is there more work to be done here _besides_ removing the unnecessary Classify()?  If not, we should change the title.  If so, fixing Classify() should probably be split into a separate bug to leave this about said remaining work.

Now, regarding that Classify() call.  I think we should try and fix this (it affects omnibox responsiveness when typing the first character, which is the most important).

The first fix I thought of would be to move the SetInputInProgress() call until after StartAutocomplete().  This would skip Classify() in the end, because we'd have a match to return directly.  However, SetInputInProgress() does various things like record "time to start editing" that I worry about delaying until after starting autocompletion (because the time to run the synchronous pass will now be included in that number).  I don't think we can move all of this.

A better fix, I think, would be to notify the OmniboxEditController that input is in progress later.  This notification will result in things like the content settings views in the location bar disappearing.  It seems OK -- better, even -- for these things to happen at the time when the synchronous pass returns and we actually open the dropdown.

Should OnInputStateChanged() be moved later as well?  That ultimately seems to call into SearchTabHelper::UpdateMode(), and I'm not familiar enough with this to know the consequences of moving it.

Once we figure this out, we need to refactor SetInputInProgress() into two pieces, one that goes before any autocompletion, and one that goes after.  This is error-prone for callers; we _might_ be able to make this only accessible via instantiating a scoping object that does the "early" phase on construction and the "late" phase on destruction.

(While reading SetInputInProgress(), I also found myself wondering why the code relating to |user_input_since_focus_| had to go before the check to see if we were already in progress.  I suspect there's a subtle good reason for this, but if so, the code needs to document it.)

> Mark, is there more work to be done here _besides_ removing the unnecessary
> Classify()?  If not, we should change the title.
No other work.  Feel free to change the title.

>>>
Should OnInputStateChanged() be moved later as well?  That ultimately seems to call into SearchTabHelper::UpdateMode(), and I'm not familiar enough with this to know the consequences of moving it.
>>>
I'm not familiar with this at all.

>>>
(While reading SetInputInProgress(), I also found myself wondering why the code relating to |user_input_since_focus_| had to go before the check to see if we were already in progress.  I suspect there's a subtle good reason for this, but if so, the code needs to document it.)
>>>
I'm not sure what you're referring to.   Are you referring to this?
https://cs.chromium.org/chromium/src/components/omnibox/browser/omnibox_edit_model.cc?l=349-350
There's a little subtlety here.  If you want to move the early return up, the only time it would make a difference is when
in_progress = true
user_input_in_progress_ = true
user_input_since_focus_ = false

I think this can only happen if you have an omnibox with a tab that you've been editing, you switch to another tab, then switch back and add another character to the text in the omnibox.  In cases like this, I think the current code is likely behaving right: recording the focus to edit time for this new focus + edit.  If you early return, you won't get an emit for this histogram.  Adding a comment is a good idea.


I'm not replying to anything else in comment #4 because I think your statements sound correct and your analysis is certainly better than anything I could've done.

The edit model, controller, and friends are not my cup of tea.  Can I leave this bug assigned to you to do or hand off (to someone besides me) as you will?

You can leave this assigned to me.

I will think about the example you gave.  Another potential case is when the user defocuses the omnibox while editing it (by clicking on the page), then refocuses it.  I need to step through both these cases to see what they record, and consider whether this is the right place to record it.

pkasting@ wrote:
>>>
(While reading SetInputInProgress(), I also found myself wondering why the code relating to |user_input_since_focus_| had to go before the check to see if we were already in progress.  I suspect there's a subtle good reason for this, but if so, the code needs to document it.)
>>>
The case you're worried about is when user_input_in_progress_ == true, in_progress == true, and user_input_since_focus_ == false.  I think this can happen with tab-switching, that is, switching back to an existing tab with an omnibox already focussed / open in progress.  If we switch back to the tab, the omnibox becomes focussed automatically, it likely becomes user_input_in_progress_ = true (I'd think), and yet the user hasn't added user input since the omnibox was focussed (so user_input_since_focus_ should be false).

This is my theory at least.  The changes that added that code don't mention this.
https://chromiumcodereview.appspot.com/18866003
https://chromiumcodereview.appspot.com/21452002

treib@, you're familiar with SearchTabHelper.  Can you weigh in on the question below?

We need to do some refactoring of the omnibox model in order to help performance upon the first character.  Right now it's doing duplicate work.  This refactoring would involve notifying the OmniboxEditController that input is in progress later.  This notification will result in things like the content settings views in the location bar disappearing.  It seems OK -- better, even -- for these things to happen at the time when the synchronous pass returns and we actually open the dropdown.

As part of this, we're wondering if we can/should move OnInputStateChanged() later as well.  That ultimately seems to call into SearchTabHelper::UpdateMode().  We're not familiar enough with this to know the consequences of moving it.  Let us know.

thanks,
mark

P.S. Much of this message was stolen from pkasting@'s comment #4.  It was well written.  Credit given where credit is due. :-)

ping treib@ for comment #9

Assigning to treib@ per comment #9.  Please assign it back to me or pkasting@ after you answer.

Sorry, I completely missed this :(

These days, all that SearchTabHelper::OnInputStateChanged does is tell the NTP to hide/unhide the fakebox and logo. It should be perfectly fine if that happens slightly later. Thanks for checking!

Narrowing to Views platforms.  

I will not have time to work on this anytime soon.  Putting back in pool.  I still think this is worth doing.


I thought about Peter's plan from comment #4.  I think SetInputInProgress() should be reorganized into two pieces:
SetInputInProgress()
and
NotifyWatchersInputInProgress().  The latter does
  controller_->OnInputInProgress(in_progress);

  if (user_input_in_progress_ || !in_revert_)
    client_->OnInputStateChanged();


I'm not convinced with this kind of split Peter's suggestion of using "a scoping object" to make sure both parts get called is appropriate or necessary.  (I can imagine some places where we don't want to notify callers.)

Or maybe I'm not too concerned about callers calling one without the other.  After all, there are only four (non-test) callers in the whole code base.

+jdonnelly

I've looked into this a bit. I only see the double-call on the very first character after a navigation. If you hit a char, then backspace, then a char, you only see a single call - the one through StartAuto - historical matches or not.

If this is useful, it's a simple enough change (although perhaps with some risk as always.)

What I think is more interesting is that, with other triggers, I'm seeing up to 8 calls to Classify - 3 blank, 5 with the Omnibox contents. Looking at the stack traces makes it pretty obvious what is happening. Here's one example:

OV::TextChanged()
OVV::EmphasizedURLComponents()  // also calls CurrentTextIsURL directly
     UpdateTextStyle()
OEM::CurrentTextIsURL()

Another:

OVV::RevertAll()
OV::RevertAll()  // also calls TextChanged directly
OEM::Revert()
OVV::SetWindowTextAndCaretPos()
OV::TextChanged()  // see above :(

As you can see, the redundancy multiplies.

Fixing these might be pretty tricky. Certainly untieing a stack trace pays dividends, but what might be simpler is to avoid redoing the expensive parts of the calls.

The bit in common among the 8 calls is OEM: :CurrentMatch, called by OnChanged, CurrentTextIsURL and CurrentTextType. It calls:

  AutocompleteMatch match = omnibox_controller_->current_match();

  if (!match.destination_url.is_valid()) {
    GetInfoForCurrentText(&match, alternate_nav_url);
  ...

If we could let CurrentMatch() update the controller's copy of the match instead of a temporary, perhaps the subsequent calls would call it valid.

I added a non-const version of OmniboxController: :current_match() and changed the above line to:

  AutocompleteMatch& match = ...
                   ^

and now I only see a single call reach Classify for each switch. My first question is, of course, did I break something? If not, is this useful? My print-outs give the impression that a lot of work is being avoided, but I suspect people don't switch tabs too often and if they do, have to deal with a large redraw anyways. Still, if it's a harmless improvement...

I can include the other stack redundancies if people think there's fruit there too.

I have trouble following your post.  I think eliminating the extra duplicate call is the only one that can easily be done.  It sounds like you want to remove the call to classify that come through the OmniboxEditModel::CurrentMatch() code path.  But that one, gotten to via GetInfoForCurrentText() and then the else clause in that function is clearly because the autocomplete system hasn't been run with the current input.  There's not way to get around running that call.  (This call happens when there is edited text in the omnibox but the dropdown is not open.  E.g., you type something in the omnibox, switch tabs, then switch back.  The omnibox doesn't have any memory of what that text in the omnibox could do.)

That said, maybe I'm misunderstanding your message entirely.  Feel free to send me a code review if you think you've made an improvement that works.  When I investigated extra Classify() calls, the only ones I saw that were actually extra (in my opinion) were on the first keystroke.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8625456d9b11a702630c631a0d5dbd3e36041ff9

commit 8625456d9b11a702630c631a0d5dbd3e36041ff9
Author: Kevin Bailey <krb@chromium.org>
Date: Thu Feb 01 01:45:13 2018

[omnibox] Avoid calls to Classify() surrounding CurrentTextIsURL()

OmniboxView::UpdateTextStyle() needs to know if the text is a URL, but
so does its caller. Rather than calculate it twice - a non-trivial
run through Classify() - pass it.

Bug:  669727 
Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs
Change-Id: I6a8be0bfaaf7fd6c2b5ada831de65340e71d47d6
Reviewed-on: https://chromium-review.googlesource.com/890658
Commit-Queue: Kevin Bailey <krb@chromium.org>
Reviewed-by: Rohit Rao <rohitrao@chromium.org>
Reviewed-by: Mark Pearson <mpearson@chromium.org>
Cr-Commit-Position: refs/heads/master@{#533518}
[modify] https://crrev.com/8625456d9b11a702630c631a0d5dbd3e36041ff9/chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm
[modify] https://crrev.com/8625456d9b11a702630c631a0d5dbd3e36041ff9/chrome/browser/ui/views/omnibox/omnibox_view_views.cc
[modify] https://crrev.com/8625456d9b11a702630c631a0d5dbd3e36041ff9/chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc
[modify] https://crrev.com/8625456d9b11a702630c631a0d5dbd3e36041ff9/components/omnibox/browser/omnibox_view.cc
[modify] https://crrev.com/8625456d9b11a702630c631a0d5dbd3e36041ff9/components/omnibox/browser/omnibox_view.h
[modify] https://crrev.com/8625456d9b11a702630c631a0d5dbd3e36041ff9/ios/chrome/browser/ui/omnibox/omnibox_view_ios.mm

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6befda714b78527946d7bcf300d8d00218ea11d2

commit 6befda714b78527946d7bcf300d8d00218ea11d2
Author: Kevin Bailey <krb@chromium.org>
Date: Thu Feb 01 18:40:48 2018

[omnibox] Remove Unnecessary Calls to StartAutocomplete

This change is to split SetInputInProgress() so the notifications
occur after the Start. This change reduces calls to StartAutocomplete
from 2 to 1 on the first character typed.

Bug:  669727 
Change-Id: I6fbba62284a3cd6c8192ffb9c9d1381433ebe798
Reviewed-on: https://chromium-review.googlesource.com/883947
Reviewed-by: Rohit Rao <rohitrao@chromium.org>
Reviewed-by: Mark Pearson <mpearson@chromium.org>
Commit-Queue: Kevin Bailey <krb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#533751}
[modify] https://crrev.com/6befda714b78527946d7bcf300d8d00218ea11d2/chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm
[modify] https://crrev.com/6befda714b78527946d7bcf300d8d00218ea11d2/chrome/browser/ui/views/omnibox/omnibox_view_views.cc
[modify] https://crrev.com/6befda714b78527946d7bcf300d8d00218ea11d2/components/omnibox/browser/omnibox_edit_model.cc
[modify] https://crrev.com/6befda714b78527946d7bcf300d8d00218ea11d2/components/omnibox/browser/omnibox_edit_model.h