924968 - Pinning, unpinning, and editing top sites thumbnails are slow

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Logcat dump during slow down

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.

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

Brian - Can you look at profiling this?

Comment 2

[Lucas Rocha (:lucasr)]

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.

Comment 3

[Brian Nicholson (:bnicholson)]

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.

Comment 4

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

> 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).

Comment 5

[Wesley Johnston (:wesj)]

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.

Comment 6

[Brian Nicholson (:bnicholson)]

I'd like to see bug 919234 fixed before looking at this one since this may have the same cause.

Comment 7

[Lucas Rocha (:lucasr)]

Brian, bug 926574 which probably fixes bug 919234 has landed some days ago. Could you re-evaluate the perf situation with these changes?

Comment 8

[Brian Nicholson (:bnicholson)]

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.

Comment 9

[Brian Nicholson (:bnicholson)]

Attachment: Don't send superfluous content observer update in LocalBrowserDB#pinSite

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.

Comment 10

[Richard Newman [:rnewman]]

(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.

Comment 11

[Brian Nicholson (:bnicholson)]

Attachment: Do an update or insert instead of multiple operations when pinning site

(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 12

[Richard Newman [:rnewman]]

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.

Comment 13

[Brian Nicholson (:bnicholson)]

Attachment: Do an update or insert instead of multiple operations when pinning site, v2

Ian confirmed that comment 12 is the desired behavior. I added a comment in LocalBrowserDB explaining this.

Comment 14

[Brian Nicholson (:bnicholson)]

https://hg.mozilla.org/integration/fx-team/rev/7381dc390a7e

Comment 15

[Richard Newman [:rnewman]]

It's merge day. Let's wait until fx-team is merged to m-c to decide which version this hit!

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7381dc390a7e

Comment 17

[Brian Nicholson (:bnicholson)]

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

Comment 18

[(PTO June 19-July 7) Ryan VanderMeulen]

https://hg.mozilla.org/releases/mozilla-aurora/rev/53af1176a62e
https://hg.mozilla.org/releases/mozilla-beta/rev/3389719f188a

Comment 19

[(PTO June 19-July 7) Ryan VanderMeulen]

Automated merge from beta to b2g26.
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/3389719f188a

Comment 20

[Flaviu Cos]

Verified as fixed on build: nightly 28.0a1 (11.14.2013).
Device: Asus Transformer Tab (Android 4.0.3)