|
Description
Jonathan Druart
2021-05-21 11:56:16 UTC
Created attachment 121274 [details] [review] Bug 28413: Reduce background job worker memory footprint It's loading the modules in RAM then forks. Then do not need it to have them loaded in the parent process (the worker/daemon). I think that this is an improvement, but it would be nice to not have the job type hard-coded into the process method. Personally, I'd like the job type to either be the fully qualified module name (e.g. Koha::BackgroundJob::BatchUpdateBiblio) or a shortened version (e.g. BatchUpdateBiblio), so that Koha::BackgroundJob::process() could to "load $job_type;" (using Module::Load) and "$job_type->process($args)". But perhaps that's out of scope for this change.. Test plan: Confirm the difference of memory allocated to the background job worker with and without the patch. Confirm the it's still working correctly after you applied the patch (batch modify some biblios for instance). On my radar but I'm catching up on a backlog atm. Created attachment 125392 [details] [review] Bug 28413: Reduce background job worker memory footprint It's loading the modules in RAM then forks. Then do not need it to have them loaded in the parent process (the worker/daemon). Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Nice little improvement.. I too am looking forward to the next steps but I see no reason to hold this step up. Signing off.. tempted to go straight PQA as it's a trivially simple piece of code to understand.. but I'll er on the side of caution and poke a fellow QA team person to push it over the edge. Sorry for taking so long on this one. Patch doesn't apply anymore. As a side note, before applying the patch, it looks like VIRT 241.2m RES 175.3m SHR 19.2m, according to top. Created attachment 125941 [details] [review] Bug 28413: Reduce background job worker memory footprint It's loading the modules in RAM then forks. Then do not need it to have them loaded in the parent process (the worker/daemon). Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> VIRT RES SH 200880 145832 25496 # Before 104196 60224 15192 # After VIRT RES SHR 200880 145832 25496 # Before 104196 60224 15192 # After Patch no longer applies 8-(... Applying: Bug 28413: Reduce background job worker memory footprint Using index info to reconstruct a base tree... M Koha/BackgroundJob.pm Falling back to patching base and 3-way merge... Auto-merging Koha/BackgroundJob.pm CONFLICT (content): Merge conflict in Koha/BackgroundJob.pm error: Failed to merge in the changes. Patch failed at 0001 Bug 28413: Reduce background job worker memory footprint Created attachment 127081 [details] [review] Bug 28413: Reduce background job worker memory footprint It's loading the modules in RAM then forks. Then do not need it to have them loaded in the parent process (the worker/daemon). Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> This has been signed off actually and is waiting for QA. Thanks David, you can still test it if you want to! :) I wondered about that (whether it was already signed off)... I think Martin signing it off is more than sufficient! 8-) Created attachment 127329 [details] [review] Bug 28413: Reduce background job worker memory footprint It's loading the modules in RAM then forks. Then do not need it to have them loaded in the parent process (the worker/daemon). Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> works (tested on bib record, auth, item and hold cancel, ram usage decreased), QA script happy, code look ok. Passed! It's surprising that these classes take dozens of MB of RAM to load. (In reply to Victor Grousset/tuxayo from comment #17) > works (tested on bib record, auth, item and hold cancel, ram usage > decreased), QA script happy, code look ok. > > Passed! Thanks! > It's surprising that these classes take dozens of MB of RAM to load. If you load one, you load all. Pushed to master for 21.11, thanks to everybody involved! in https://git.koha-community.org/Koha-community/Koha/src/branch/master/Koha/BackgroundJob/BatchDeleteItem.pm#L113 my $item = Koha::Items->find($record_id) || next; it dies because was lost "use Koha::Items;" like here: https://git.koha-community.org/Koha-community/Koha/src/branch/master/Koha/BackgroundJob/BatchUpdateItem.pm#L33 since now packages are not included all at once, like before this ticket, when all modules were loaded by neighbors so the error was hidden. Created attachment 127598 [details] [review] Bug 28413: (QA follow-up) Fix for missing use Koha::Items (In reply to Andrew Nugged from comment #21) > Created attachment 127598 [details] [review] [review] > Bug 28413: (QA follow-up) Fix for missing use Koha::Items Pushed to master, thanks Andrew! I reckon we could keep reducing the memory footprint on background_jobs_worker.pl. When you have a lot of instances with at least 2 of those workers, the memory usage does add up. |