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)
Summary: Oprimizing deawing in BeOS widget code → Oprimizing drawing in BeOS widget code
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
Summary: Oprimizing drawing in BeOS widget code → Optimizing drawing in BeOS widget code
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.
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
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
Comment on attachment 138750 [details] [diff] [review] patch (diff -uwp) obsolete patch
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 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+
>> +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
Created attachment 138827 [details] [diff] [review] Patch (diff -uwp) Same as previous, but PRBool mess corrected.
Created attachment 138912 [details] [diff] [review] Patch (diff -uwp) Taking in account changes from nsEvent patch
Attachment #138827 - Attachment is obsolete: true
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
You need to log in before you can comment on or make changes to this bug.