75672 - Optimize controller/command updating when command has no visible UI element.

Status: RESOLVED INACTIVE

(Core :: XUL, enhancement)

Description

[Mark Hammond [:markh] [:mhammond]]

From a mail conversation with Hyatt:
--- markh ---
The current way the world works is this:

* People create commands and place them in commandsets.  These commandsets 
nominate "events" that may change their state.

* People define nsIControllers to update the comments.

* At certain times, these "events" are triggered - either automatically 
(like "focus") or manually by the user [using window.updateCommands('event')]

* On these events we find all commandsets that nominated this event, for each 
command find its controller, then update the command.

The problem Komodo has is that we have around 150 different commands.  Many of 
these are basic editor commands.  We have optimized the commandsets heavily, 
but often find outselves updating 50 or so commands for simple focus switching, 
for example.  All 150 are updated as the product starts.

When commandsets are implemented in javascript, the penalty for updating 
commandsets is significant.

I'm wondering if somehow we could move to a model that only updates each 
command as its state is actually needed.  Something like:

* Still define commands and commandsets that nominate events.

* Still define nsIControllers in the same way.

* As each event is triggered, find each command that could be updated, and set 
it's enabled state to "unknown".

* Whenever UI code needs the "enabled" state, instead of looking at 
the "enabled" tag, it calls a function to determine the state.  If the state 
is "unknown", we then find the controller and update only that command.

* We cache the state until it is next set to "unknown".  Thus only the first UI 
element to need the state takes the penalty.

It seems to me that this could offer significant performance benefits. 

-- From Dave --
It seems like what you really want to know is whether or not there's a 
visible UI widget that is currently observing a command.  If there is, 
your command updater must deal with that command, but if not, that 
command need not be updated during the command updating phase and could 
simply be removed from any command updating routines. 

So a command should automatically know if it has visible UI on it 
without you havingto specify anything.  That would just involve caching 
a little bit of statewith the command.  Then the only remaining step is 
to augment the key and menu C++ code to call some new routine like 
UpdateCommand on the controller if it sees this attribute set.

File a bug on me.

dave
([e-mail])

Comment 1

[David Ascher]

hyatt: any way we can help w/ this patch?

Comment 2

[Mark Hammond [:markh] [:mhammond]]

I've been playing with this a little.  I have a patch that adds 
a "hasBroadcastListener()" method to a XULElement.

This allows the commandset updater to skip any commands without visible 
elements.

What this means however is that the menu and key handlers must explicitly 
handle this command.  For example, the menu must force a check of the enabled 
state as the frame is created as menu items can be skipped with the above check.

However, I am stuck working out how to make only _one_ command get updated.  I 
can send a NS_XUL_COMMAND_UPDATE event, but this still causes the entire 
commandset to be refreshed rather than the specific command.  Indeed, there 
appears to be no mechanism for updating a single command at all - ie, will we 
need to allow 

<command oncommand="whatever()" onupdatecommand="be_smart()"/>

? So now I must patiently wait for (or politely harass :) Hyatt to offer some 
more guidance.

Comment 3

[Mark Hammond [:markh] [:mhammond]]

Attaching a possible patch for this.

Notes:
* The menu handling code attempts to find the controller, then sets 
the "disabled" attribute on the command depending on the result of the 
controller call.  The existing code remains unchanged, which will re-read 
the "disabled" attribute.  It would be faster to ignore the 
controller's "disabled" attribute alltogether, but I thought I would get some 
comments first.

* The key handling code does _not_ do this - if a controller is found, 
the "disabled" attribute is ignored.  

* A new |nsIDOMXULElement| method |boolean                   
hasBroadcastListener(in DOMString attr);| has been added.  '*' is supported for 
the attribute name, which is the only semantic required for this particular 
optimization.  This, the 'attr' param could be dropped all-together - but this 
seemed a better fit with the existing interface.

Comment 4

[Mark Hammond [:markh] [:mhammond]]

Attachment: Possible patch - works, but requires some "style" comments from Hyatt.

Comment 5

[Peter Trudelle]

->moz1.0

Comment 6

[Cathleen]

same here...
hyatt, how much will this help window open and what's the scope for fixing this?
-- thanks!  :-)

Comment 7

[Mark Hammond [:markh] [:mhammond]]

The patch I submittted seem to work fine - however, it does not really play 
well with "checked" items.  nsIController does not attempt to work 
with "checked" items, but even still, for ActiveState's Komodo we find that it 
does not work as well in practice as it should.

I would really like to start a dialog with Hyatt on this, but getting his 
attention is pretty hard at the moment :)

Comment 8

[Blake Ross]

Hyatt, I was looking at the command updater code recently and noticed a targets 
attribute that isn't currently used anywhere.  If I understand correctly, this 
is a filter to specify that the commands are only updated when certain elements 
receieve the |events|, as opposed to *all* the elements.  This could be a great 
help in places like msg compose, where I found that we update tons of commands 
when switching focus between the address and subject fields, even though the 
body isn't even involved.

The other topic worth noting is updating commands that are in disabled 
toplevel menus.  In bug 89624 I have a prototype that makes focusing the msg 
compose body instanteous after the first time.  It reduces the number of 
commands we have to update when focusing the body from like 50 to 6.  We're able 
to do this in part because we don't have to update commands that have no visible 
toolbar equivalent and are in top level menus that are disabled.  (Of course, 
this is an ideal case because the msg compose window has few accelerators, and 
the only condition for their enabled-ness is whether the body is focused).

Comment 9

[Intermittent Failures Robot]

1 failures in 2041 pushes (0.0 failures/push) were associated with this bug in the last 7 days.

Repository breakdown:

• try: 1

Platform and build breakdown:

• macosx1015-64-devedition-qr: 1
  • opt: 1

For more details, see:
https://treeherder.mozilla.org/intermittent-failures/bugdetails?bug=75672&startday=2022-09-26&endday=2022-10-02&tree=all