Closed Bug 924968 Opened 8 years ago Closed 8 years ago
Pinning, unpinning, and editing top sites thumbnails are slow
1) Open about:home 2) Long press a thumbnail on the Top Sites screen 3) Click "Pin Site" Expected: The site is pinned and the page is responsive Actual: The site is pinned but about:home freezes for several seconds (usual ~3 on my device though I've gotten ANRs before) Selecting "Edit" shows similar behavior, and seems to freeze for longer sometimes. "Unpin Site" is also slow. Note that this is on my personal (synced) profile which probably has an average usage. This is somewhat better with today's Nightly (previously, I was running maybe a week-old version), though it would be good to get a consensus on performance across devices.
Brian - Can you look at profiling this?
Assignee: nobody → bnicholson
tracking-fennec: ? → 26+
Most likely caused by the fact that we refresh the list (i.e. re-run the top sites query) when you unpin/pin/remove something in the grid.
With the latest Nightly on my old Samsung Galaxy S, with a pretty big profile synced, pinning/unpinning takes about 1 second. On newer devices (e.g., HTC One), pinning/unpinning takes less than half a second. Michael, you said the lag is "somewhat better" on the latest Nightly -- are you still seeing times >1s? What phone are you using? I think bug 919768 helped with most of the lag from comment 0. IMO, the more noticeable issue is that the thumbnails shift around whenever pinning/unpinning.
> Michael, you said the lag is "somewhat better" on the latest Nightly -- are you > still seeing times >1s? What phone are you using? The whole process is variable but generally seems to take somewhere between 1 and 2 seconds. I'm using a Galaxy Nexus. > IMO, the more noticeable issue is that the thumbnails shift around whenever pinning/unpinning. This is much more noticeable now (for reference, the issue is bug 919234).
Hmm.. In the old version of this I basically didn't refresh the adapter. When about:home was recreated the next time, it would update, but at the moment it felt better to just show the user what they expected. Animations for thumbnails sliding around would also have helped.
I'd like to see bug 919234 fixed before looking at this one since this may have the same cause.
Depends on: 919234
Brian, bug 926574 which probably fixes bug 919234 has landed some days ago. Could you re-evaluate the perf situation with these changes?
Hmm, pinning is still quite slow on my phone. It's not unbearable (about 1 second), but definitely slower than it should be. I'll look into this again.
Some benchmarks: on an HTC One with 2500 history entries, it takes about 300-400ms to execute pinSite/unpinSite. It then takes about 400-500ms to query the top sites list. One bug I found is that we query the top sites twice after calling pinSite(). This happens because we do a deletion followed by an insertion, both which trigger a cursor requery. So we can improve the performance of pinSite() by suppressing the first notification, which this patch does. In general, though, the slowness from pinning/unpinning/editing is due to the fact that our views are dynamically backed by a cursor. Richard suggests that these views shouldn't be dynamic at all; a single query is sufficient to populate the grid/list, and we can just act on that cached data directly when doing write comands such as pin/unpin/remove. I think I agree with him, though that's a bigger effort that we probably shouldn't block on. On a positive note, bug 919768 has already improved performance a bit since this bug was filed.
(In reply to Brian Nicholson (:bnicholson) from comment #9) > One bug I found is that we query the top sites twice after calling > pinSite(). This happens because we do a deletion followed by an insertion, > both which trigger a cursor requery. So we can improve the performance of > pinSite() by suppressing the first notification, which this patch does. That seems pretty hacky. Why not extend the provider with a notion of pinning, and have it do the work within a transaction, thus only causing one notification (and probably saving 50-100msec anyway by only committing a single transaction)? That is, rather than LocalBrowserDB making two trips through the ContentResolver to say "delete", "insert", let's send a structured "update" and have BrowserProvider do the right thing.
(In reply to Richard Newman [:rnewman] from comment #10) > That seems pretty hacky. Why not extend the provider with a notion of > pinning, and have it do the work within a transaction, thus only causing one > notification (and probably saving 50-100msec anyway by only committing a > single transaction)? > > That is, rather than LocalBrowserDB making two trips through the > ContentResolver to say "delete", "insert", let's send a structured "update" > and have BrowserProvider do the right thing. ...no, that makes too much sense.
Comment on attachment 822684 [details] [diff] [review] Do an update or insert instead of multiple operations when pinning site Review of attachment 822684 [details] [diff] [review]: ----------------------------------------------------------------- This takes care of the 'manual' update-or-insert idiom that was previously in use, but it doesn't implement the logic that was expressed by the initial deletion. That is, if previously I'd pinned Slashdot to position 1, then tried to pin it to position 2, it would be *moved* -- position 1 would be deleted, and position 2 would be updated or inserted. After this patch, position 1 would continue to hold a reference to Slashdot, and now so would position 2. Arguably that's OK -- I don't know why we would force a user to have only a single reference, given that users sometimes want to do weird things like bookmark the same page twice. But perhaps not.
Ian confirmed that comment 12 is the desired behavior. I added a comment in LocalBrowserDB explaining this.
Attachment #823557 - Flags: review?(rnewman) → review+
It's merge day. Let's wait until fx-team is merged to m-c to decide which version this hit!
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 28
Comment on attachment 823557 [details] [diff] [review] Do an update or insert instead of multiple operations when pinning site, v2 [Approval Request Comment] Bug caused by (feature/regressing bug #): new-about-home User impact if declined: pinning sites will be slower Testing completed (on m-c, etc.): m-c Risk to taking this patch (and alternatives if risky): very low risk String or IDL/UUID changes made by this patch: none
Automated merge from beta to b2g26. https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/3389719f188a
Verified as fixed on build: nightly 28.0a1 (11.14.2013). Device: Asus Transformer Tab (Android 4.0.3)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.