404232 - goUpdateGlobalEditMenuItems caller optimization

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect, P2)

Description

[Myk Melez [:myk] [@mykmelez]]

Attachment: work in progress

goUpdateGlobalEditMenuItems takes up a lot of time in a Tp run profiled with js_calltime.d.

Count,
   editMenuOverlay.js   func       goUpdateGlobalEditMenuItems          1188
   globalOverlay.js     func       goUpdateCommand                      9504
Exclusive function elapsed times (us),
   editMenuOverlay.js   func       goUpdateGlobalEditMenuItems        326525
   globalOverlay.js     func       goUpdateCommand                   1248300
Inclusive function elapsed times (us),
   globalOverlay.js     func       goUpdateCommand                   7256719
   editMenuOverlay.js   func       goUpdateGlobalEditMenuItems       8122288

real	9m58.864s
user	5m36.356s
sys	2m2.856s

The time is being spent calling goUpdateCommand for all the commands in the Edit menu every time the focus or select changes, which is presumably at least once for every page the browser loads, plus some number of times on pages that set the focus to f.e. a search field.

This is something of a shot in the dark, but I wonder if it's really necessary to update the command status so frequently.  After all, goDoCommand asks the controller if the command is enabled before running it, so it's not like a stale status will cause the application to try to execute a disabled command.

In theory, it should only be necessary to update the status when some UI related to the commands becomes visible (f.e. onpopupshowing for the Edit menu) as long as we enable all the commands once the UI disappears again (f.e. onpopuphiding for the Edit menu) so keyboard shortcuts work.

The context menu already does this, as far as I can tell, calling goUpdateGlobalEditMenuItems as part of its initialization (from nsContextMenu.constructor > initMenu > initItems > initClipboardItems).

The only complication is Edit-related toolbar buttons (i.e. the Cut, Copy, and Paste buttons <http://mxr.mozilla.org/mozilla/source/browser/base/content/browser.xul#380> that are hidden by default).  If those are visible, then we do have to update them whenever focus or select changes.

Here's an experimental patch that updates the commands in Firefox only when the Edit or context menus are being shown.  It doesn't deal with the toolbar buttons yet or with other toolkit consumers.

With this patch applied, goUpdateGlobalEditMenuItems doesn't even register in the Tp run, which finishes a bit sooner (but these numbers are probably not reliable):

real	9m52.359s
user	5m34.657s
sys	1m59.727s

Comment 1

[Mike Schroepfer]

Moving this to the blocking list since it is a known target for perf opt

Comment 2

[Mike Schroepfer]

Myk is this something you can help out with?

Comment 3

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #2)
> Myk is this something you can help out with?

Yes, I've been thinking about this, and I think I (mostly) know now how to work around the complication of the Edit-related toolbar buttons and make the fix for Firefox without busting other toolkit applications (so folks can fix those applications out of band instead of us having to submit a single uberpatch that needs to be tested across all of them).

The only thing I haven't figured out yet is how to get this working on Mac.  But perhaps the initial version can be ifdefed out for Mac until I corral some Mac guys into helping me figure it out there.

Comment 4

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v1: only update Edit commands if edit UI is visible

Here's a patch that adds some infrastructure for detecting whether or not edit UI is visible and only updating the edit commands if it is.  Specifically, it checks whether the Edit menu is being shown, the context menu is being shown, or the toolbar has been customized to show the Cut, Copy, and Paste buttons.

If some edit UI is visible, the code updates the edit commands on every change in focus/selection, just as before.  Otherwise, it lazily determines command state when a user presses the applicable keyboard shortcut.

The code is also written so that it doesn't hork other apps using the toolkit Edit menu overlay (Thunderbird, SeaMonkey, etc.).

The changes are ifdefed out for Mac OS X because Mac OS X flashes the Edit menu when you press a keyboard shortcut for an edit command before we get a chance to lazily determine command state, so these changes would cause the menu to flash even when you were pressing a shortcut for a disabled command.

There's probably some way around this in the Mac widget code, but in the meantime, this fix makes things better on Windows and Linux.

Specifically, the time spent in goUpdateGlobalEditMenuItems goes from about eight seconds without the patch to about 0.1 seconds with the patch on the Tp test when run on my laptop.

Before:
   editMenuOverlay.js   func       goUpdateGlobalEditMenuItems       8168842

After:
   editMenuOverlay.js   func       goUpdateGlobalEditMenuItems        112223

That's about a 2% win based on the super-rough numbers my laptop generates (6-7 minutes to run through the test), although the numbers are too inconsistent to be reliable.

Note: for the purposes of measuring the difference, I removed the ifdefs that disable the new code on Mac so I could use DTrace to measure it there.  The patch I profiled was otherwise the same.

Comment 5

[Mike Connor [:mconnor]]

Comment on attachment 297139 [details] [diff] [review]
patch v1: only update Edit commands if edit UI is visible

Looks good, seems like a pretty sane win.

Comment 6

[Myk Melez [:myk] [@mykmelez]]

Checking in browser/base/content/browser.xul;
/cvsroot/mozilla/browser/base/content/browser.xul,v  <--  browser.xul
new revision: 1.407; previous revision: 1.406
done
Checking in browser/base/content/browser.js;
/cvsroot/mozilla/browser/base/content/browser.js,v  <--  browser.js
new revision: 1.936; previous revision: 1.935
done
Checking in browser/base/content/browser-menubar.inc;
/cvsroot/mozilla/browser/base/content/browser-menubar.inc,v  <--  browser-menubar.inc
new revision: 1.128; previous revision: 1.127
done
Checking in toolkit/content/editMenuOverlay.js;
/cvsroot/mozilla/toolkit/content/editMenuOverlay.js,v  <--  editMenuOverlay.js
new revision: 1.3; previous revision: 1.2
done

Comment 7

[Steve England [:stevee]]

It's my guess this caused bug 414131

Comment 8

[Myk Melez [:myk] [@mykmelez]]

Attachment: post-checkin perf graphs

Strangely, this doesn't seem to have had an effect on pageload.

Comment 9

[Myk Melez [:myk] [@mykmelez]]

Note: I loaded those graphs by going to:

http://tinderbox.mozilla.org/showbuilds.cgi?tree=Firefox&maxdate=1201340610&hours=24&legend=0&norules=1

... and then clicking the Tp2, Tp2, and Tdhtml links for the build *before* the build incorporating my changes (which were checked in at 21:46:34):

http://build-graphs.mozilla.org/graph/query.cgi?testname=pageload&units=ms&tbox=bl-bldxp01_HEAD&autoscale=1&days=7&avg=1&showpoint=2008:01:25:22:25:43,446
http://build-graphs.mozilla.org/graph/query.cgi?testname=pageload2&units=ms&tbox=bl-bldxp01_HEAD&autoscale=1&days=7&avg=1&showpoint=2008:01:25:22:28:54,362.45
http://build-graphs.mozilla.org/graph/query.cgi?testname=dhtml&units=ms&tbox=bl-bldxp01_HEAD&autoscale=1&days=7&avg=1&showpoint=2008:01:25:22:31:54,1110

http://build-graphs.mozilla.org/graph/query.cgi?testname=pageload&units=ms&tbox=bl-bldlnx03_fx-linux-tbox-HEAD&autoscale=1&days=7&avg=1&showpoint=2008:01:25:22:05:16,249
http://build-graphs.mozilla.org/graph/query.cgi?testname=pageload2&units=ms&tbox=bl-bldlnx03_fx-linux-tbox-HEAD&autoscale=1&days=7&avg=1&showpoint=2008:01:25:22:07:46,181.775
http://build-graphs.mozilla.org/graph/query.cgi?testname=dhtml&units=ms&tbox=bl-bldlnx03_fx-linux-tbox-HEAD&autoscale=1&days=7&avg=1&showpoint=2008:01:25:22:09:50,627