Closed Bug 53657 Opened 24 years ago Closed 24 years ago

Optimize region updating, possible separation of chrome and content

Categories

(Core :: XUL, defect, P3)

Product:
Core
Component:
XUL
Platform:
PowerPC
Mac System 8.5
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla0.9

People

(Reporter: sfraser_bugs, Assigned: mikepinkerton)

Details

Attachments

(3 files)

version of nsWindow.cpp with some rect amalgamation code
66.81 KB, text/plain
Details
better region amalgamation patch for nsWindow
10.69 KB, patch
Details | Diff | Splinter Review
take 2, now with no malloc's
13.30 KB, patch
Details | Diff | Splinter Review 
Window updates now, when switching pages, happen in this weird way where the page 
gets redrawn in vertical stripes.

For exmaple, load www.mozilla.org. Click links on the left side of the page, 
watching what gets redrawn. First, a stripe on the left is redrawn, then most of 
the right-hand side of the page, and then a stripe that continues up from the 
progress bar.
i didn't touch anything. layout must have somehow changed....
Assignee: pinkerton → buster
any of you guys on the cc-list have a clue?
simon, I know you run with double-buffering turned off frequently.  Are you 
doing that now, or are you using double-buffering.  
I haven't see this on WinNT, is this Mac specifiic?  Anybody with a linux box?  
Anyone else seeing this on mac?
Status: NEW → ASSIGNED
Confirming on Mac.

I am seeing this both in this morning's binary of Seamonkey and with a pull I 
did on Monday. I am not seeing the stripe from the progress bar but the left and 
right stripes appear quite quickly on my machine so it almost looks normal. 

checking with qa....
Yes, this is reproducing for me on the Mac Sept 22 build. Tested on two macs:
8600/300 (8.5) and G4/450 (9.0). This only happens for me with the modern skin.
Using the classic skin, I 'm not able to reproduce.

Please don't assign this to skin guys because of that last comment.
So it seems that some combination of animated images is causing regions to be 
dirtied in such a way that we redraw them in this ugly staggered fashion. Maybe 
we need some better smarts for update region coalescing or something.

cc-ing some skins guys.

Testing in Chris P's machine, painting the content area on Mac using the classic 
skin is *much* faster than the same paint operation using modern skin.  Could 
this "ugly updating" just be a symptom of slow painting with modern skin?  In 
other words, could the painting behavior be identical with both skins in terms 
of the regions and order of update, but because something in modern skin makes 
updating in general slow, it causes the visual effect of banding?

Either way, it seems like the fact that updating is considerably slower with 
modern skin is a key issue, and it really might be the point of this bug.  
Simon, what do you think?  Chris is working on determining when this regression 
was introduced. We can try backing up to a skin before that date with more 
recent view and layout code to see if there's a difference.

We tested by using home.netscape.com and my.netscape.com, updating the page via 
reload, with and without sidebar.  It seemed the size of the band might have 
something to do with the width of the sidebar, but that's just a guess.  
smfr, what do you want to do about the region coalescing code you have? I don't 
know what effects it would have on this, though.

Because we paint non-rectangular regions as a series of their component 
rectangles, slowness in painting one rectangle will delay other rects being 
painted, which shows up to the user as flashing in these later rects. Either the 
blitting could be taking longer, or the prep-time to draw could be taking longer.

I just finished pulling the last remnants of instrumetnation for window painting 
out of my tree (damnit!) but I could try a profile if warranted.
Keywords: nsbeta3, nsmac1, skins
At steve's request, I looked at older mac builds to see when the bug first
occured. I was able to reproduce it in the Sept 15th M18 build. The next
archived build that is the August 17th. Attempting to run this build, crash when
starting up on my Mac. So I all I can say is this problem was occuring in the
Sept 15th build.

The reason this happens is that the progress bar is directly adjacent to the 
content area of the window. Because this, on page loads both the progress bar, 
and the page content area is invalidated, so we get an update event for a region 
that covers both:

_____________________________
|                            |
|                            |
|                            |
|                            |
|                            |
|                            |
|                            |
|                            |
|                            |
|____          ______________|
    |__________|


Because layout can only deal with updating rectangles, the Mac widget code
then breaks this into 3 rects vertically:

_____________________________
|   |          |             |
|   |          |             |
|   |          |             |
|   |          |             |
|   |          |             |
| 1 |    2     |     3       |
|   |          |             |
|   |          |             |
|   |          |             |
|___|          |_____________|
    |__________|
    
and calls down into layout for each one, hence the striping effect (since the
drawing is low enough to be visible to the naked eye).

What changed to cause this to happen is a change in the modern skin to make
the progress bar abut the content area. This will be fixed (hangas to file a
separate bug).



simon:  thanks for the great explanation, but it leaves me wondering...is there 
a bug here or not?  if paul is going to change the modern skin under the 
auspices of a separate bug, what is there to fix here?
I wondered too, which is why I left this bug open. I see a number of sucky things 
here:
1. Chrome and content updates are being handled at the same time. Maybe we should
   handle them independently?

2. Region->rect code on Mac is generating tall, thin rectangles, rather than 
   short, wide ones (which are more efficient for QuickDraw to render, and more
   pleasing to the eye).

3. It's possible for a skin author to make content updating look sucky with some
   bad widget placement. We should try to minimise the possibility that this can
   happen (maybe by fixing 1).

Reassigning to Kevin. I don't think this is a core layout bug, based on Simon's
research. Looking at his comment, I think (1) could be some combination of hyatt
and kevin, and (2) should go to kevin as well.

(1) sounds like a new feature, and I think this probably falls below the line
for NS6. What do you think?

(2) kind of sounds like a new feature, too (a flag that says whether the view
manager should bias it's rects horizontally or vertically?)  It could be a
simple change, but I really don't know.  If it really has significant impact on
mac performance it's worth examining now, at least.  If the impact isn't large,
we should Future it too.  Is there any way any of the macheads could mock up a
simple test program that could be used to compare painting performance of the
tall-and-skinny's vs. the short-and-wide's?  Maybe post a request to some of the
mac newsgroups for feedback from outside netscape?
Assignee: buster → kmcclusk
Status: ASSIGNED → NEW
um, the region->rect code is all mac-specific. what does this have to do with the 
view manager?

This bug should be reassigned to Pink (who I believe last worked in that code) or 
me (who volunteered to work from time to time on Mac specific issues), or maybe 
Simon (who never refuses anything). We'll work it out between us. In the 
meanwhile, if some pointy-heads can approve for beta3 or rtm...

A copy of some mails we exchanged with Simon and Mike is below.

Simon Fraser wrote:
> 
> Pierre Saslawsky wrote:
>>
>>Nice investigation on http://bugzilla.mozilla.org/show_bug.cgi?id=53657...
>>
>>I have a question though. When we have this:
>>
>>_____________________________
>>|                            |
>>|                            |
>>|                            |
>>|                            |
>>|                            |
>>|                            |
>>|                            |
>>|                            |
>>|                            |
>>|____          ______________|
>>    |__________|
>>
>>Is it really faster to draw this:
>>
>>_____________________________
>>|   |          |             |
>>|   |          |             |
>>|   |          |             |
>>|   |          |             |
>>|   |          |             |
>>| 1 |    2     |     3       |
>>|   |          |             |
>>|   |          |             |
>>|   |          |             |
>>|___|          |_____________|
>>    |__________|
>>
>>or even this:
>>
>>_____________________________
>>|                            |
>>|                            |
>>|                            |
>>|                            |
>>|       1                    |
>>|                            |
>>|                            |
>>|                            |
>>|                            |
>>|____________________________|
>>    |___2______|
>>
>>rather than this (as we used to - in one pass)?
>>
>>_____________________________
>>|                            |
>>|                            |
>>|                            |
>>|                            |
>>|       1                    |
>>|                            |
>>|                            |
>>|                            |
>>|                            |
>>|   ............             |
>>|___|__________|_____________|
> 
> In this case, it might be faster to draw the last one, but that's not
> true in general (consider the case of updating two, small, distant
> rectangles at the same time). Indeed, this is what we used to do (just
> draw the bounding rectangle), and an optimization that Pinkerton put in
> was to break this up into rects, and update each rect; this really sped up
> scrolling. We've been playing with some rect coalescing here, but need
> some code that a) breaks a RgnHandle into horizontal (not vertical) rects,
> and b) coalesces rects intelligently to avoid updating lots of small rects.
> 

A solution could be to break the region into horizontal rects as you wrote,
but also to do the process in 2 passes: 1 to calculate all the rects and put
them into an array, 1 to draw each rect. Then we could optimize by looking
into the array. If one of the rect is larger than 60% or 80% of the entire
region bounding rect, or if the number of rectangles is larger than let's say
4 (or whatever number is required for a faster scrolling), then we would draw
the region bounding rect instead of the array of rectangles.


Bug 54069 covers the specific issue in the modern skin that causes this.

Cool Mac stuff: reassigned to myself :-)
Assignee: kmcclusk → pierre
Keywords: rtm
Whiteboard: [rtm+]
Adding rtm+.
I attach a version of nsWindow.cpp with some preliminary rect amalgamation code. 
This code does not work properly in all instances, because of the way that rgn->
rect stuff works.

Marking rtm-.  PDT is happier with the workaround in 54751 per our discussions
last week with Paul Hangas (separate thermometer and content area by one pixel.)

cc'ing hangas.
Whiteboard: [rtm+] → [rtm-]
Adjusting summary to generalize this beyond the original case.
Summary: Ugly window updating → Optimize region updating, possible separation of chrome and content
Keywords: skins
improved on sfraser's patch to amalgamate not only adjacent rects, but any rects
whose combined area is a large % of their combined bounding rect (allows
combining close regions that don't actually touch).

oddly shaped windows, such as the tops of tabbed windows in the finder, which
used to cause 8-10 rects, and thus 8-10 redraws, now only cause 1. in quick
tests with the timing code already in nsWindow for timing draws, this improves
the redraw speed by 10% in some cases, and not at all in others.
Assignee: pierre → pinkerton
Keywords: nsbeta3, rtm
Whiteboard: [rtm-]
Target Milestone: --- → mozilla0.9
Comments:
	It's a shame to have to allocate all those Rect pointers. Why not first 
count the # rects, then allocate a buffer of size n * sizeof(Rect). You can then 
use qsort to sort them in the buffer.

Also, when calculating boundingRectArea, you could use the UnionRect call to get 
the bounding rect of cur and next.

ah, UnionRect is a great idea. 

as far as the quicksort, i think that the overhead will be too great since they
are already mostly sorted to start with and there are generally only about 10
rects max. *shrug*
Status: NEW → ASSIGNED
quickSort (O(n*log(n)) can beat bubble-sort (O(n**2)), but maybe it doesn't
matter for short arrays.  However, the malloc-each-rect bug is separate from the
use-the-best-sort-alg bug, and should be fixed -- my 2 cents.

/be
Two things re: the sort:
- why we're so concerned about a 10 item list (max) is beyond me
- quicksort is O(n**2) on a list that is already, or mostly, sorted as this one
is. quicksort is not a panacea to be blindly applied to every situation just
because it's in the c std library.

I can look into reducing the mallocs, but again, we're talking about 10 max,
usually (in the most common case) one. It's not like i'm malloc'ing 4000 times
every redraw.
pink: thanks for pointing out that quicksort is not a panacaea -- now who ever
said it was?

Not me: if the length is bounded to 10, and the array is usually sorted, then my
comments about "it doesn't matter" still stand.  But if the length is bounded to
10, why malloc at all?  Why not use an auto or otherwise non-malloc'd array of
10 rects?  Sorry if that's a dumb question.

/be
sr=sfraser

patch landed, so what do we want to do with this bug now? there's still the bit 
about separating chrome and content, but i'm not sure how the toolkit can make 
that distinction (since it's all content to the toolkit)...

should we leave this bug open? who should own it? should we just file a new one 
and reference this?

Try mouse-wheeling on this page, then try clicking scrollbar to scroll. Very
slow on Linux at least. To see how bad it really is: after clicking scrollbar,
change focus to other apps, focus back, then click File menu (or any menu)
http://www.w3.org/TR/DOM-Level-2-HTML/html.html
R.K., this bug was specifically a Mac issue. The problems that you note with 
scrolling (and other behaviours) on w3.org pages, a recent regression, is bug 
68821
John, I checked and that page does not have the same problems as those reported
in 68821 - they use different stylesheets. I do not know what the problem is
with http://www.w3.org/TR/DOM-Level-2-HTML/html.html, I could not see anything
too nasty myself.

Sorry for the OT noise, but I don't want any mistaken associations propagating.
Didn't read much further than the summary. I thought that page in particular was
a good show of how content interferes with chrome. If I'm the only one who see
it it's probably just a local misconfiguration - sorry.
fixed to get off my radar.
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: