Use new command structure and refactor commands/broadcasters/keys

VERIFIED FIXED in mozilla0.9

Status

defect
VERIFIED FIXED
18 years ago
13 years ago

People

(Reporter: bugzilla, Assigned: bugzilla)

Tracking

({perf})

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: test builds available, )

Attachments

(5 attachments)

Assignee

Description

18 years ago
Per bug 71470, change all observes= to command= to take advantage of hyatt's 
new, more efficient command system.

Comment 1

18 years ago
Note, don't convert *all* of them.  Only convert those instances where the
observes="" is clearly being used to observe a command.  For any menuitem or key
element, this is probably the case.

Blocks: 70753
Assignee

Comment 2

18 years ago
Hyatt, I'm a little confused.  The example you give in 71470 converts elements 
that are observing a broadcaster.  Has the fate of broadcasters been decided?
Assignee

Comment 3

18 years ago
Or are you saying just to make sure that the elements are observing 
broadcasters or commands with oncommand handlers?
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9
i want to be in on this just in case anything goes wrong on macos.
(implying i don't trust hyatt's underlying code) ;)

Updated

18 years ago
Depends on: 71470

Comment 6

18 years ago
Blake, right.

Assignee

Comment 7

18 years ago
Work here involves:

  - Using new command structure (broadcaster/observes -> command/command)
  - Removing all the extraneous attributes on broadcasters that we have,
    putting them where they belong (usually on menuitems)
  - Stop being lazy and pulling in large collections of unused nodes, just
    for the convenience of being able to say, e.g.
    <broadcasterset id="broadcasterset"/> (for example, the bookmarks panel
    currently brings in task menuitems, page/print setup menuitems,
    new editor/navigator menuitems, etc.)

All of these should have a positive impact on performance.  Just began to do 
step 1 in navigator, and speedup is already noticeable (early quantify runs are 
also promising).
Keywords: perf
Summary: Convert all instances of observes= to command= → Use new command structure and refactor commands/broadcasters/keys
Assignee

Comment 8

18 years ago
Er, the bookmarks panel brings in the broadcasters for all of those actions 
currently, not the menuitems themselves.
Assignee

Comment 9

18 years ago
DOM trees show that many windows are bringing in far too many extraneous nodes 
for the sake of convenience, so at this point I'm eliminating all of the 
<keyset id="keyset"/> / <broadcasterset id="broadcasterset"/> craziness and 
making clients explicitly ask for specific nodes.  This, in combination with 
the switch to the new command structure, the moving of attributes to where they 
really belong, and the arbitrary cleaning up of attributes (e.g. removing id's 
from many elements that don't require them) has produced some extremely 
promising results.

I'm about 80% done with Navigator, and here are some nice results I'm seeing 
for the opening of a second Navigator window (comparison between two tip 
builds, one containing only these Navigator changes):

Attribute setting (nsXULElement::SetAttribute) is down from 3,627 to 2,015 
(and, largely as a result, calls to nsXULDocument::GetScriptGlobalObject -- 
which are cheap --  are down from 4,296 to 2,725).

Calls to XULBroadcastListener::ObservingEverything/Attribute are down from 481 
to 8.

Calls to nsXULElement::NodeInfo are split in half, from 32,721 to 16,194.

Calls to StyleSetImpl::FindPrimaryFrameFor are down from 2,421 to 525.

Not that any of these are significantly expensive, but there are many such 
notable cuts in other areas.  The nice thing is that Navigator doesn't use 
broadcasters/commands extensively; I look forward to results from editor and 
mail, which do.
Assignee

Comment 10

18 years ago
Assignee

Comment 12

18 years ago
Assignee

Comment 13

18 years ago
Assignee

Comment 14

18 years ago
So here's the plan: uh, I'm never checking anything in again without a 
carpool ;)  I'd like to land this in a mini-carpool and get some nightlies out 
first for some people to test, since this covers such a wide range of things.  
I've heard mailnews wants to do their own, so this patch only really touches 
the bare minimum that had to be done for it (as a result of touching the 
overlays).

Looking for r/sr from ben (emailed) and hyatt.

cc'ing the world so no one can complain that they weren't aware of this 
change ;)

Comment 15

18 years ago
sr=hyatt.  Land as a carpool, and send email directly to notify some people who 
might not read their bugmail (Simon, Prass, Mscott, etc.)
Assignee

Comment 16

18 years ago
cc'ing leaf, who's going to help get some test builds on ftp once ben reviews 
(cough)
Why the heck was I not cc'ed on this? Bad boys!
Assignee

Comment 18

18 years ago
r=ben, with the note that this patch contains other cleanup work which will 
also help performance.

cc'ing the rest of the world to keep everyone apprised; people who complain 
after this landing will be silenced :P

Comment 19

18 years ago
I notice that in xpfe/components/prefwindow/resources you modified the following 
files:

   pref-cookies.xul
   pref-cookies.dtd
   pref-images.xul
   pref-images.dtd
   pref-wallet.xul
   pref-wallet.dtd
   pref-passwords.xul
   pref-passwords.dtd

Unfortunately these files are obsolete and not part of the build.  They have 
been replaced by files of the same number in either extensions/cookie/resources 
or extensions/wallet/resources.  But these files are not included in your patch.  
They should be.  That is, the change you were making to the xpfe versions should 
be made to these extensions versions instead.

Comment 20

18 years ago
Oops, above I said:

   replaced by files of the same number

whereas I meant to say

   replaced by files of the same name
Steve: why haven't the obsolete files been cvs removed?

/be

Comment 22

18 years ago
What is the plan for landing this?  Is there a carpool date/time scheduled yet?
Once we get scc's string changes put into test builds and landed, i can spin 
test bits with these changes for testing, and we can then schedule a landing (my 
guess is tuesday or wednesday, depending on scc's landing.

Comment 24

18 years ago
Can we create test bits for commercial as well? That we way the aim changes can 
also land at the same time this gets landed in mozilla.

Comment 25

18 years ago
Jason has volunteered to create commercial optimized builds. Imqa will run some 
tests on it with Blake's patch and my patch. Ping Jason if you want to try the 
test builds.
Assignee

Comment 26

18 years ago
We have tests builds available at 
http://ftp.mozilla.org/pub/mozilla/nightly/latest-72923 for linux and windows
Whiteboard: test builds available

Comment 28

18 years ago
Mac test build is up!
http://ftp.mozilla.org/pub/mozilla/nightly/latest-72923
or
http://ftp.mozilla.org/pub/mozilla/nightly/2001-04-04-14-72923

Note that the patch to mozilla/mailnews/compose/prefs/resources/content/pref-
messages.xul had to be removed from the most recent patch attached because this 
file (pref-messages.xul) is no longer pertinent.
blake--Leme take your head! You landed something in bugzilla! http://
bonsai.mozilla.org/cvsquery.cgi?dir=mozilla/webtools/bugzilla/docs/
sgml&date=explicit&mindate=04/05/2001&maxdate=04/06/2001
Assignee

Comment 30

18 years ago
navigator stuff landed, uncc'ing people (that I cc'ed earlier) until we figure 
out what's happening next.
Assignee

Comment 31

18 years ago
Sick and tired of hearing complaints from everyone (especially a certain 
reviewer, who began complaining about the patch after-the-fact because he was 
too lazy to actually look at any of it).  Closing this for the work done on 
Navigator, and MailNews and Editor can feel free to take advantage of this ~7% 
performance boost on their own if they want.
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED
okay.
Status: RESOLVED → VERIFIED
Product: Core → Mozilla Application Suite
You need to log in before you can comment on or make changes to this bug.