230758 - [BEOS]Improving Asynchronous drawing in BeOS-widget

Status: RESOLVED WONTFIX

(Core Graveyard :: GFX: BeOS, defect)

Description

[Sergei Dolgov]

There is special function 
ConsumeRedundantMouseMoveEvent(MethodInfo *pNewEventMInfo)
in beos/nsAppShell, which reduces load of sequential mouse move messages.

Same can be done with synchronous drawing, because nsViewBeOS::Draw collects all
update rects, and then flushes it in single NS_PAINT event.

It will reduce message load and improve visual appearance.

This bug slightly depends on bug 230382, because patch for that bug separates
synchronous events from BeAPI Invalidate()/Draw() and uses both last methods
only for asynchronous painting.

Comment 1

[Sergei Dolgov]

Attachment: Patch (diff -uwp)

implements
void nsAppShell::ConsumeRedundantDrawingEvent(MethodInfo *pNewEventMInfo)
method:
This method detects sequential ONPAINT event generated by nsViewBeOS::Draw()
and deletes older one, for the purpose of performance. 

It is safe because nsViewBeOS::Draw()  collects update rects.
So, if we are discarding ONPAINT events here, paintregion still keeps those
rects until isolated (last) event happens, and is passed to CallMethod in
nsWidget where OnPaint() is called.

Comment 2

[Sergei Dolgov]

Attachment: Patch (diff -up)

Same as previous, but different diff flags used.

Comment 3

[Sergei Dolgov]

Comment on attachment 138938 [details] [diff] [review]
Patch (diff -up)

Biesi, are you brave enough to review it?

(Let's timeless to sleep a bit)

Comment 4

[Sergei Dolgov]

Comment on attachment 138938 [details] [diff] [review]
Patch (diff -up)

found typo there. fixed version is coming

Comment 5

[Sergei Dolgov]

Attachment: patch for estimation

OK, people, i think it is time to test that patch more widely.
It implements following methods
    void ConsumeRedundantDrawingEvent(MethodInfo *pNewEventMInfo);
    void ConsumeRedundantMoveEvent(MethodInfo *pNewEventMInfo);
    void ConsumeRedundantActivateEvent(MethodInfo *pNewEventMInfo);
    void ConsumeRedundantResizeEvent(MethodInfo *pNewEventMInfo);
    void ConsumeRedundantGotFocusEvent(MethodInfo *pNewEventMInfo);
    void ConsumeRedundantLostFocusEvent(MethodInfo *pNewEventMInfo);
It's intention is to reduce message load through bottleneck of proxy betweeen
several BeOS native events from different threads and single-threaded
mozilla-beos-port message dispatcher.

It improves live-resize, makes drawing visual appearance even faster, removes
confusing extra-events for activation and focus changes.

Currently there are printfs() in code to show in Terminal if some redundant
event was consumed/dropped. Drawing events from debug output excluded -
outcommentd, because printf there reduces performace a lot, but if you enable
it, you will see that there is zillion redundant drawing events passed from
invalidation engine, which really weren't required for proper drawing.

To estimate full performacnce boost - outcomment all printfs. Sure, it can be
done with environment options, like debug, but not at that stage.

Comment 6

[Sergei Dolgov]

Attachment: Patch (diff -r HEAD -up7)

Final patch.
All redundancy-decreasing methods joined into one with switch.
Common event structure moved in *.h
Priority list enym is now typedef-ed for use in method parameter.

Ugly mix of styles unified.

Comment 7

[Sergei Dolgov]

Comment on attachment 139764 [details] [diff] [review]
Patch (diff -r HEAD -up7)

review request

Comment 8

[tqh]

Apart from a missing break after case nsWindow::ONACTIVATE :

it looks good.

Comment 9

[Sergei Dolgov]

Attachment: Patch (-r HEAD -up5)

same as previous but break; in switch added

Comment 10

[Sergei Dolgov]

Comment on attachment 139764 [details] [diff] [review]
Patch (diff -r HEAD -up7)

obsolete

Comment 11

[Sergei Dolgov]

Comment on attachment 139831 [details] [diff] [review]
Patch (-r HEAD -up5)

r=?

Comment 12

[tqh]

Comment on attachment 139831 [details] [diff] [review]
Patch (-r HEAD -up5)

r=[e-mail]

Comment 13

[Sergei Dolgov]

it seems there will be lot of changes soon in widget code, so this patch needs
revision.
Only things that will remain here for sure are ONPAINT, ONMOUSE and, in
consideration - ONRESIZE

Comment 14

[Doug Shelton]

is this patch still relevant?  Or have subsequent changes made it obsolete? 

Comment 15

[Sergei Dolgov]

Idea about reducing event number, going through the ports and CallMethod(Async)
bottleneck is still actual, but really, in light of made and planned changes in
AppShell and Toolkit this one may be irrelevant anymore.

I have feeling that actually we need to process (and generate, btw) only events
from top-view, attached to window first, as Mozilla IMHO handles all remaining
by self - keeping accounting of children coordinates and visibility(Z-order).

Comment 16

[tqh]

That is the conclusion I've come to as well. We are allowed one event thread per
top level window and it's BView (maybe same as the first childview for Mozilla)
should probably handle most of keyboard, mouse, resizing and drawing events. 
Unfortunatly such a redesign requires some free time to do it.

I think this patch may not make much difference at that point and might only
confuse those trying to understand the code.

Comment 17

[Doug Shelton]

given these sentiments, the "resolved / won't fix" makes sense.  Thanks for the
review, folks.  I've slowly been trying to go through the various patches and
understand the ones not yet committed to CVS.