User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.3a) Gecko/20021216 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.3a) Gecko/20021216 It seems that the xul tree paint way too often and way too much. I discovered this by putting some printf in mozilla\layout\xul\base\src\tree\src\nsTreeBodyFrame.cpp in PaintCell, PaintRow and Paint. First of all it looks as if everything (and I mean everything) in PaintCell is drawn twice, once on the backgroundlayer and once on the foregroundlayer. It seemed somewhat awkward so I solved this by changing the code if (NS_FRAME_PAINT_LAYER_BACKGROUND == aWhichLayer) PaintBackgroundLayer(cellContext, aPresContext, aRenderingContext, cellRect, aDirtyRect); in the file nsTreeBodyFrame.cpp into if (NS_FRAME_PAINT_LAYER_BACKGROUND == aWhichLayer) { PaintBackgroundLayer(cellContext, aPresContext, aRenderingContext, cellRect, aDirtyRect); //return; } in a trunk of about half a year old which I still had on my harddisk. This made the horizontal splitter in the mailform work *really* smooth and the vertical splitter and resizing the window were also a lot faster. So I downloaded the latest trunk and applied the same patch but now it had only little effect. It seems that the current trunk paint way too much and which makes this fix less noticable. I discovered that by adding the printf's again. The entire tree is repainted when mouse enters and leaves the tree. You easily see this by moving your mouse in and out of e.g. the download manager, the cpu goes high. When moving a splitter in the mailwindow, you'll see the trees are painted *many* times, probably because the mousecursor first enteres the tree then leaves it again and then has to be repainted for its new size. I'm not sure if the following is the cause, but I noticed that in mozilla\widget\src\nsWindow.cpp in ProcessMessage on every WM_MOUSEMOVE DispatchPendingEvents() is called which results in a new WM_PAINT message. Reproducible: Always Steps to Reproduce:

if (NS_FRAME_PAINT_LAYER_BACKGROUND == aWhichLayer) { PaintBackgroundLayer(cellContext, aPresContext, aRenderingContext, cellRect, aDirtyRect); return; } you should not do this, otherwise background layer of the twisty, image and text will be not painted

anyway, bug 124478 may help here

Another problem here is that treebody is just one frame So frame specific invalidations cause entire treebody to repaint. It means that if mousein/mouseout event causes a frame to repaint it repaints entire treebody. It would be probably better to fix splitter to not update frames after a move immediately and rather coalesce moves and then update at once.

Several questions: > you should not do this, otherwise background layer of the twisty, > image and text will be not painted Is there an example where I can see this go wrong? I can't see any difference. As far as I can see this is what is happening in PaintCell: 1. if backgroundlayer then paint background 2. if cell is primary then paint twisty and lines 3. paint image (which in turn paints the background again(!)) 4. if cel is not a cycler then paint text or progress meter in cell This is done for both foreground and background layer so if I don't bailout after step 1 for the backgroundlayer, it does step 2, 3 and 4 again for the foreground layer. That's why you don't see any difference if we return after step 1: it's painted again on the foreground layer. Is there a design document on why xul works the way it does? I can't really see why it would be usefull to have 3 layers. (are these layers merged and painted or something like that?) Why should a frame be repainted on a mousein event in the first place? Isn't that up the frame itself to decide whether it needs painting on a mousein? I don't understand why the splitter is involved here. The problem also occurs in e.g. the Download Manager where there is no splitter.

try and see by yourself

Ah thanks. I see now that some of my remarks are not true. Sorry for that. Nevertheless, it seems rather inefficient to me that many things are calculated twice, e.g. the metrics for each cell text, which is calculated in PaintText, once for drawing the background and once for drawing the text. I guess that leaves us with that and the repaint on the mousein and mouseout.

Since all the calls to nsTreeBodyFrame::Paint seem to originate from PresShell::Paint and PresShell paint all layers one after the other: rv = frame->Paint(mPresContext, aRenderingContext, aDirtyRect, NS_FRAME_PAINT_LAYER_BACKGROUND); rv = frame->Paint(mPresContext, aRenderingContext, aDirtyRect, NS_FRAME_PAINT_LAYER_FLOATERS); rv = frame->Paint(mPresContext, aRenderingContext, aDirtyRect, NS_FRAME_PAINT_LAYER_FOREGROUND); it seemed worth a try to merge these in a single paint. I've changed the paint methods in nsTreeBodyFrame such that they now paint everything when the backgroundlayer is painted. This gives a small but noticable speedup. I'm trying to find out why the mousein/mouseout causes a repaint. I have found out that a WM_MOUSE_MOVE in nsWindow::ProcessMessage somehow gets translated into a WM_PAINT in nsWindow::ProcessMessage, but the callstack in between only shows some user32 calls. Is there any option I can add to the compiler that would show the callstack correctly?

It does look like nsTreeBodyFrame::Paint could move more of the work into a single layer, but probably foreground would be better. The paint message is probably coming from an event enqueued during the processing of the previous message. There's probably a style change that causes invalidates. Break in nsViewManager::UpdateView to see what's causing it.

Rene, do you volunteer to refactor nsTreeBodyFrame to remove layers from all paint methods ? We should also ask hyatt what he thinks of it. If we can eliminate layers in nsTreeBodyFrame completely it should give us real speed boost. Thanks for catching this. Nominating for Buffy.

I would be delighted. This patch puts all the drawing of the tree in the foreground layer. The patch might seem rather large but I've corrected the indentation; what I've basically done is just remove the if (aWhichLayer == ...) in the paint functions. The speedup is reasonable.

diff -uw would be much easier to review

I thought that we can remove aWhichLayer parameter from paint methods completely. Comments in the code should be fixed too.

New patch - diff -uw - aWhichLayer parameters removed, were obsolete - comments fixed

PaintDropFeedback looks like it might be a problem there, actually, since before it painted between the two layers of the rows. Also nsLeafFrame::Paint probably needs to be called for the background layer. If these are problems, then there might be a need to keep layers here. If so, you could at least get a performance win by fixing the numerous places in the current code where we paint in both layers (e.g., in PaintCell). (However, if layers aren't needed, it's probably better to remove them, since I may want to remove them from the CSS rendering objects at some point.)

If my assumption is correct that the nsTreeBodyFrame::Paint is only called from PresShell::Paint with the code in #8 it looks to me more as if the nsLeafTree::Paint was accidently called twice, once for background and once for foreground. Either way, nsLeafTree shouldn't be a problem. PaintDropFeedback could indeed be a problem. Jan, do you have an example that calls this PaintDropFeedback? Normal drag&drop doesn't call it (which works fine by the way) and I can't figure when it is called.

I'll try to check with hyatt and make sure he is ok with this change.

This is a new patch. In PaintImage and PaintTwisty we were resolving the image twice. This now done only once. We were also always drawing the background and borders for an image, even if there is was none (that is in most cells the case). Now the background and borders are only drawn if there is an image. This makes sense since we draw no background either when there is no text. In 99% of the cases the celltextbackgroundcolor is the same as the cellbackgroundcolor and the same as the rowbackgroundcolor and the same as the treebackgroundcolor. Indeed, right now we're painting the background of each cell 4 times. If the backgroundcolor is inherited, I would think we don't need to draw it again. Is there a way to see if we need to call PaintBackgroundLayer? I noticed that PaintBackgroundLayer and PaintImage both take up a big part of the time needed to draw the tree (which is not surprising since they're both called at least once per cell). In each of these functions a lot of information is resolved, very time consuming. Isn't it possible to avoid calls by storing some information closer to the tree object? E.g. if each row in the tree could just walk through a linked list of cells containing e.g. the text, imageContainer, image size and color. That would save a lot of time. These values could be resolved the first time they are needed from the corresponding DOM element. An observer could watch the DOM element and invalidate the cache if it changes. This should be, if possible of course, in a new bug. I don't volunteer for this one since I don't consider myself qualified for the job: I know too little about Mozilla's internals right now and have some exams coming up. Any word of Hyatt yet or is he still busy with his 314 Christmas dinners? :-)

Nav triage team: nsbeta1+/adt3

hyatt confirmed that he is ok with this change. I'll take a look at the latest patch once I complete high priority items on my list.

Nav triage team: Jan, under what components will users see this performance improvement? Mail, address book, bookmarks? What else? Also, what percentage performance improvement do you expect to see? Thanks.

Currently we paint twice, background and foreground layer separately. When we paint we need to compute the position and size of tree pseudo elements. While computing these values we ask a tree view to return e.g. row and cell properties, then resolving style, etc. The computation is many times redundant because we do it separately for background and foreground layer. This patch could speed up painting by 50-100% I guess. I haven't measured it yet, but I'm sure it will help performance of all tree widgets in the product. I was just not sure if we can make such a radical change, but I talked to hyatt and he is ok with it too.

Ok I measured it. Currently it takes 10 + 18 msec to paint folder pane. It takes just 18 msec to paint folder pane with the v2 patch. So we can save 36%

components which use the tree widget: - mail - addressbook - bookmarks - history - preferences - help - JS Debugger - DOM Inspector - file picker - component view - wallet - security - page info - autocomplete

And - Download manager - about:config - password/cookie/image/popup/password manager With patch v3 the gain should be even more since some expensive paints in PaintImage are removed when there is no image. With patch v3, the majority of the time is consumed by calls to PaintBackground. It is called once for each tree, then for each row, then for each cell and then for each celltext. It would be nice of we could get this out but I haven't figured out how. Anyone a clue?

Rene, optimization of GetImage and GetImageSize sounds good, but the implementation looks weird. That's why I didn't use that one, it even didn't compile on my linux box. I think you should take a look at the XPCOM tutorial on mozilla.org, especially the reference counting part and how to avoid leaks.

>With patch v3 the gain should be even more since some expensive paints in >PaintImage are removed when there is no image. actually, which expensive paints do you mean ? if I'm reading your patch correctly, you just calling GetImageAndSize() instead of GetImageSize() and GetImage() Anyway, I think this is not so expensive since images are cached. I'm not sure if optimization of PaintBackgroundLayer is worth a try. Other frames in the layout call PaintSelf() which is very similar to our PaintBackgroundLayer(). We should rather try to fix bug 113528, that would help performance even more.

ah I see, PaintImage() with your patch v3 checks for null image yeah that will help performace too.

I'll take a look at the XPCOM tutorial soon. I admin that I'm not very familiar with it. I didn't mean optimizing PaintBackgroundLayer but reducing the number of calls it. For example, for a normal all-cells-white table of 15 rows and 5 columns we do the following: - call PaintBackgroundLayer for the table (1x) - call PaintBackgroundLayer for each row (15x) - call PaintBackgroundLayer for each cell (75x) - call PaintBackgroundLayer for each cell-text (75x) - optionally we call PaintBackgrounLayer for every image, twisty and progressmeter We're doing a total of 166 calls to PaintBackgroundLayer where only 1 should be enough since all cells are white and have no border or outline. If the cell-text knowns it has the same backgroundcolor as the cell and no border and no outline, which is in 99% the case, it can skip PaintBackgroundLayer. That's already half the work. If each cell would know it has the same backgroundcolor as its row and no border and no outline then can save another 75 calls. If I have the downloadmanager maximized, I have a 36 rows and 6 columns, totalling 1 + 36 + 36*6 + 36*6 + 36(for the images)=505 calls to PaintBackgroundLayer where 1 should be sufficient. Jan, could you perhaps measure the potential speedup by inserting "return NS_OK;" at the beginning of PaintBackgroundLayer and then measure the time needed to paint the tree. The real speedup would of course be less, but just to know if it's worth it. With respect to the GetImageAndSize, now we are first calling GetImageSize which internally get the image (and needs to resolve a couple of thing for that) and then we get image ourselves. It seemed double work to, that's why the GetImageAndSize. I'm not sure how much we win here though.

Comment on attachment 110228 [details] [diff] [review] patch v3 Forget about that third patch. I don't know how that got so messed up. Probably mixed up some trees I have here. I hope to come up with a new patch and some numbers on this soon. With patch v2 I get a speedup of 33% when viewing the messagepane maximized. This value depends somewhat on the tree you're looking at, it might be more, it might be less.

Nav triage team: nsbeta1+/adt2

New patch. This avoids some unnecessary calculations. I joined the backgroundlayer and foregroundlayer in a single run. This gave the biggest gain. Besides that I moved some stuff to outer Paint function and cached some stuff that was worth caching. This patch also contains a 1 byte fix in EnsureRowIsVisible that avoids flickering when 1 line is selected and the tree is made as small as 1 line. Here are some numbers. I did this testing with the mailwindow maximized and the threadpane maximized: Without the patch: Paint foreground layer: 57ms Paint background layer: 75ms Total: 132ms With patch: Paint: 83ms This reduces the time needed to paint the tree by 39% (or speeds it up by 60%, it's the same but sounds better :-). This seems to be as good as it gets. We could scrape off a few more milliseconds by having GetImageSize also return the image but I couldn't get that to work. The majority of the 83ms is now consumed by the following calls: 10 ms bidiUtils->RenderText I found that rather slow especially is you know that the call to DrawString is so fast that I can't even measure it. 20 ms mView->GetCellText This call is outside the tree so we can't do much about it here. 13 ms aRenderingContext->DrawImage Seems reasonable 7 ms mView->GetCellProperties Also outside the tree. However, this seems like a lot to me, especially if you find out that in case of the threadpane it does nothing usefull. Perhaps I'll fix it in another bug if possible. 7 ms in PainBackgroundLayer This calls draws the background very often as mentioned before. Avoid this call would however require copying part of the logic in PaintBackgroundWithSc if we don't want to break things so I've skipped this for now. The rest of the time is consumed by other stuff which is consuming only little time.

Comment on attachment 112696 [details] [diff] [review] patch v4 Requesting r= and sr= The gain is 37%, not 39%. Sorry for smuggling 2 percent.

Setting to all/all

Comment on attachment 112696 [details] [diff] [review] patch v4 Bah, this will conflict with my patch to bug 120734

Apart from that we're poking around in the same piece of code I don't see any real problem here or am I missing something? If you deflate the textRect again after the PaintBackgroundLayer everything should alright, right?

So this is ready for 1.4a :)

yeah, but I need to go through this patch and make sure everything is ok give me some time

I hope recent checkins didn't break the patch. http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp

Rene, there were some changes in your patch4 which I didn't like or were not quite right. 1. you added additional 6 member variables, each 4 bytes I measured the the caching effect and it was around 1%. I don't think it's a worth, especially when you increased mem footprint by 24 bytes. Keep in mind that XUL trees can be used as HTML selects in future (aka XBL Forms). So memory footprint is also important here. Btw, names of member variables should be preceded by 'm' character 2. I was told it's not a good idea to remove parenthesis for one line "if statements" 3. IBMBIDI define removed ? 4. Can you explain that "<" -> "<=" change in EnsureRowIsVisible ? 5. Painting of drop feedback was not updated as dbaron pointed out 6. You forgot to call nsLeafBoxFrame::Paint for background layer as dbaron pointed out My patch only contains the removal of layers with all necessary changes (5, 6). And I also refactored painting of drop feedback to match the style in this file. Thanks for initial patch.

Ad 1. I went for speed but if you think footprint is in this case more important, no problem. Ad 2. I didn't know that, sorry. Ad 3. Yes, ifdef IBMBIDI was removed as part of bug 89203. See especially #46 and beyond. Ad 4. Yes I can. If Mozilla is in Modern theme, go to the mail window and select the first message in the threadpane. Now resize the threadpane and make it smaller than 1 line. You'll see the line flicker. It'll jump back and forth between the first and second item in the list. This fixes that problem. Ad 5. I know but I couldn't get PaintDropFeedback get called (is it really still in use?) and I couldn't really figure out what it was doing but since it seemed not to draw over parts drawn by the foreground I figured it should be fine. Ad 6. Oops, sorry.

>Ad 3. Yes, ifdef IBMBIDI was removed as part of bug 89203. See especially #46 >and beyond. I see, but we shouldn't mix it with this bug, feel free to file a new one. >Ad 4. Yes I can. If Mozilla is in Modern theme, go to the mail window and >select >the first message in the threadpane. Now resize the threadpane and make it >smaller than 1 line. You'll see the line flicker. It'll jump back and forth >between the first and second item in the list. This fixes that problem. Again, feel free to file a new bug for this issue. >Ad 5. I know but I couldn't get PaintDropFeedback get called (is it really >still >in use?) and I couldn't really figure out what it was doing but since it seemed >not to draw over parts drawn by the foreground I figured it should be fine. Yes, it's called, for example in bookmarks sidebar or bookmarks manager. It's the line painted in between rows when you drag an item.

Comment on attachment 116649 [details] [diff] [review] new patch Doesn't break anything.

checked in

Marking verified (rs). Checked in a long time ago and no comments since the checkin