We now have CanAddURI and IsRecentlyVisitedURI in AndroidHistory. We can use those to do some quick URI checks before calling the JNI method to lookup URIs. While doing some Traceview profiling of pageloads, I see CheckURIVisited showing up. It's not high in the profile, but I remembered that desktop uses a quick check to avoid doing the SQL lookup. We can do the same. It might not add up to much, but I don't see a downside.

Comment on attachment 8355013 [details] [diff] [review] Quick URI checks v0.1 Review of attachment 8355013 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/components/build/nsAndroidHistory.cpp @@ +46,5 @@ > + bool canAdd; > + nsresult rv = CanAddURI(aURI, &canAdd); > + NS_ENSURE_SUCCESS(rv, rv); > + if (!canAdd) { > + return NS_OK; this makes sense @@ +52,5 @@ > + > + // Do a shortcut check on the URI and skip the JNI call, if possible. > + if (IsRecentlyVisitedURI(aURI)) { > + NotifyVisited(aURI); > + return NS_OK; Is this much of a win? I'm thinking that a very small percentage of URIs will hit this fast path, and for the rest we're just adding overhead of iterating through the list of recently visited URIs to each call.

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #1) > > + // Do a shortcut check on the URI and skip the JNI call, if possible. > > + if (IsRecentlyVisitedURI(aURI)) { > > + NotifyVisited(aURI); > > + return NS_OK; > > Is this much of a win? I'm thinking that a very small percentage of URIs > will hit this fast path, and for the rest we're just adding overhead of > iterating through the list of recently visited URIs to each call. I agree that the small number of URIs in the list make it less than ideal, but for pages such as Wikis, where the previous page is shown in a breadcrumb link, or for homepage links shown on articles, it might be worth it. This is the one quick check desktop does too: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/History.cpp#459 Although, mEmbedVisit holds 128, not 8 URIs.

Measure it, document the tipping point, add a test to make the assumption brittle. And if IsRecentlyVisitedURI is expensive, perhaps we should make it less so?

Based on nytimes and cnn pageloads, neither quick check makes a lot of difference. There are a few CanAddURI blocks, mainly due to "javascript:" links. I saw around 10 to 15 IsRecentlyVisitedURI blocks per pageload on subsequent loads off of a main page, like on linked article posts. If we have reservations about the IsRecentlyVisitedURI check, I can remove it.

Now with only the CanAddURI check

Since we use IsRecentlyVisitedURI for other purposes, I looked at making it faster. Two simple changes: * Cache the length * Return early if we find a matching URI

The for-loop already kicks out early if we find an equal URI

https://hg.mozilla.org/integration/fx-team/rev/3594f20c57ca https://hg.mozilla.org/integration/fx-team/rev/8c72e71051d8

https://hg.mozilla.org/mozilla-central/rev/8c72e71051d8 https://hg.mozilla.org/mozilla-central/rev/3594f20c57ca

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #9) > https://hg.mozilla.org/mozilla-central/rev/3594f20c57ca I noticed that this changeset has cruft in it from a separate patch in my queue. I am backing out the bad part.

Back out the BrowserToolbar.java part https://hg.mozilla.org/integration/fx-team/rev/ba1c65720453

Looks like the BrowserToolbar.java part was responsible for the Ts Paint improvement: http://graphs.mozilla.org/graph.html#tests=[[83,64,29]]&sel=1388248588482,1388853388482&displayrange=7&datatype=running This means the spinning throbber was hurting Ts Paint. More reason to move ahead with bug 917896.

mfinkle, I saw a significant improvement in throbber stop times (especially on the older devices) on 2014-01-02. https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=098c85bfc598&tochange=574bc211308b See http://phonedash.mozilla.org/#/org.mozilla.fennec/throbberstop/local-blank/2014-01-02/2014-01-04/notcached/noerrorbars/standarderror and cycle through the tests for the full picture. Is this you?

(In reply to Bob Clary [:bc:] from comment #13) > See > http://phonedash.mozilla.org/#/org.mozilla.fennec/throbberstop/local-blank/ > 2014-01-02/2014-01-04/notcached/noerrorbars/standarderror > > and cycle through the tests for the full picture. Is this you? Yes, that's from this patch, but sadly, it's the part I had to back out. Bug 917896 should give us the improvements again.