martymac / fpart

Sort files and pack them into partitions
https://www.fpart.org/
BSD 2-Clause "Simplified" License
230 stars 39 forks source link

fpsync: start all idle workers before refresh #40

Closed jcphill closed 2 years ago

jcphill commented 2 years ago

There is no reason to call work_list_refresh after every job start; only call it on loop iterations when more idle workers are needed.

martymac commented 2 years ago

Hello Jim,

Thanks for your patch.

Function work_list_refresh() updates ${WORK_NUM}.

Your patch would lead to updating ${WORK_NUM} only when ${WORK_NUM} >= ${OPT_JOBS} but that is not always the case. We need to detect finished jobs when ${WORK_NUM} < ${OPT_JOBS} too. I can see two cases :

With your patch, when there are more workers available than jobs, you skip detecting finished jobs and last ${OPT_JOBS} will silently terminate without being detected.

jcphill commented 2 years ago

Thank you for your reply. Note that the loop exits based on line 918: while [ "${_NEXT}" != "fp_done" ] && [ "${_NEXT}" != "sl_stop" ] so work_list_refresh() is not called more than once after the job_queue_next() returns fp_done or sl_stop, and instead wait is called at line 954 to detect when the remaining jobs have finished. ${WORK_NUM} and ${WORK_LIST} are not used later.

martymac commented 2 years ago

Hello again,

Good catch ! I have overlooked my code, sorry. So everything seems good to me :)

Thanks for your contribution !

Best regards,

Ganael.

jcphill commented 2 years ago

Thanks. Since work_list_refresh() is O(WORK_NUM) it could be a bottleneck when called after every job launch and not all workers were kept busy. With this change I see a substantial performance increase with 80 workers.

martymac commented 2 years ago

That's right! Thanks again for your feedback!