832641 - Menu items slow to paint/respond after peeking their sub-menu popups

Status: RESOLVED FIXED

(Core :: Widget, defect)

Description

[Avi Halachmi (:avih)]

This happens in Firefox 18 and current nightly. Tested and reproduced on Windows (HW accel either on or off) and Linux (Xubuntu 12.10 fully updated).

Steps to reproduce:
1. Restart the browser, make sure the menu is visible, have many (30 at least) bookmarks folders at the bookmarks menu.
2. Hover all the main menu items with mouse, note that it responds fast with highlight (on Linux - click one of the main menu items first such that it responds to hovering different top menu items).
3. Click the bookmarks menu, hover each folder such that the subfolder popup shows.
4. Collapse the bookmarks menu, hover all the main menu items one after the other again.

Expected result:
- Main menu items respond quickly with highlight.

Actual result:
- The bookmarks menu is very slow to highlight or respond (could easily get over 1 second).

- This also happens with bookmark toolbar folders.


Other symptoms of this issue:
- Resizing becomes very slow on windows, as apparently the menu is repainted on each resize paint.

Workaround:
- Hide the menu (or bookmarks toolbar) and show it again -> all menu items respond fast again, window resizing is fast again.


Observations:
- On windows, I noticed that each subfolder peek adds few GDI objects which are never released.
- If a subfolder was already peeked before, it doesn't add GDI handles, even if the menu was collapsed after peeking and then peeked again.
- I suspect (though haven't tested) that livemarks accumulate this issue each time they're updated and peeked.
- Looking at the profiler after hovering slow main menu items, it appears that the callstack is in Paint most of the time, probably of the collapsed subfolder popups.

Personally, I restarted Firefox many times due to degrading responsiveness over time. I think this could be one of the reasons (if not the main ones) for this issue. This might be related to the fact that I use livemarks regularly.

Comment 1

[Avi Halachmi (:avih)]

I see that nsMenuFrame (and nsMenuTimerMediator) are constructed per menu item (and sub menu item etc) on the first time the item is displayed, but are not destructed when the menu collapses (the exception would be "Live" menus, like History and recently visited, whose items are destructed the next time the menu is expanded, just before it re-populates with updated items).

This is probably by design to save construction at the next time menu items are displayed.

So the bug could be that all the sub-items (recursively) are possibly repainted/composed when painting the top menu item, even when the items themselves are invisible while the menu is collapsed.

The bug description might be better as: "Top menu items slow to paint/respond after displaying their sub-items (recursively) - the more the merrier".

Comment 2

[Vladan Djeric (:vladan)]

Is this related to bug 820247?

Comment 3

[Avi Halachmi (:avih)]

(In reply to Vladan Djeric (:vladan) from comment #2)
> Is this related to bug 820247?

Yes, and bug 794246 (fix isn't complete apparently), bug 807581, bug 812904, and bug 830697.

Some of those mention the degradation over time (due to new items displaying, possibly livemarks). Some mention it's fixed with HW accel off (it's faster without HW accel, but the basic issue still exists).

This seems to describe the issue, though I'm not sure from this description if every hidden-but-previously-visible item should be repainted even when the entire menu is collapsed:

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from bug 794246 comment #14)
> (In reply to Girish Sharma [:Optimizer] from comment #12)
> > Even while using the default theme ?
> 
> Yes. The problem is that there is not reliable way to determine that the
> current theme isn't going to draw anything, so we have to do a lot of work
> to set things up and ask the theme to draw even if it doesn't end up drawing
> anything.

Possible fix, at least if the issue raised by roc is hard to solve, is to treat the bookmark menu/toolbar similar to the history/recently-visited items, by destructing items, and repopulate the menu when required.

It's not exactly the same though, because on the history menu the items are kept in mem and only deleted when new items replace them (when opening the menu). Using the same strategy for bookmarks will not solve this issue since many items will be kept in mem - The items need to be deleted when the popup closes IMO.

Comment 4

[Alice0775 White]

I can reproduce in Firefox18+ but not in Firefox17.

Regression window(m-c)
Good:
http://hg.mozilla.org/mozilla-central/rev/360ab7771e27
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0 ID:20120820140803
Bad:
http://hg.mozilla.org/mozilla-central/rev/f9a8fdb08193
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0 ID:20120821092158
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=360ab7771e27&tochange=f9a8fdb08193

Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/06ea22b8654c
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0 ID:20120820013102
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/1f6cd8529ae9
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0 ID:20120820030143
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=06ea22b8654c&tochange=1f6cd8529ae9



And the problem is fixed in Aurora17.0a2 but not in mozilla-central.
I.e., This problem still exist in Firefox18+.

Fixed window(aurora channel)
Bad:
http://hg.mozilla.org/releases/mozilla-aurora/rev/7328213574c2
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0 ID:20120917160002
Fixed:
http://hg.mozilla.org/releases/mozilla-aurora/rev/cbb5d4bf1617
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0 ID:20120917171701
Fixed pushlog:
http://hg.mozilla.org/releases/mozilla-aurora/pushloghtml?fromchange=7328213574c2&tochange=cbb5d4bf1617

Comment 5

[Alice0775 White]

FYI
The slowness of the menu expanding resolves temporarily if I carry out the following procedures.
1. View > Toolbars > Customize...
2. Done

Comment 6

[Alice0775 White]

In local build
Last Good: bc76f965c041
First Bad: 1f6cd8529ae9
Triggered by:
1f6cd8529ae9	Matt Woodrow — Bug 783449 - Move android specific NeedsPaint checks into the android widget implementation. r=roc

Comment 7

[Avi Halachmi (:avih)]

I've built 18.01 (cloned http://hg.mozilla.org/releases/mozilla-release/), and reverted the patch from bug 783449 (only at /widget/nsIWidget.h) and can confirm big responsiveness improvement on Windows (menu/bookmarks hovers, window resize - after expanding several bookmark folders with many subitems).

With some usage patterns which I suspect are not uncommon (frequently using live/bookmarks toolbar or menu) Firefox 18.01 quickly becomes apparently unusable. I suggest to promote a fix for this, possibly upto FX18.

With the reverted patch on FX18.01, on Windows:
- Respond is noticeably faster at the relevant scenarios.
- I cannot reproduce with STR at bug 783449 comment #0 (page info popup highlight remains).
- I cannot reproduce with STR at bug 784117 comment #0 (awesomebar duplicate highlight).
- I cannot reproduce with STR at bug 784137 comment #0 (awesomebar highlight remains).
- I also tried collapsing menus while an item was highlighted, and then expand the menu again, and that item was not (erroneously) highlighted.

Possible regressions to reverting the patch of bug 783449, which I didn't test:
- bug 770000 (html5 video repaints too often).
- bug 782980 (black boxes on bottom/right during resize). I did test this one on Windows, but the behavior seems the same before/after the revert (shows white area, as if the resize is async - though still slow as if it's sync). However, the bug is marked with platform=all.

Maybe a promoted patch should only be applied for Windows?

Comment 8

[Avi Halachmi (:avih)]

Oops...

Comment 9

[Avi Halachmi (:avih)]

Also could not reproduce with STR from bug bug 770000. However, my system is relatively fast (i7 3630qm), so the issue might be masked.

Comment 10

[Cameron McCormack (:heycam)]

It is btw not just bookmarks menus.  I also find it laggy when I mouse over tab strip overflow "List all tabs" menu, with maybe 50 menu items.

Comment 11

[Cameron McCormack (:heycam)]

This is on a Mac.

Comment 12

[Mayank Bansal]

any idea why the slowing while hovering mouse over bookmarks button does not occur if the buttons are set to the default size ?

Comment 13

[Alice0775 White]

(In reply to mayankleoboy1 from comment #12)
> any idea why the slowing while hovering mouse over bookmarks button does not
> occur if the buttons are set to the default size ?

Because the bookmark placesView are reset after Customize Toolbar.

Comment 14

[Matt Woodrow (:mattwoodrow)]

Attachment: Revert, and invalidate the popup instead

Thanks for the detailed analysis and regression range here.

The issue is that each submenu is creating a new widget, and we then end up spending large amounts of time processing each of these widgets during paint.

I think the right thing to do here is revert the original change, so that we skip over hidden or 0-sized widgets.

I've also added an alternative fix for the original bug, which just invalidates the entirety of the popup when it becomes visible.

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Please nominate for Aurora and Beta after this has landed.

Comment 16

[Matt Woodrow (:mattwoodrow)]

ttps://hg.mozilla.org/integration/mozilla-inbound/rev/2d5b54f00e2e

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2d5b54f00e2e

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2d5b54f00e2e

Comment 19

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 704692 [details] [diff] [review]
Revert, and invalidate the popup instead

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 783449
User impact if declined: Massive slow downs after a large number of submenus have been viewed.
Testing completed (on m-c, etc.): Landed on m-c
Risk to taking this patch (and alternatives if risky): Low risk.
String or UUID changes made by this patch: None

Comment 20

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 704692 [details] [diff] [review]
Revert, and invalidate the popup instead

approving low risk fix for uplift. please land to beta immediately so it's ready for beta 4.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/13a8dd1531da
https://hg.mozilla.org/releases/mozilla-beta/rev/4e3c61f30a3f

Comment 22

[:Gavin Sharp [email: [e-mail]]]

(In reply to Avi Halachmi (:avih) from comment #3)
> (In reply to Vladan Djeric (:vladan) from comment #2)
> > Is this related to bug 820247?
> 
> Yes, and bug 794246 (fix isn't complete apparently), bug 807581, bug 812904,
> and bug 830697.

Should those (and the other bugs in the dependency tree) be marked fixed too now?

Comment 23

[Avi Halachmi (:avih)]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #22)
> Should those (and the other bugs in the dependency tree) be marked fixed too
> now?

Yes for bug 807581, bug 812904, bug 829563.

Bug 820247: probably yes (mentions also slow tab close which I don't recall as a symptom of this bug, and relates the issue to number of open tabs - which I also don't recall as related).

Bug 830697: I'd say yes, but will want the reporter to confirm. It doesn't states a problem but rather a solution (to a symptom of bug 794246), but it might be valid in its own right (delay widget border paint until its content is ready). The behavior which this bug wants to fix _should_ be gone now, though I couldn't reproduce it in the first place as well.

Comment 24

[Avi Halachmi (:avih)]

(In reply to Avi Halachmi (:avih) from comment #23)
> Bug 830697: I'd say yes, but will want the reporter to confirm.

Girish prefers to keep this one open.

Comment 25

[Avi Halachmi (:avih)]

Oops #2 (Bugzilla tweaks 1.12.1?? I never got close to these flags...).
I couldn't restore the tracking flags.

Comment 26

[(no longer active)]

Can you guys please do the honors for resetting the flags?  I don't have the super powers necessary for the task!  :-)

Comment 28

[Paul Silaghi, QA [:pauly]]

Saw the slowness in FF 19b3. Verified fixed in FF 19b4 Win 7 x64.

Comment 33

[Paul Silaghi, QA [:pauly]]

Verified fixed in FF 20RC Win 7 x64.

Comment 34

[Paul Silaghi, QA [:pauly]]

Verified fixed Mozilla/5.0 (Windows NT 6.1; WOW64; rv:21.0) Gecko/20100101 Firefox/21.0b3