Closed
Bug 955870
Opened 11 years ago
Closed 11 years ago
Do some quick checks before calling CheckURIVisited
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 29
People
(Reporter: mfinkle, Assigned: mfinkle)
References
Details
Attachments
(2 files, 2 obsolete files)
(deleted),
patch
|
blassey
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
blassey
:
review+
|
Details | Diff | Splinter Review |
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.
Attachment #8355013 -
Flags: review?(blassey.bugs)
Updated•11 years ago
|
OS: Linux → Android
Hardware: x86_64 → All
Comment 1•11 years ago
|
||
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.
Attachment #8355013 -
Flags: review?(blassey.bugs) → review-
Assignee | ||
Comment 2•11 years ago
|
||
(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.
Comment 3•11 years ago
|
||
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?
Assignee | ||
Comment 4•11 years ago
|
||
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.
Assignee | ||
Comment 5•11 years ago
|
||
Now with only the CanAddURI check
Assignee: nobody → mark.finkle
Attachment #8355213 -
Flags: review?(blassey.bugs)
Assignee | ||
Comment 6•11 years ago
|
||
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
Attachment #8355013 -
Attachment is obsolete: true
Attachment #8355214 -
Flags: review?(blassey.bugs)
Assignee | ||
Comment 7•11 years ago
|
||
The for-loop already kicks out early if we find an equal URI
Attachment #8355214 -
Attachment is obsolete: true
Attachment #8355214 -
Flags: review?(blassey.bugs)
Attachment #8355236 -
Flags: review?(blassey.bugs)
Updated•11 years ago
|
Attachment #8355213 -
Flags: review?(blassey.bugs) → review+
Updated•11 years ago
|
Attachment #8355236 -
Flags: review?(blassey.bugs) → review+
Assignee | ||
Comment 8•11 years ago
|
||
Comment 9•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/8c72e71051d8
https://hg.mozilla.org/mozilla-central/rev/3594f20c57ca
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
Assignee | ||
Comment 10•11 years ago
|
||
(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.
Assignee | ||
Comment 11•11 years ago
|
||
Back out the BrowserToolbar.java part
https://hg.mozilla.org/integration/fx-team/rev/ba1c65720453
Assignee | ||
Comment 12•11 years ago
|
||
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.
Comment 13•11 years ago
|
||
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?
Assignee | ||
Comment 14•11 years ago
|
||
(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.
Comment 15•11 years ago
|
||
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•