230382 - Optimizing drawing in BeOS widget code

Status: RESOLVED FIXED

(Core Graveyard :: GFX: BeOS, defect)

Description

[Sergei Dolgov]

Currently NSMETHODS Update(), Invalidate() and InvalidateRegion() use
BView::Draw() and BView::Invalidate().
It involves BeOS app_server and results in calling it at least twice (second
time in gfx code, where actual drawing is happening). Also it generates
unneccessary message due usage of CallMethodAsync(), adding load to main
bottleneck of BeZilla.

At least in case of synchronous painting, NSMETHODs must use direct call of
OnPaint() instead.

Case of asynchronous painting may involve BView methods (already tested), or, as
hypothesis, may use CallMethod(ONPAINT)

Comment 1

[Sergei Dolgov]

Attachment: patch (diff -up)

Draft. For testing and estimation. No review will requested.
Applies mentioned ideas + fixes scrolling of i-frames.
Also seem fixing problems with native clipping regions

Comment 2

[Sergei Dolgov]

Attachment: patch (diff -uwp)

Drawing and scrolling optimization.

Minor changes:
Excessive locking removed where possible. Some places for Lock/Unlock adjusted.

BView::SetViewCursor() replaced to BApplication::SetCursor, to avoid locking 
and add responisveness.
For same purpose removed unused argument from MethodInfo in
nsViewBeOS::MouseMoved() (i observed sad truth that current messaging via
CallMethodAsync and ports is quite ineffective and suffers from every
additional argument).

return values for NSMETHODs reorganized in some places.
:Enable method cleanup - current implementation was useless.
Source identation adjusted.

Major changes:
Invalidate methods now use direct OnPaint() calls for synchronous drawing
and nsViewBeOS::Draw() to asynchronous.

Drawing now uses scanning through region rects rather than using region.Frame()

(in NSMETHOD InvalidateRegion and in nsViewBeOS::Draw())

Begin/EndResizingChildren rewritten to use switching between BView::Draw() and
DrawAfterChildren(). 
NSMETHOD Scroll() now uses that technique too.

Comment 3

[Sergei Dolgov]

Comment on attachment 138750 [details] [diff] [review]
patch (diff -uwp)

review request.

timeless, hope you have some time to look at.

Patch was tested by BeOS users and seems improving performance. Was really
noticeable on Pentium 166.

Though, even imporved scrolling still has issue - now those i-frames blink a
bit on scrolling, instead previous awful lagging.

I hope i will fix it in future, maybe with some changes in mozilla/gfx

Comment 4

[Sergei Dolgov]

Attachment: patch (diff -uwp)

Same as previous, but rolling back NSMETHOD Enable() changes.
Will be addressed in future in bug about floating and modal windows

Comment 5

[Sergei Dolgov]

Comment on attachment 138750 [details] [diff] [review]
patch (diff -uwp)

obsolete patch

Comment 6

[Sergei Dolgov]

Comment on attachment 138751 [details] [diff] [review]
patch (diff -uwp)

see
http://bugzilla.mozilla.org/show_bug.cgi?id=230382#c2
except Enable()

Comment 7

[timeless]

Comment on attachment 138751 [details] [diff] [review]
patch (diff -uwp)

can you change
> +rv = PR_TRUE == OnPaint(mBounds)?NS_OK:NS_ERROR_FAILURE;
to be a bit cleaner (at least add some whitespace)?
you should be able to drop |PR_TRUE ==|. if you can't, then your api is broken

<fyysik> timeless - sure. I introduced that horro because you said me that
PRBool shouldn't be compatible with bool, so i added it to correctness and
portability
anything that returns PRBool should be testable as if (foo())
<fyysik> timeless - that's nice, but i couldn't find formal rules about PRBool.
Sure, i can find definition, but it isn't correct style. same as to use int
foo; in if(foo)
hrm
somewhere we should have a guidleline at least for PRBool
for bools simple if (foo()) is always fine
i'm not sure there's a standard for int across modules
<fyysik> you told me in previous patch that PRBool  is bad to compare with real
bool
it is! but i didn't say to actually compare to PR_TRUE/PR_FALSE :)

<fyysik> timeless - so, if PRBool is even good for if() - is it also good for
?:  expression? Do we need to describe each possible use of Mozilla's onw types
for compatibility?
yes it's fine in ?:
> +if (NS_OK != ((nsIRegion *)aRegion)->GetRects(&rectSet) )
:(
1. tabs
2. why not NS_FAILED(...)
> +return NS_ERROR_FAILURE;
3. why not return the rv from GetRects?
<fyysik> timeless - IIRC NS_FAILED is macro and it failed parsing parenthesises
there once, so i replaced it with clean code
oh! yuck

<fyysik> 3rd notice may be good.
nsresult rv = ((nsIRegion *)aRegion)->GetRects(&rectSet) ;
if (NS_FAILED(rv))
return rv;
(and of course indent...)

> +bRet = (PR_TRUE == OnPaint(r));
you can't simply drop the |PR_TRUE ==| as bRet is bool, instead you'd have to
do |!!OnPaint(r)| or if you really want to do a comparison it should be
|PR_FALSE != OnPaint(r)|

Comment 8

[Sergei Dolgov]

>> +bRet = (PR_TRUE == OnPaint(r));
>you can't simply drop the |PR_TRUE ==| as bRet is bool, instead you'd have to
>do |!!OnPaint(r)| or if you really want to do a comparison it should be
>|PR_FALSE != OnPaint(r)|

Really? Always thought, from first reading of C&R that comparison of two values
of ANY type is expression which has boolean value. So inspite what we have as
types in  
= (foo1 == foo2);
it is always bool:)
fixing other things

Comment 9

[Sergei Dolgov]

Attachment: Patch (diff -uwp)

Same as previous, but
PRBool mess corrected.

Comment 10

[Sergei Dolgov]

Attachment: Patch (diff -uwp)

Taking in account changes from nsEvent patch

Comment 11

[Sergei Dolgov]

Attachment: Patch (diff -up)

Submitting for checkin purpose

Comment 12

[Christian :Biesinger (don't email me, ping me on IRC)]

Checking in nsWindow.cpp;
/cvsroot/mozilla/widget/src/beos/nsWindow.cpp,v  <--  nsWindow.cpp
new revision: 1.74; previous revision: 1.73
done
Checking in nsWindow.h;
/cvsroot/mozilla/widget/src/beos/nsWindow.h,v  <--  nsWindow.h
new revision: 1.29; previous revision: 1.28
done