Specifically, if most of the faults from prototype to heavyweight occur on attribute changes, is there a more efficient (in time and space) mechanism to note that an attribute has changed? Analysis at news://news.mozilla.org/3A8AF290.446AA622@netscape.com along with some deeper digging prompted by /be suggests that this might be a win. Specifically, we spend 4% of new window time faulting prototype elements to heavyweights. How many of the occur simply because an attribute's value has changed?

Specifically, in that article, I wrote: There are 3,875 calls to nsXULAttribute::SetValue(), accounting for 4% of the overall time (20msec). I just re-profiled this, and now only see 2,564 calls to SetValue(), accounting for 5msec, and accounting for about 1.5% of the overall time. I'm going to FUTURE this for now, and continue with the mantra ``avoid setting attributes''.

-> shaver

So the Obvious Thing is to just not have EnsureSlots copy over the additional attributes, but allocate enough space for them in mSlots. Then we SetAttr in mSlots, and GetAttr by checking mSlots and then the prototype. Currently, we fault everything in a handful of places, and we'll need to make sure that we still do what's required to maintain consistency at those points: - nsXULElement::Create calls EnsureSlots, but it will only ever allocate mSlots, so we can do that by hand in ::Create. - QueryInterface does it when it aggregates with the inner nsXULTreeElement. Can anyone tell me why, and whether or not it's worth making that code handle multi-level lookup? - GetAttributes faults them all in so it can create an nsXULAttributes structure to reflect them all. We could probably nsXULAttributes about prototype searching, if we wanted to. Otherwise, that can continue to fault everything. Do we know if it's called often? - SetPrefix faults everything so it can change the prefix on everything. That'll probably continue. (But if we're the only child of the prototype, can we just change the prototype's prefix? Is that worth it?) - SetContainingNameSpace is basically just like SetPrefix. (But GetContainingNameSpace doesn't seem to interact with mSlots -- so why do we fault?) - ForceElementToOwnResource faults everything in via EnsureSlots, but it seems that it really only cares about mSlots->mOwnedResource. (And why does it fault at all in the drop-reference case?) - SetAttr faults, but that's what we're talking about here. - UnsetAttr faults, but it would seem that we could store a bitmask of removed- from-the-prototype entries to avoid having to fault everything. - AddBroadcastListeners faults everything in, but it'll do what SetAttr does, I think. (How do prototyped event listeners work? I should investigate that.) - GetControllers faults everything, but only cares about mSlots->mControllers. I think that's it.

Controllers should be pulled out into a map maintained by the document.

What hyatt meant to say was, ``you can keep the controllers in a sparse map in the document, rather than burning a word in each element.'' Also, I don't think we need to ForceElementToOwnResource crap anymore: IIRC that was a hack we did for performance in the mailnews threadpane. I'll look into removing it.

Comment on attachment 51313 [details] [diff] [review] First, non-working cut at partial attribute sharing What I've done so far is make elements be of 4 "weights": - prototype, nothing else - prototype and mSlots (EnsureSlots sets this) - prototype, mSlots and mSlots->mAttributes (EnsureAttributes) - mSlots and mSlots->mAttributes, no proto (MakeHeavyweight) We call EnsureSlots in a bunch of places so that we can store stuff in mSlots, like controllers and new nodeinfos and such. We call EnsureAttributes either when made heavyweight, or when we first set an attribute on the element. We become heavyweight when we enumerate the attributes (don't do that) or unset an attribute (I'll get to that later). This is also the only place where we break prototype linkage. In the future, we'll probably want to become heavyweight when we've changed n values locally. We can argue about n a lot later. I also took out the force-to-own-resource thing, because waterson said I could. (This patch also contains a fix for 101234, the crash when focussing or blurring a document-less element.) Right now I crash while bringing up the browser window (though some things do display!) when we get into NormalizeAttrString from SetAttribute, which appears to be called from the basetext#disabled binding. We have no prototype and no nodeinfo. That's not good. I wasted about two hours before I realized that my tree was built for tracemalloc, which means -g but no DEBUG, ergo no assertions. I'll get off the pipe and try to track down what's going on after dinner, if I don't have too much (or too little) to drink. In the meantime, I'd love to have people tell me more about the XBL element-creation path, so I can try to figure out what part of this beast to meditate on first.

Comment on attachment 51406 [details] [diff] [review] Working (almost) patch This patch lets me bring up browser windows and use them. I'm missing some menu items (window list in Tasks and the Manage/File entries in Bookmarks, for example), which I need to investigate further. As of right now (brought up a window, went to the bug, attached patch, editing this comment), we've made a grand total of 13 XUL elements be heavyweight. I just realized that I'm being really dumb about UnsetAttr, too, in that I only need to be heavyweight if the prototype has the attribute and the local attribute set doesn't. I bet that's pretty rare, since I expect that most of the UnsetAttrs we do are symmetric with a preceding SetAttr. I'll poke at that a little, and I think it'll reduce the faulting that happens for menu use to near-zero. Also, we need to decide if we want to fault elements once they have > n local attributes. I'm not sure it's worth it. If someone wants to do A-B memory testing with this patch, I'd love it. I'll do it later if nobody else gets to it first. Once I get menus working, I'll be looking to get r and sr for checkin, I think.

Comment on attachment 51313 [details] [diff] [review] First, non-working cut at partial attribute sharing Tidying up, sorry for the spam.

Comment on attachment 51601 [details] [diff] [review] the secret rules of engagement are hard to enforce Still a fair ways to go, but I can open mail and read it now. Focus doesn't work. The bookmarks menu is missing the text for the "File"/"Add"/"Manage" entries, probably due to the lord-of-the-dark powers of template. SetDocument s.b. a lot faster now, in addition to handling the partial sharing case. Some icons (like the mail "Stop" button one) don't display. Information leading to the apprehension of these bugs will be greatly appreciated.

Comment on attachment 51406 [details] [diff] [review] Working (almost) patch (More tidying. It makes me feel good about myself.)

This is the top of my list, and I've got a new patch to post shortly (overlays work again). We create a lot of non-prototyped nodes, some from XBL (hyatt wants me to attack that next), and some from other places like GFX scrollbars. I've got a heavily instrumented build here that shows me the stack for every one (ahem), and I'm going to do some trace-malloc-like analysis to see where we can cheapen up. I hate XUL.

Some icons (back, forward) don't appear until they get moused-out once. I bet it's related to commands. I can't type in text fields. Other than that little problem, mail seems to come up OK. There's a _lot_ of debug spew in that patch, but if you're not me you shouldn't see any effects from it.

Comment on attachment 51601 [details] [diff] [review] the secret rules of engagement are hard to enforce tidy, tidy, tidy, it's all I do.

Comment on attachment 52525 [details] [diff] [review] when the appearance of conflict meets the appearance of force Lest anyone suspect otherwise: needs-work.

Comment on attachment 52623 [details] [diff] [review] that's what I'm here for I'm showing a 200K win bringing up the first window with this patch, though there's probably some noise in the trace-malloc data. I'll do some more tests (more windows, mailnews, etc.) later, when I get done making UnsetAttr not wreck everything. (I hate XUL.)

Comment on attachment 52525 [details] [diff] [review] when the appearance of conflict meets the appearance of force OBSOLETE! BEGONE!

Tried the above patch (attachment 52623 [details] [diff] [review]). trace-malloc says that, after opening ten windows, this ought to save us about 6% worth of malloc heap (or about 1MB out of 17MB). ---- Base ---- ---- Incr ---- ----- Difference ---- Type Count Bytes Count Bytes Count Bytes %Total TOTAL 388378 16891616 401600 17978189 13222 1086573 100.00 nsXULAttribute::mValue 5961 372976 10085 679182 4124 306206 28.18 nsXULAttribute 29 356352 48 589824 19 233472 21.49 nsXULAttributes 5172 351696 7538 512584 2366 160888 14.81 nsXULElement 19890 1040216 22225 1149572 2335 109356 10.06 nsImageGTK 107 42412 185 147980 78 105568 9.72 nsRuleNode 11172 318416 12442 353976 1270 35560 3.27 CSSRuleProcessor 28688 671872 29136 700480 448 28608 2.63 nsSupportsHashtable 3654 160776 4211 185284 557 24508 2.26 AtomImpl 3487 99536 3713 114666 226 15130 1.39 nsFontGTK 13 12028 25 19164 12 7136 0.66 StyleSetImpl 875 62936 983 69848 108 6912 0.64 orkin-unclassified 54 87682 54 93298 0 5616 0.52 nsShellISupportsKey 5569 44552 6204 49632 635 5080 0.47 unclassified-string 9317 415545 9354 420367 37 4822 0.44 FrameArena-unclassified 5345 134726 5473 138666 128 3940 0.36 nsStyleBorder 688 66048 729 69984 41 3936 0.36 void* 12541 417244 12858 420760 317 3516 0.32 nsJSEventListener 3598 100744 3708 103824 110 3080 0.28 PRLock 541 47608 575 50600 34 2992 0.28 OTHER 271677 12088251 272054 12108498 377 20247 1.86 The BASE data includes the patch, the INCR data does not, so this is sort of ``backwards'', but that's because this report orders things by largest increase to smallest. Note that I'm seeing some bugs with the patch, including: - Back, forward, print buttons don't have images loaded on first mouseover - Bookmarks menu doesn't show the ``static'' items (Add Bookmark, File Bookmark, etc.) - None of the toolbar buttons come up in mail/news. I'm using the modern skin.

Comment on attachment 52920 [details] test case. save as $(DIST)/bin/chrome/comm/content/communicator/bookmarks/test.xul Frig. Never mind.

Okay, so the above test case should at least illustrate the problem with the bookmarks menu (which are getting their label's set from a broadcaster): 1. Save attachment 52922 [details] as $(DIST)/bin/chrome/comm/content/communicator/bookmarks/test.xul 2. cd $(DIST)/bin/chrome/comm 3. zip ../comm.jar content/communicator/bookmarks/test.xul 4. cd $(DIST)/bin 5. ./mozilla -chrome chrome://communicator/content/bookmarks/test.xul In a stock build, you should see labels set for the bookmarks ``add bookmark'' menu, etc. With the patch from attachment 52623 [details] [diff] [review], I get nothing but sad emptiness.

Waterson's going to finish with the template debugging while I work on some more RSS tools. Breaks my heart, but it's the right thing.

I've fixed a few bugs in the last patch, including... 1. Make sure to do clever attribute checking logic in GetID(). This was what was causing images to not show up until mouseover (style rules didn't match). 2. Make sure to walk prototype attributes in AddBroadcastListener(). This was what was causing the bookmarks menu to not show up correctly. 3. Be sure to check |aNameSpaceID == kNameSpaceID_None|, not just |kNameSpaceID_None| (which happens to be zero, I think!) This was a result of fusing logic in UnsetAttr(). I didn't observe any side effects from this, but it was certainly a latent bug. Also, some other stuff: 4. Removed the mBroadcaster field from the nsXULElement::Slots structure; it wasn't being used. Saves four bytes -- yay! 5. Removed the mIsScriptObjectRooted field from nsXULElement, which was made obsolete by the XPCDOM landing. (This was debug-only, so no real wins here.) 6. Removed a bit of code in nsXULDocument's observer stuff. By counting backwards through the observers, we don't have to do the special check to see if an observer removed itself.

I'm suspicious that this patch may break focus.

The problem with focus had to do with the code factored out into UpdateAttributeForSetDocument(), which I renamed to AddListenerFor() and called from SetDocument(), SetAttr(), and nsXULElement::Create() to avoid copying that heinous attribute checking stuff all over. (I am so ashamed.) I _think_ the last patch is money. Reviews, anyone?

Comment on attachment 53720 [details] [diff] [review] fix problems with focus + // These are the things that we need to have done for an attribute + // when we get a new document That comment's not accurate anymore: we call this code from Create as well as SetDocument. In UnsetAttr, I think we only want to fault if we're actually unsetting a value that's present in the prototype. I have some code in my tree to do that, I'll mail you. + * Lazily instantiated if/when object is mutated. Instantiating + * the mSlots makes an nsXULElement 'heavyweight'. + */ No longer true: having mSlots allocated make us "middleweight", since we still share attributes. Also, I think I can live without the debug-spew, now that we have the patch fixed. =) In GetAttrCount, should we detect that dups == protoCount and break the proto link? That'll keep us from searching through it every time, once we know that we don't need to. + } else { + // Didn't find it on ourselves, but we might need to shadow the value in + // our prototype. Empty else clause can go. Lemme mail you my latest patch, so you can cherry-pick from it.

Okay, I've ripped off (and one-upped, I think) your UnsetAttr changes -- I think there are some cases where people set and remove attributes exclusivly from the DOM, which'd make this worthwhile. I'm going to be a lamer on your GetAttrCount optimization; I'm too stupid to understand why that works right now.

Comment on attachment 54034 [details] [diff] [review] fix shaver's nits Suh-weet. Thanks for being my closer. r=shaver

sr=hyatt

Fix checked in.