Bug 28413 - background job worker is running with all the modules in RAM
Summary: background job worker is running with all the modules in RAM
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on: 22417 27756
Blocks: 28410
  Show dependency treegraph
 
Reported: 2021-05-21 11:56 UTC by Jonathan Druart
Modified: 2021-11-15 11:42 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Text to go in the release notes:
Version(s) released in:
21.11.00


Attachments
Bug 28413: Reduce background job worker memory footprint (1.79 KB, patch)
2021-05-21 12:10 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28413: Reduce background job worker memory footprint (1.87 KB, patch)
2021-09-28 08:43 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28413: Reduce background job worker memory footprint (1.26 KB, patch)
2021-10-08 08:38 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28413: Reduce background job worker memory footprint (1.35 KB, patch)
2021-10-29 09:09 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28413: Reduce background job worker memory footprint (1.40 KB, patch)
2021-11-05 03:11 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 28413: (QA follow-up) Fix for missing use Koha::Items (691 bytes, patch)
2021-11-12 21:50 UTC, Andrew Nugged
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2021-05-21 11:56:16 UTC
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).
Comment 1 Jonathan Druart 2021-05-21 12:10:10 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).
Comment 2 David Cook 2021-05-24 00:40:07 UTC
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..
Comment 3 Jonathan Druart 2021-05-24 07:31:54 UTC
Yes, that's for bug 27434.
Comment 4 Jonathan Druart 2021-07-06 09:25:32 UTC
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).
Comment 5 David Cook 2021-07-29 04:33:30 UTC
On my radar but I'm catching up on a backlog atm.
Comment 6 Martin Renvoize 2021-09-28 08:43:14 UTC
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>
Comment 7 Martin Renvoize 2021-09-28 08:44:43 UTC
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.
Comment 8 David Cook 2021-10-08 05:59:41 UTC
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.
Comment 9 Jonathan Druart 2021-10-08 08:38:50 UTC
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>
Comment 10 Jonathan Druart 2021-10-08 08:44:51 UTC Comment hidden (obsolete)
Comment 11 Jonathan Druart 2021-10-08 08:45:34 UTC
VIRT    RES    SHR
200880 145832  25496 # Before
104196  60224  15192 # After
Comment 12 David Nind 2021-10-29 08:27:50 UTC
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
Comment 13 Jonathan Druart 2021-10-29 09:09:40 UTC
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>
Comment 14 Jonathan Druart 2021-10-29 09:10:40 UTC
This has been signed off actually and is waiting for QA.

Thanks David, you can still test it if you want to! :)
Comment 15 David Nind 2021-10-29 09:19:53 UTC
I wondered about that (whether it was already signed off)... 

I think Martin signing it off is more than sufficient! 8-)
Comment 16 Victor Grousset/tuxayo 2021-11-05 03:11:14 UTC
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>
Comment 17 Victor Grousset/tuxayo 2021-11-05 03:11:47 UTC
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.
Comment 18 Jonathan Druart 2021-11-05 08:08:51 UTC
(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.
Comment 19 Jonathan Druart 2021-11-05 11:14:47 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 20 Andrew Nugged 2021-11-12 21:43:28 UTC
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.
Comment 21 Andrew Nugged 2021-11-12 21:50:04 UTC
Created attachment 127598 [details] [review]
Bug 28413: (QA follow-up) Fix for missing use Koha::Items
Comment 22 Jonathan Druart 2021-11-15 11:42:26 UTC
(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!