312755 - [BeOS] To reduce mousemove flood

Status: RESOLVED WONTFIX

(Core Graveyard :: Widget: BeOS, defect)

Description

[Sergei Dolgov]

Due proxy-ing native events from several threads to single thread we have
(still) real bottleneck here.
For Draw() events we use BRegion to cache BRects to solve this problem.
For mouse events (and this is special problem and subject to care also in
Mozilla for platforms without such proxy!) we may get required info at main
thread, at least big part of that, instead pushing lot of arguments through
single message queue via CallMethodAsync().

I tested this approach, and "performance" boost is very noticeable, e.g. when
scrolling complex pages. At least it really makes big difference at Dual
PIII-550, so may be helpful also for modern computers.

Comment 1

[tqh]

I think we can rely a lot less on callmethod at all if we handle locking
correctly. See what windows uses callmethod for.

Comment 2

[Sergei Dolgov]

Attachment: patch

First take. Simplest, more like demo but with already huge impact in my case.

Actually, zero arguments may be used for MouseMove and MouseUp, and single
argument (clicks) for MouseDown.

Comment 3

[tqh]

Hmm, I wonder how the locking affect things.

Comment 4

[Sergei Dolgov]

Performace boost is visible anyway. It may be not real boost, but sensible for
user in appearance.

I think that locking is not so dangerous, as we spending less time in (anyway)
locking state in BView hooks + reducing time spent in sending messages through
port + getting port-queue free for other events faster - so those also shouldn't
stay in some state, waiting to obtain lock.

Actually, this GetMouse() should me moved to DispatchMouseEvent() - for all
events, be it buttonclick or onmouse.

Actually I'm using this approach in most of my builds since StripZilla days, and
this is one of the reasons why users wondered about its better "responsivness
and speed", even with O0 optimization or without optimization at all.

Comment 5

[tqh]

Hmm, I'm a bit worried that this might complicate splitting nsWindow to a
nsWidget part. Maybe this change is a bit premature, even though it does have
it's advantages. I'd rather see that we focus on getting nsWindow in the shape
that we want before we start working on optimization.

Comment 6

[tqh]

Ok, thought about it some more.
a) The locking can be avoided by caching position in nsViewBeOS
b) This solution has some flaws:
  * There is no sync between the event and the position.
    While the event always gets the latest position, it does not correspond to
what it was when the event was sent.
  * There is no reduction in the number of events.
    We do get the latest position, but all the events between the first pos and
the current will still be sent.
  * The code gets more complicated.
    It will be harder to refactor the code if we start these optimizations
before being done with structure.

I think a better approach would be to update or cancel the event sent if it's
unprocessed instead as they become outdated when a new is posted (for one object
of nsWindow). I also think that the event should keep the position to always
make sure the state is absolutly correct. Above all I think we should try to
remove as much as possible of CallMethod first instead of tuning it.

Comment 7

[Sergei Dolgov]

1)I tried with caching in nsView long ago, it had some problems, canot recall
atm, but we can try
2)"While the event always gets the latest position, it does not correspond to
what it was when the event was sent" - 
a)there is flag in GetMouse(*, bool history) - when it is set to true, it don't
loose event so critically
b)actually we NEED to loose some events!!! like in my example with "swinging"
page after we released mouse after intensive scroll.
c) There is no need to increase code in CallMethod with this approach. Most of
processing can be done in DispatchMouseEvent()

btw, I suspect that at moment DispatchMouseMethod() lacks DnD processing for
case where mEventCallback == 0. I'm not sure if it is needed, but now there are
only "usual" mouse actions in use, like
ConvertStatus(mMouseListener->MouseMoved(event));

Comment 8

[Sergei Dolgov]

Also i wish to turn your attention that GetMouse isn't global method, like
get_key() or such. It is BView method like our GetPaintRegion, effectively this
is just cache which you propose to implement by own hands:
http://www.acm.uiuc.edu/bug/Be%20Book/The%20Interface%20Kit/View.html#GetMouse()
Quote:
If checkQueue is true, as it is by default, this function first looks in the
message queue for any pending reports of mouse-moved or mouse-up events. If it
finds any, it takes the one that has been in the queue the longest (the oldest
message), removes it from the queue, and reports the cursor location and button
states that were recorded in the message. Each GetMouse() call removes another
message from the queue. If the queue doesn't hold any B_MOUSE_MOVED or
B_MOUSE_UP messages, GetMouse() reports the current state of the mouse and
cursor, just as if checkQueue were false.
... By having it check the message queue, you can be sure that you haven't
overlooked any of the cursor's movement or missed a mouse-up event (quickly
followed by another mouse-down) that might have occurred before the first
GetMouse() call. 
end quote

Comment 9

[Sergei Dolgov]

"Hmm, I'm a bit worried that this might complicate splitting nsWindow..."
How?
If we don't use most radical version when we assign own identifier for each type
of events (MS Windows version uses shuch approach, btw, instead messing all
events in single ONMOUSE), like ONMOUSERIGHTBUTTONUP etc, actually code wouldn't
change in sctructure.

Comment 10

[tqh]

What I meant was the number of events to callmethod, which I think we should
reduce the events in. It's also the event flow to callmethod that I feel must be
kept in sync. If EventA is a mousemoved to pos a, and EventB is a mouseDown, and
EventC is a mouseMoved (before A is processed) EventA will get the pos of C and
I'm not sure that would be right. I must read up on GetMouse though.

When it comes to changing the code I'd like to see something like this:
* toplevel window consists of a BWin and a BView (BWin is also event processor)
* child window -> Bview
* invisble win -> only the minumum from nsWin
* popup -> BWin, BView? (Uses toplevel win as event processor, how to do this?)
* dialog ->BWin, BView? (..)

Any code that adds logic just to gain some performance or that doesn't add
functionality is what I'd call 'complicating things'.

I think we need to discuss this more, there are things to do here, but how and
in what order.

Comment 11

[Sergei Dolgov]

"Any code that adds logic just to gain some performance or that doesn't add
functionality is what I'd call 'complicating things'."

Well, i thought so too and wondered BeOS code simplicity in comparison to other
platforms until i was forced to rewrite DrawTile Method in gfx/nsImage.
Before i made "things complicate", some pages, like www.nbc.com (Late Night and
Leno's sjows)just put Mozilla to hang. On my AMD 550. For 10 minutes. And each
attempt to scroll did so.

I had lot of blame with Paul about it who had more powerful machine at moment
and was also against "complexity". 
Latest font-width caching is also "unneccessary complexity for some bit of
performance" - should we care?

As i understand, no - in your opinion.
Yes - in mine - until we can run Mozilla at same class machines as MS Windows
version does and with same speed as MS Windows version provides.
I run Mozilla at work at Pentium MMX 166 MHz and it is absolutely usable. 
BeOS version sucks event at 300 MHz machine.
Ohh, yeah, nobody has such crap nowadays, I understand.
But for me it is clear evidence, that our code is far from perfectness in
performance.
Yeh, there are global problems in development, like totally wrong approach with
nsAppShell, but do you think it can be done tomorrow? We can fall with that into
risk like it was with Paul and tb100 - some work is to said happening behind the
scene,  this and that is planned and taken - and nothing happens. For years.
Yes, I do mistakes. Frequently. That's sad. Bit it is known truth that "no
mistakes if no actions". And in our situation with tiny developers, testers and
users base we are forced to allow public to test "not so mistakes-free work".

So, i don't share your arrogance here - having AMD 2600+ at home, I'm still
developing Mozilla on PIII-550 - just to keep some connection to reality and be
able to feel affect of coding changes.

Ok. Now stopping all that philosophy. I'm waiting for some code or at least
clean and simple ideas on the problem, so i can implement it myself (as you have
lot of other things to do) and test it on my crappy machine and tell if it
provides same experience as this code in patches.

Comment 12

[tqh]

That's not my philsophy on everything it's just that I think nsWindow is in a
shape it shouldn't be, and you've said yourself that you think we should have a
BaseWidget. I just think it's better to do that kind of work before we add a lot
of code just to tune for performance when afterwards we might change the whole
structure. And I was only talking about nsWindow, not any other code, not any
other bug either for that instance. 

All I'm saying is:
 * First priority: create a good nsWindow/BaseWidget structure for BeOS.
 * Second priority: Implement all the things we havn't so far.
 * Third priority: Add optimizations where it's needed.

And I do care about performance very much, just take a look at what I've done to
condvar and nsAppShell and other bugs and you see that I've tried to get them
work as good as I can. The difference between my point of view and yours seems
to be that I look at this in a longer perspective than you. You want that
performance now, today, while I want to wait until we actually have a good
nsWindow implementation. Which might take a week, but could take months as well.
Performance comes second to quality in my book, and it always will, it doesn't
mean that I don't care about it.

Then again, if there are changes that increase performance without increasing
code complexity I'm all for it. Otherwise it depends on the code.

Comment 13

[Sergei Dolgov]

For example
https://bugzilla.mozilla.org/show_bug.cgi?id=36146
counting changes for all platforms + layout code chaned and added thousands line
of code.
Just to get some percent of performance boost for some sites.
-------------
And again about doing thing properly in very regular manner. That's very good in
theory. Don't do "temporary" fixes, don't do hacks, wait for more generic proper
solution.

Well. Same for latest work in GFX - it is senseless with such approach.. Why to
bother with fixing it, if it will be replaced by cairo anyway?
And work in nsWindow. How it looks like now - 
ok, i understand that work on just-in-time fix of StandardWindowCreate is
senseless, as it is wrong anyway - it will be changed with Window and Widget
separation - right?
And in order to not break work and new Create, i avoided lot of other patches,
like JapaneseInline patch, and especially Scroll-patch. Even if to take in
account fact that Window/Widget separation may use absolutely different approach
in that case. And so on and so on.

And separation of Window and Widget also may be dependent on AppShell
potentially better implementation, which allows us to do embedding.

So i don't see for myself any field of work at the moment - if not to take in
account endless work at AppShell/Toolkit. 
Or maybe some SeaMonkey-specific things like fixing Net+ bookmark import.
Any idea?

Comment 14

[tqh]

Ok, if we can't discuss thing you're on your own on this one. (And I don't like
to be called arrogant.)

Comment 15

[Sergei Dolgov]

We can. 
If you see problem here. 
So there is some motivation to discuss real (not very theoretical when things
will go better at whole) ways to solve.
But i got feeling that you don't.

So i will continue to put this hack in my builds aposteriory, waiting for
overall better code.

But i see need to file more symptomatic, less receipe bug, about that scrolling
inertia, where it don't react at mouse release until it performs all
scroll-moves collected in queue. According my experience, other platfroms don't
behave such way, even BeOS apps. Maybe i even filed such bug before.

Same happens with intensive user resizing  - but is less noticeable with latest
speed-ups.

Comment 16

[tqh]

Read up on GetMouse. Sounds useful.
I think you've missunderstood me, and we didn't clear that up.
Most of the talk here was only in regard to this patch:
a) What complicates refactoring is adding code in Callmethod, so it should
probably be moved to some On.. method.
b) Less locking means less troubles when trying to get several event-processors.
c) I'm not in any way (except maybe Create-method) working on nsWindow. It's
just that if I'm going to try to change how events are handled, I'm pretty much
forced to, and changing how events are handled is hard enough.
d) I have currently no plans on doing the work on refactoring nsWindow. I don't
have the time. Although I think it's very much needed.

But I think it all started when I said that I think that the patch in the
current form, needs to be refined.

In it's current form you may also loose MouseUp events GetMouse detaches one
event  of either MouseMoved or MouseUp, so you need to take special care and do
almost everything in nsViewBeOS MouseUp if you manage to detect it's a MouseUp.

So if you want to do this you should probably only send notifies in nsViewBeOS
mousemoved and mouseup to nsWindow, which then uses GetMouse and send 'mouse
moved' and 'mouse up' and DND-messages correctly. Then it will handle everything
in sync and not loose important messages.

Comment 17

[Sergei Dolgov]

GetMouse may be placed in DispatchMouseMessage, as check for closing popups and
generation of context menu events, ONMOUSE and BTNCLK replaced with single case,
so call method goes even more compact. Maybe MouseUp needs special care, but i
haven't time last days fo investigation.

Comment 18

[Sergei Dolgov]

If you look at sources you got from me previously, and don't put attention at new 
 cases in CallMethod() switch, you will see how it can be implemented.
So single GetMouse takes care about all events. 
Only questions remains about use Window()->CurrentMessage()-> in BView mouse
hoocks, if it may be affected by GetMouse. As i understand, we need it only in
order to get number of clicks.
 Number of cases may be reduced to 1 for all mouse events as i said previously. 
I multiplicated it only in order to reduce message sizes, but it isn't obligatory.
What about MouseUp, we discussed it before a bit, regarding approach proposed by
Stephan Asmus and used in his app Wonderbrush.
Like checking for fIsButtonsDown flag in MouseMove (or even in DispatchMouse
event!) and generating either MouseUp in MouseMoved (or even instantly sending
NS_*_UP event from DispatchMouseMessage).

Anyway, as i said, all this can wait and i didn't plan to checkin current patch,
as it isn't as complete/radical as i wish, considered is rather as moderate demo
to show principle.

At the moment i'm busy with work in GFX again, so all this can wait

Comment 19

[tqh]

Yes, but the current patch is already in CVS, and to me it seems to be a bad
thing, both the code and how it was handled. It's broken, like I've tried to say
in the previous messages. I do hope you plan to do a version that works
correctly SOON.

Comment 20

[Sergei Dolgov]

where did you see it in CVS?
Was it checked in by mistake?
I even didn't ask review here.
If it happened, it must be removed from there, for sure. It wasn't intentional.

And side notice/question. We do use GetMouse() in wheel processing, do you thing
it may affect also other mouse events?

Comment 21

[Sergei Dolgov]

Attachment: patch

reduced internal version - CallMethod was even squeezed, not increased.
special care about MouseUp

Comment 22

[Sergei Dolgov]

again, last patch is also only topic for discussion, as it is still rather buggy.

Comment 23

[tqh]

It was checked in with IME. I thought that was intentional on your part due to
the breakdown in communication between us. I'd be very glad if it wasn't
intentional.

http://lxr.mozilla.org/seamonkey/source/widget/src/beos/nsWindow.cpp#2151

Comment 24

[Sergei Dolgov]

Attachment: patch

context menu handling restored over previous version

Comment 25

[Sergei Dolgov]

It wasn't. 
How it happened - it is topic to discuss outside bugzilla.
As to discuss what we should do now in this situation, when IME was landed in
trunk already, but not in branch yet.

Comment 26

[tqh]

One thing I'm not sure of is if it can remove other views messages as it
apparently tracks over full screen. Also tracking outside view is not
interesting while mouse isn't down.

Comment 27

[Sergei Dolgov]

Over-screen tracking was always mistery for me. Even with old code when we do
all in BView mouse hooks.
Maybe you noticed such thing already - even when Mozilla window isn't
Front-window and we move mouse over it - all buttons react on that - bu changing
shape to 3D.
And that happens without mousepress in Mozilla which triggers EventMask on!!!
And even when mouse was pressed by occasion in Mozilla, MouseUp should stop
global scope, according to BeBook.

Comment 28

[Sergei Dolgov]

At least one additional benefit i noticed with last patch.
When you open Bookmark Menu and it is too long, so goes outside bottom of mozila
window, if you traverse it deeper, so it starts autoscroll, and then coming
back, with old code selection for those items which are below Mozilla window
bottom was 
 invisible.
Now it is visible. Maybe this is just about global things.

Comment 29

[Sergei Dolgov]

Comment on attachment 200336 [details] [diff] [review]
patch

BTNCLICK code isn't the same as in local version here, so left mouse click is
broken with the patch.
Thanks tigerdog for notice.
Obsoleting

Comment 30

[Sergei Dolgov]

Attachment: patch

left button fixed

Comment 31

[Sergei Dolgov]

I had again some more detailed conversation with Stepah Assmus from YellowBites.
He tells that this is even app_server who drops mouseUp messages when mouse is
down-and-in-move and destination thread si busy with some work, like painting.
According to him, every painting app developer in BeOS is forced to solve this
problem, in this or that way.

So it is the code for that in my last patch, now in DispatchMouseEvent - it
check in MouseMoved case if button down flag was really reset in MouseUp case
and if really  button is down or not - and generates MouseUp until this flag
goes to be off. In our case most obvious situation when we should care about is
scrolling, but in future we should also take DnD in account, especially when tqh
adds image-dragging. So we would reconsider the code, to either call MouseUp
with suitable BMessage, or forget about MouseUp and deal with ns-events directly.

Also this means that message parameters, like coordinates, if we get those from 
 BView hooks may don't reflect current state even for BView, not to say about
mWidget. IMHO we must stay in synchro rather with mWidget.

So I see in current patch code thow main things to discuss - true/false
parameter in GetMouse (especially regarding existance of GetMouse in MouseWheel
processing, and, second thing - should we really raise mView->MouseUp() or don't
care about backway to BView and send NS_*_BUTTON_UP directly from
DispatchMouseEvent instead?

Comment 32

[Sergei Dolgov]

As it follows from more generic logic described in bug
https://bugzilla.mozilla.org/show_bug.cgi?id=314687
we'd to use false flag for history in GetMouse().
Like we don in wheel event handling.
It preserves native messages and also allows us to get real current state of native widgets.

Comment 33

[Sergei Dolgov]

As tqh have suspitions about BView::GetMouse()
i wrote simplicistic functions for same purpose:
BPoint nsViewBeOS::GetMouseData(uint32 *Buttons)
{
	*Buttons = buttons;
	fMouseDispatched = true;
	return mousePos;
}
It works too, but I don't notice any difference in Mozilla behaviour from case when 
BView::GetMouse(*, false) is used.

Comment 34

[Sergei Dolgov]

Attachment: patch

Quite moderate. Actually changes only MouseMoved processing a bit - by implementing simple getter  nsViewBeOS::GetMousePos() - analogous to GetWheel(). Also adds flag sMouseMoveDispatched, like it was done for other CallMethodAsync messages.

To simplify code in CallMethod() and make it more clean, BTNCLICK was separated to ONMOUSEDOWN and ONMOUSEUP, only addition is check for consistency between sender BView and mView.

Also nsAppShell was adjusted according to new reality - new case names, less arguments in ConsumeRedundantMouseEvent to test.

Comment 35

[Doug Shelton]

many changes to nsWindow since 2005-12-11 made the last patch unuseable.  Revising this one is beyond my skills.

Comment 36

[Sergei Dolgov]

http://lxr.mozilla.org/seamonkey/source/content/html/document/src/nsHTMLContentSink.cpp#3343

Comment 37

[Sergei Dolgov]

Attachment: patch

One of possible versions for tigerdog's (Doug Shelton) convinience - created against current tree.

Comment 38

[Sergei Dolgov]

Heh, Haiku OS now are discussing this problem too, with ideas to "stop" mousemove flood internally, at OS level, with addition of special flag for that to BWindow constructor and methods.

That's really irritating problem

Comment 39

[Doug Shelton]

some issues never go away...

Comment 40

[Sylvestre Ledru [:Sylvestre]]

Nothing has been happening on the beos side for a while, closing.