Closed
Bug 312755
Opened 19 years ago
Closed 6 years ago
[BeOS] To reduce mousemove flood
Categories
(Core Graveyard :: Widget: BeOS, defect)
Tracking
(Not tracked)
RESOLVED
WONTFIX
People
(Reporter: sergei_d, Assigned: sergei_d)
Details
Attachments
(2 files, 4 obsolete files)
11.81 KB,
patch
|
Details | Diff | Splinter Review | |
7.56 KB,
patch
|
Details | Diff | Splinter Review |
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.
I think we can rely a lot less on callmethod at all if we handle locking correctly. See what windows uses callmethod for.
Assignee | ||
Comment 2•19 years ago
|
||
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.
Assignee | ||
Comment 4•19 years ago
|
||
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.
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.
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.
Assignee | ||
Comment 7•19 years ago
|
||
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));
Assignee | ||
Comment 8•19 years ago
|
||
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
Assignee | ||
Comment 9•19 years ago
|
||
"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•19 years ago
|
||
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.
Assignee | ||
Comment 11•19 years ago
|
||
"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•19 years ago
|
||
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.
Assignee | ||
Comment 13•19 years ago
|
||
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•19 years ago
|
||
Ok, if we can't discuss thing you're on your own on this one. (And I don't like to be called arrogant.)
Assignee | ||
Comment 15•19 years ago
|
||
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•19 years ago
|
||
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.
Assignee | ||
Comment 17•19 years ago
|
||
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.
Assignee | ||
Comment 18•19 years ago
|
||
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•19 years ago
|
||
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.
Assignee | ||
Comment 20•19 years ago
|
||
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?
Assignee | ||
Comment 21•19 years ago
|
||
reduced internal version - CallMethod was even squeezed, not increased. special care about MouseUp
Assignee | ||
Comment 22•19 years ago
|
||
again, last patch is also only topic for discussion, as it is still rather buggy.
Comment 23•19 years ago
|
||
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
Assignee | ||
Comment 24•19 years ago
|
||
context menu handling restored over previous version
Attachment #199838 -
Attachment is obsolete: true
Attachment #200335 -
Attachment is obsolete: true
Assignee | ||
Comment 25•19 years ago
|
||
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•19 years ago
|
||
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.
Assignee | ||
Comment 27•19 years ago
|
||
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.
Assignee | ||
Comment 28•19 years ago
|
||
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.
Assignee | ||
Comment 29•19 years ago
|
||
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
Attachment #200336 -
Attachment is obsolete: true
Assignee | ||
Comment 30•19 years ago
|
||
left button fixed
Assignee | ||
Comment 31•19 years ago
|
||
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?
Assignee | ||
Comment 32•19 years ago
|
||
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.
Assignee | ||
Comment 33•19 years ago
|
||
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.
Assignee | ||
Updated•19 years ago
|
Summary: [BeOS] To reduce native mouse events size → [BeOS] To reduce mousemove flood
Assignee | ||
Comment 34•19 years ago
|
||
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.
Attachment #200353 -
Attachment is obsolete: true
Comment 35•19 years ago
|
||
many changes to nsWindow since 2005-12-11 made the last patch unuseable. Revising this one is beyond my skills.
Assignee | ||
Comment 36•19 years ago
|
||
http://lxr.mozilla.org/seamonkey/source/content/html/document/src/nsHTMLContentSink.cpp#3343
Assignee | ||
Comment 37•18 years ago
|
||
One of possible versions for tigerdog's (Doug Shelton) convinience - created against current tree.
Assignee | ||
Comment 38•16 years ago
|
||
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•16 years ago
|
||
some issues never go away...
Updated•10 years ago
|
Product: Core → Core Graveyard
Comment 40•6 years ago
|
||
Nothing has been happening on the beos side for a while, closing.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → WONTFIX
You need to log in
before you can comment on or make changes to this bug.
Description
•