Re: WIP: new VCL scheduler feature branch
On 2016/09/19 2:27 PM, Jan-Marek Glogowski wrote: Some random low priority comments: In "Just walk the task list once per timeout" https://cgit.freedesktop.org/libreoffice/core/commit/?h=feature/new-vcl-scheduler=435e21fe0330436e76e5e053d5d5d94df734a554 The evaluate_entry label doesn't make the code any easier to read. So you're suggestion is? Introduce an other level of indention for the if? The while loop is short enough and the goto labels are just used in there. If you think it improves readability, I can change it. No, just inline that single line of code at the evaluate_entry label and then jump to the exit label. But that's just my taste, if you disagree, feel free to ignore. In "Reorganize Scheduler priority classes" https://cgit.freedesktop.org/libreoffice/core/commit/?h=feature/new-vcl-scheduler=5e4361e84607fc6d7623b31630505da7c934b945 (1) you haven't used a consistent mapping from old to new - sometimes LOW maps to HIGH_IDLE, sometimes HIGHEST maps to HIGH_IDLE. Perhaps there is a reason for this? From naming and code reading I tried to guess, if the idle should be processed before drawing. As I wrote I would like to get a comment or annotation for all non-default priorities, i.e. all calls to "SetPriority". I have no idea, if my guesses are correct, just that mail merge is much faster now and LO is still usable while the mail merge runs. Actually you can see the slowdown of mail merge when doing stuff in LO :-) OK, just checking, because from the commit message it looks like a mechanical change, but then the mapping was not consistent. (2) I would call HIGH_IDLE, either PRE_RESIZE or BEFORE_RESIZE, because it's not any kind of IDLE anymore. I would call DEFAULT_IDLE just IDLE. I just took the priority names from glib. I would keep the DEFAULT_IDLE, or rename DEFAULT to TIMER? OTOH the Scheduler class sets the default priority... Ah, then probably leave them alone, if they map to something we already use. ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: WIP: new VCL scheduler feature branch
Hi Noel, thanks for the feedback. Am 17.09.2016 um 13:54 schrieb Noel Grandin: > Nice work! > > Some random low priority comments: > > In "Just walk the task list once per timeout" > https://cgit.freedesktop.org/libreoffice/core/commit/?h=feature/new-vcl-scheduler=435e21fe0330436e76e5e053d5d5d94df734a554 > The evaluate_entry label doesn't make the code any easier to read. So you're suggestion is? Introduce an other level of indention for the if? The while loop is short enough and the goto labels are just used in there. If you think it improves readability, I can change it. > In "Reorganize Scheduler priority classes" > https://cgit.freedesktop.org/libreoffice/core/commit/?h=feature/new-vcl-scheduler=5e4361e84607fc6d7623b31630505da7c934b945 > (1) you haven't used a consistent mapping from old to new - sometimes > LOW maps to HIGH_IDLE, sometimes HIGHEST maps to HIGH_IDLE. Perhaps > there is a reason for this? From naming and code reading I tried to guess, if the idle should be processed before drawing. As I wrote I would like to get a comment or annotation for all non-default priorities, i.e. all calls to "SetPriority". I have no idea, if my guesses are correct, just that mail merge is much faster now and LO is still usable while the mail merge runs. Actually you can see the slowdown of mail merge when doing stuff in LO :-) > (2) I would call HIGH_IDLE, either PRE_RESIZE or BEFORE_RESIZE, because > it's not any kind of IDLE anymore. I would call DEFAULT_IDLE just IDLE. I just took the priority names from glib. I would keep the DEFAULT_IDLE, or rename DEFAULT to TIMER? OTOH the Scheduler class sets the default priority... > In "Handle all main loop and task events" > https://cgit.freedesktop.org/libreoffice/core/commit/?h=feature/new-vcl-scheduler=102c41c2e429bee489334361536779aa298bc181 > -bProcessedEvent = bProcessedEvent || bScheduledEevent; > +bProcessedEvent |= bScheduledEevent; Will do. > In "Run Idle tasks immediatly" > https://cgit.freedesktop.org/libreoffice/core/commit/?h=feature/new-vcl-scheduler=a1ecb280872c5487615558f8d140a380ef3e0d36 > It occurs to me that when we get an overload condition, it would be very > helpful to dump the names of the current events. Dumping the current event would probably not help much. But we could dump the whole scheduler event state. OTOH you can dump it in GDB with p *ImplGetSVData()->mpFirstSchedulerData BTW: there are some LOK-Tests, which generate more then 1000 events. The 1000 is arbitrary from my POV. Probably checking the runtime of ProcessAllPendingEvents() would be better? Thanks, Jan-Marek ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: WIP: new VCL scheduler feature branch
Nice work! Some random low priority comments: In "Just walk the task list once per timeout" https://cgit.freedesktop.org/libreoffice/core/commit/?h=feature/new-vcl-scheduler=435e21fe0330436e76e5e053d5d5d94df734a554 The evaluate_entry label doesn't make the code any easier to read. In "Reorganize Scheduler priority classes" https://cgit.freedesktop.org/libreoffice/core/commit/?h=feature/new-vcl-scheduler=5e4361e84607fc6d7623b31630505da7c934b945 (1) you haven't used a consistent mapping from old to new - sometimes LOW maps to HIGH_IDLE, sometimes HIGHEST maps to HIGH_IDLE. Perhaps there is a reason for this? (2) I would call HIGH_IDLE, either PRE_RESIZE or BEFORE_RESIZE, because it's not any kind of IDLE anymore. I would call DEFAULT_IDLE just IDLE. In "Handle all main loop and task events" https://cgit.freedesktop.org/libreoffice/core/commit/?h=feature/new-vcl-scheduler=102c41c2e429bee489334361536779aa298bc181 -bProcessedEvent = bProcessedEvent || bScheduledEevent; +bProcessedEvent |= bScheduledEevent; In "Run Idle tasks immediatly" https://cgit.freedesktop.org/libreoffice/core/commit/?h=feature/new-vcl-scheduler=a1ecb280872c5487615558f8d140a380ef3e0d36 It occurs to me that when we get an overload condition, it would be very helpful to dump the names of the current events. ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/libreoffice
WIP: new VCL scheduler feature branch
Hi everybody, finally I have a working solution, which doesn't instantly show bugs on Linux with KDE4 (feature/new-vcl-scheduler). This fixes my mail merge performance problem since the 5.0 merge of idle job handling. It completely drops the idea of separated idle and timer handling and drops a lot of special handling and workarounds for problems in the original code, but keeps the Idle class for convenience. I tried to list all the revert commit ids and started to test the original bugs. Everything is just scheduled by priority and Idles now get a very low one per default, while they previously had the same one, then timers. Additionally it drops the 1ms lag for tasks, which actually adds up, as idle tasks weren't instantly scheduled. I already fixed the two most obvious busy loop bugs in Writer with workarounds for the general idle job handler and the statistics collector. Instead of polling for work I would like to switch these to be enabled on demand, but that'll definitely take more work. Currently they are converted to timers, which check for work every few seconds, then should switch to idle - ok 0ms timeouts, until all work is processed. I tried to simplify the priority groups to make it easier to select the correct one. There are still some suspicios HIGHEST ones. I had to guess the new dependencies from the original priority of the idles, but I'm quite sure there is some missing stuff. I would like to get some kind of annotations to be able to build a priority tree of all of them for a better overview. It would be great to get feedback from people of all other platforms / VCL backends. Thanks for your comments and feedback Jan-Marek P.S. it adds a VCL python GDB script, which can be used to dump the list of currently scheduled tasks. P.P.S. naming is sill a mess. Sometimes it's task, sometimes event. The scheduler class is actually the base class for the timers and idles and should be renamed Event or Task. ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/libreoffice
