Closed
Bug 72923
Opened 24 years ago
Closed 24 years ago
Use new command structure and refactor commands/broadcasters/keys
Categories
(SeaMonkey :: UI Design, defect)
SeaMonkey
UI Design
Tracking
(Not tracked)
VERIFIED
FIXED
mozilla0.9
People
(Reporter: bugzilla, Assigned: bugzilla)
References
()
Details
(Keywords: perf, Whiteboard: test builds available)
Attachments
(5 files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review |
Per bug 71470, change all observes= to command= to take advantage of hyatt's new, more efficient command system.
Comment 1•24 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•24 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•24 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
Assignee | ||
Comment 7•24 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•24 years ago
|
||
Er, the bookmarks panel brings in the broadcasters for all of those actions currently, not the menuitems themselves.
Assignee | ||
Comment 9•24 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•24 years ago
|
||
Assignee | ||
Comment 11•24 years ago
|
||
Assignee | ||
Comment 12•24 years ago
|
||
Assignee | ||
Comment 13•24 years ago
|
||
Assignee | ||
Comment 14•24 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•24 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•24 years ago
|
||
cc'ing leaf, who's going to help get some test builds on ftp once ben reviews (cough)
Assignee | ||
Comment 18•24 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•24 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•24 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
Comment 22•24 years ago
|
||
What is the plan for landing this? Is there a carpool date/time scheduled yet?
Comment 23•24 years ago
|
||
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•24 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•24 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•24 years ago
|
||
Comment 27•24 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•24 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.
Comment 29•24 years ago
|
||
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•24 years ago
|
||
navigator stuff landed, uncc'ing people (that I cc'ed earlier) until we figure out what's happening next.
Assignee | ||
Comment 31•24 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
Closed: 24 years ago
Resolution: --- → FIXED
Updated•20 years ago
|
Product: Core → Mozilla Application Suite
You need to log in
before you can comment on or make changes to this bug.
Description
•