Optimizing drawing in BeOS widget code

RESOLVED FIXED

Status

RESOLVED FIXED
15 years ago
10 years ago

People

(Reporter: sergei_d, Assigned: sergei_d)

Tracking

({perf})

Trunk
x86
BeOS

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

15 years ago
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)
(Assignee)

Updated

15 years ago
Summary: Oprimizing deawing in BeOS widget code → Oprimizing drawing in BeOS widget code
(Assignee)

Comment 1

15 years ago
Created attachment 138614 [details] [diff] [review]
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
(Assignee)

Updated

15 years ago
Summary: Oprimizing drawing in BeOS widget code → Optimizing drawing in BeOS widget code
(Assignee)

Updated

15 years ago
Assignee: arougthopher → sergei_d

Updated

15 years ago
Keywords: perf
(Assignee)

Comment 2

15 years ago
Created attachment 138750 [details] [diff] [review]
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.
(Assignee)

Updated

15 years ago
Attachment #138614 - Attachment is obsolete: true
(Assignee)

Comment 3

15 years ago
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
Attachment #138750 - Flags: review?(timeless)
(Assignee)

Comment 4

15 years ago
Created attachment 138751 [details] [diff] [review]
patch (diff -uwp)

Same as previous, but rolling back NSMETHOD Enable() changes.
Will be addressed in future in bug about floating and modal windows
Attachment #138750 - Attachment is obsolete: true
(Assignee)

Comment 5

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

obsolete patch
Attachment #138750 - Flags: review?(timeless)
(Assignee)

Comment 6

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

see
http://bugzilla.mozilla.org/show_bug.cgi?id=230382#c2
except Enable()
Attachment #138751 - Flags: review?(timeless)

Comment 7

15 years ago
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)|
Attachment #138751 - Flags: review?(timeless) → review+
(Assignee)

Comment 8

15 years ago
>> +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
(Assignee)

Comment 9

15 years ago
Created attachment 138827 [details] [diff] [review]
Patch (diff -uwp)

Same as previous, but
PRBool mess corrected.
(Assignee)

Updated

15 years ago
Attachment #138751 - Attachment is obsolete: true
(Assignee)

Comment 10

15 years ago
Created attachment 138912 [details] [diff] [review]
Patch (diff -uwp)

Taking in account changes from nsEvent patch
Attachment #138827 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Blocks: 230758
(Assignee)

Comment 11

15 years ago
Created attachment 138937 [details] [diff] [review]
Patch (diff -up)

Submitting for checkin purpose
Attachment #138912 - Attachment is obsolete: true
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
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.