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
