Which platforms are affected by this issue?

(On Android, we just compare strings, so I assume you mean the search::IsNTPURL() function from chrome/browser/search/search.h? ... and even there, it's not obvious how this is connected to the described problems)

csharrison, can you expand a bit on what exactly you think is slow? On Android in particular, all the "Instant" stuff will early-out (IsInstantExtendedAPIEnabled returns false), so as fhorschig says it essentially boils down to a strcmp.

Ouch! Sorry about that. I was running profiles on Linux and traces on Android and I may have made some wrong assumptions trying to associate the two (e.g. method A is slow on both platforms and on Linux it is slow partly b/c of NTP).

Most of my measurements of NTP slowness was on Linux, and I don't have direct evidence that this is slow on Android, except for circumstantial evidence :)

As such, probably low priority until I do more investigating.

No worries! If it's slow on Linux, then that probably means it's slow on Win and Mac as well, and it might well be worth fixing.

So, do you know what exactly it is that's so slow? Which of the "is NTP" functions did you look at? We have about 5 of them... :-/

I have the flamegraph URL saved but for some reason it isn't in my Chrome history. When I get into the office let me check I could have navigated on a Chrome instance I wasn't logged into :P.

In general it seemed like a death of a thousand cuts starting from NewTabURLDetails::ForProfile. I remember template URL stuff being slow, and I can also see a couple of GURL reparsing / copying / compares which aren't snappy. Maybe also supervised user code? The code could probably be optimized but it didn't look easy to me, which was why my first instinct was to just cache it. Caching it also could remove some copying since the APIs could return const refs instead of copied GURLs. 

In general, the #1 thing imo to avoid with URLs is re-parsing them (e.g. giving a string input to GURL constructor). This is because parsing / canonicalization requires scanning the URL potentially multiple times and doing validity checks.

Thanks for the explanation! You're right that NewTabURLDetails::ForProfile does a whole bunch of stuff which I guess adds up. The problem with caching is that things can change at more or less any time, e.g. when the default search engine changes. However, I think it's possible to rearrange that method a bit to avoid at least some of the reparsing in the common cases.

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

commit 3ac2b20f294ae4bbbaa142c2fdee9c588f0412ce
Author: Marc Treib <treib@chromium.org>
Date: Fri Nov 17 16:05:48 2017

NTP: Avoid supervised user URL filter checks for non-SUs

These checks can be fairly expensive, and they're called on the critical
path for all navigations. No reason to pay the cost for non-SUs.

Bug: 786035
Change-Id: Id47d0b2fe4c86371fd4c277e363a4f7fcfd49194
Reviewed-on: https://chromium-review.googlesource.com/775959
Reviewed-by: Charlie Harrison <csharrison@chromium.org>
Commit-Queue: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#517413}
[modify] https://crrev.com/3ac2b20f294ae4bbbaa142c2fdee9c588f0412ce/chrome/browser/search/search.cc
[modify] https://crrev.com/3ac2b20f294ae4bbbaa142c2fdee9c588f0412ce/chrome/browser/search/search_unittest.cc

Zine triage: assigning this "Untriaged" issue to treib as he seems to have started working on it.

Eh, sure, though I'm not actively working on this so feel free to grab.

Had some time today and put together https://crrev.com/c/793151. I think that's about as far as we can go here without major restructuring.

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

commit 21b35066287034fac8a282d5d635482ad07f7e96
Author: Marc Treib <treib@chromium.org>
Date: Tue Nov 28 16:38:20 2017

NewTabURLDetails: Reorder to avoid expensive URL parsing in some cases

For now, this will make little difference, but once the local NTP is
used by default, we'll avoid re-parsing the search provider's NTP URL
twice each time.

Bug: 786035
Change-Id: Ifa7e5095cee881caa943d1308b798cd39b962de6
Reviewed-on: https://chromium-review.googlesource.com/793151
Reviewed-by: Charlie Harrison <csharrison@chromium.org>
Commit-Queue: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#519704}
[modify] https://crrev.com/21b35066287034fac8a282d5d635482ad07f7e96/chrome/browser/search/search.cc

That does most of it I think. For now, we still parse GURL(chrome::kChromeSearchLocalNtpUrl) every time. As a further optimization, we could cache that by storing it in a LazyInstance, or attaching it to the Profile as UserData, or something along those lines.

Marking available, since I don't have concrete plans to do any more here soon.