Closed Bug 549426 Opened 14 years ago Closed 14 years ago

[OS/2] build break in widgets due to new layer infrastructure

Categories

(Core :: Layout, defect)

x86
OS/2
defect
Not set
major

Tracking

()

RESOLVED FIXED

People

(Reporter: wuno, Assigned: dragtext)

References

Details

(Whiteboard: OS/2 only file)

Attachments

(1 file, 5 obsolete files)

User-Agent:       Mozilla/5.0 (OS/2; U; Warp 4.5; en-US; rv:1.9.3a2pre) Gecko/20100227 Minefield/3.7a2pre
Build Identifier: 

from Dave posted in the newsgroup
I:/comm-central/mozilla/widget/src/os2/nsWindow.cpp
I:/comm-central/mozilla/widget/src/os2/nsWindow.cpp: In member function
'PRBool nsWindow::OnPaint()':
I:/comm-central/mozilla/widget/src/os2/nsWindow.cpp:2124: error: 'class
nsPaintEvent' has no member named 'rect'
I:/comm-central/mozilla/widget/src/os2/nsWindow.cpp:2125: error: no
match for 'operator=' in 'event.nsPaintEvent::region = 0'
../../../dist/include/nsRegion.h:293: note: candidates are: nsIntRegion&
nsIntRegion::operator=(const nsIntRect&)
../../../dist/include/nsRegion.h:294: note:                 nsIntRegion&
nsIntRegion::operator=(const nsIntRegion&)
I:/comm-central/mozilla/widget/src/os2/nsWindow.cpp:2149: error: 'class
nsPaintEvent' has no member named 'renderingContext'
I:/comm-central/mozilla/widget/src/os2/nsWindow.cpp:2157: error: 'class
nsPaintEvent' has no member named 'renderingContext'
make.exe[7]: *** [nsWindow.o] Error 1 
Probably something to fix by Rich

Reproducible: Always
Blocks: layers
Version: unspecified → Trunk
A fix should be written on the cleaned version of nsWindow, setting this bug dependent on bug522896
Depends on: 522896
Attached patch patch on top of new nsWindow (obsolete) — Splinter Review
Rich, after first attempts that resulted in black windows, this to works, menus and option windows show up. Maybe some fine-tuning is necessary, you'll sure know it better.
Assignee: nobody → wuno
Attachment #430151 - Flags: review?(dragtext)
Attached patch Layers + Thebes clipping - v0 (obsolete) — Splinter Review
Sorry to obsolete your patch, Walter, but while I was developing my own (nearly identical) patch, I discovered we were missing something big.  Both Win & Mac create a clip region for the Thebes context prior to dispatching a paint event, so I added that code.

In at least one circumstance, the results are remarkable.  Scrolling thru files in Mozilla-central one line at a time requires 75% of my CPU when using the down-arrow and almost 100% when using the mouse.  With a Thebes clipping region in place, usage drops to 35% and 55%, respectively.  The time required to scroll x number of lines remains the same because the system was already able to keep up with the keyboard or scrollbar's repeat rate.  User's with slower systems may see a real increase in scrolling speed.

My "v0" patch contains 2 1/2 different implementations so that people can see which works best.  If 'SIMPLE_CLIP' is #define'd, a rectangular clipping region is set based on the rectangle returned by WinBeginPaint().  If 'FULL_CLIP' is #define'd, the code retrieves all of the individual rectangles that make up the window's update region & uses them.  This is more complicated and adds unnecessary overhead in the most common case (just 1 rectangle) but may be a big win where the layout is complicated (e.g. YouTube's homepage usually has 15+ rects).  With neither #define'd, you just get the basic Layers patch with performance largely the same as before.
Attachment #430151 - Attachment is obsolete: true
Attachment #430151 - Flags: review?(dragtext)
(In reply to comment #3)
> Created an attachment (id=430207) [details]
> Layers + Thebes clipping - v0
> 
> Sorry to obsolete your patch, Walter, but while I was developing my own (nearly
> identical) patch, I discovered we were missing something big.  Both Win & Mac
> create a clip region for the Thebes context prior to dispatching a paint event,
> so I added that code.
No problem, I see that you removed even a few more bits in the lines that I touched.
> 
> In at least one circumstance, the results are remarkable.  Scrolling thru files
> in Mozilla-central one line at a time requires 75% of my CPU when using the
> down-arrow and almost 100% when using the mouse.  With a Thebes clipping region
> in place, usage drops to 35% and 55%, respectively.  The time required to
> scroll x number of lines remains the same because the system was already able
> to keep up with the keyboard or scrollbar's repeat rate.  User's with slower
> systems may see a real increase in scrolling speed.
> 
I occasionally get hangs when scrolling with the mouse wheel. Though it happens less often with newer trunk builds its happens relatively often with branch builds (1.9.1 and 1.9.2). Could that be related? So I'm curious to test your patch.
Assignee: wuno → dragtext
Status: NEW → ASSIGNED
Attached patch Layers + Thebes clipping - v1 (obsolete) — Splinter Review
This fixes an error in the original OnPaint() code.

I removed a validation at the beginning [if (mContext && mEventCallback)] that could prevent WinBeginPaint()/WinEndPaint() from being called.  Even if nothing else is done, they have to be called to prevent the same paint msg from being sent repeatedly.  In any case, these tests were unnecessary.  Having a valid mContext is a prerequisite for creating a window, and mEventCallback is tested again where it's actually needed.
(In reply to comment #5)
> Created an attachment (id=430376) [details]
> Layers + Thebes clipping - v1
> 

I'm running Minefield built yesterday with your patch using the SIMPLE_CLIP define. Now I'm working to update the libffi patch, having about 10 tabs open. I've been working for a longer time only in one tab and didn't switch to other tabs for say about half an hour or maybe an hour. When I then click on another tab its content is black as is the case with the next inactive tab. Switching back to the first re-activated tab the content is there again as it comes back of course when I use the refresh button.
Just to confirm also with FULL_CLIP defined the idle tabs show up black upon the first activation.
A further problem is that with both SIMPLE and FULL_CLIP defined when one wants to save a page it takes some seconds until the file saving dialog comes up and the processor usage spikes up to 100 % in this time. Undefining both clip variants shows the file saving dialog immediately.
(In reply to comment #7)
> Just to confirm also with FULL_CLIP defined the idle tabs show up black
> upon the first activation.

I've seen this too, but I find it hard to reproduce.  Try adding this to each version:
   thebesContext->SetFlag(gfxContext::FLAG_DESTINED_FOR_SCREEN);
+  thebesContext->ResetClip();

I haven't seen the problem since adding ResetClip() but it could just be luck.

> A further problem is that with both SIMPLE and FULL_CLIP defined when one
> wants to save a page it takes some seconds until the file saving dialog
> comes up and the processor usage spikes up to 100 % in this time. 

I can't reproduce this at all.  I'm going to do an update & rebuild - I'll what how it performs after that
(In reply to comment #8)
> Try adding this to each version:

That didn't work.  I'm trying this instead so that the clip region is cleared after use.  So far, so good.

        thebesContext->Paint();
      }
+     thebesContext->ResetClip();
    } // if mEventCallback

> >it takes some seconds until the file saving dialog comes up

I still don't see this.
(In reply to comment #9)
> (In reply to comment #8)
> > Try adding this to each version:
> 
> That didn't work.  I'm trying this instead so that the clip region is cleared
> after use.  So far, so good.
added the lines below, built with FULL_CLIP, first I thought it was gone, but the black tabs came back. I admit, it's hard to reproduce, because it seems that a longer time the tabs have to be idle. However, I'm not yet sure what longer means, my guess is about half an hour. During this time I'm reloading the active tab often and switch between the editor and the browser (still with only one active tab). Probably not an every day scenario. When I activate the tabs after only a few minutes all is ok (probably more an every day scenario).
> 
>         thebesContext->Paint();
>       }
> +     thebesContext->ResetClip();
>     } // if mEventCallback
> 

> > >it takes some seconds until the file saving dialog comes up
> 
> I still don't see this.
I've checked this a few times since my last comment, and I don't see it anymore either, also after the first built I had problems with scrolling of large files during loading (eg configure.in in mxr). It wouldn't scroll until the whole file was loaded. Now, that's also working again.
The "black tabs" problem is caused by the Layers implementation & is triggered when the window is resized.  You can eliminate the clip-region code and it will still occur.
(In reply to comment #10)
[...]
> > > >it takes some seconds until the file saving dialog comes up
> > 
> > I still don't see this.
> I've checked this a few times since my last comment, and I don't see it anymore
> either, also after the first built I had problems with scrolling of large files
> during loading (eg configure.in in mxr). It wouldn't scroll until the whole
> file was loaded. Now, that's also working again.

I also saw the delay with the file save dialog appearing, at that I thought it was broken. Now it seems to work fine.
I also found that with FULL_CLIP defined some pages scrolled very slowly with large chunks getting redrawn. Now to try SIMPLE_CLIP.
I have an update that fixes the "black tab" bug as well as a dumb error in FULL_CLIP.  I'll post it after I do a new non-debug build (my debug build seems extraordinarily slow, so I want to do a comparison first).
Attached patch Layers + Thebes clipping - v2 (obsolete) — Splinter Review
This fixes the "black tabs" bug and improves the 'FULL_CLIP' version of OnPaint().

The black tabs were caused by the NS_PAINT handler "working-as-designed":  it intentionally does not paint a window the first time it is shown after being resized.  Instead, it updates the size, then re-invalidates it.  This should trigger a WM_PAINT msg but for some reason it doesn't.  I added code that tests for this condition and forces an update if found.

The FULL_CLIP implementation is slightly less efficient than SIMPLE_CLIP for very simple updates (i.e. only one invalid area) but should provide an improvement for moderately complex updates (2-12 invalid areas). Unfortunately, I don't have a way to test this.  Any ideas?
Attachment #430207 - Attachment is obsolete: true
Attachment #430376 - Attachment is obsolete: true
Attached patch Layers + Thebes clipping - v3 (obsolete) — Splinter Review
> It [the NS_PAINT handler] updates the size, then re-invalidates it.
> This should trigger a WM_PAINT msg but for some reason it doesn't.

Dumb, dumb, dumb...  When the NS_PAINT handler returns false, OnPaint() returns FALSE.  This causes the current WM_PAINT msg to get sent to the default wndproc which validates the area that was just invalidated.  Simply returning TRUE prevents the msg from being forwarded, fixing the problem.

After I posted the previous patch, I found a patch Peter had done that added a Thebes validation, so I added something similar.  While I was at it, I also added other validations and reformatted everything.

Now, it's just a matter of deciding which version to use.  FULL_CLIP should be better when there are a few invalid areas that are spaced far apart. It blits each area to the screen separately, while SIMPLE_CLIP blits those areas *and everything in between* all at once.
Attachment #432444 - Attachment is obsolete: true
I think this should be the final version.  It's 'FULL_CLIP' from v3 but with the maximum number of clipping rectangles reduced from 12 to 8 - beyond 8, the extra overhead probably exceeds the performance gains.
Attachment #432473 - Attachment is obsolete: true
Attachment #433779 - Flags: review?(wuno)
Comment on attachment 433779 [details] [diff] [review]
Layers + Thebes clipping - v4

Yep, now it works nice, also scrolling of large source files.
Attachment #433779 - Flags: review?(wuno) → review+
Keywords: checkin-needed
Whiteboard: OS/2 only file
http://hg.mozilla.org/mozilla-central/rev/b046a079a16e
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.