Created attachment 123656 [details] [review]
Bug 28833: Speed up holds queue builder via parallel processing

The holds queue builder can take a very long time to run on systems with many holds. For example, a partner with 124,784 unfilled ( not found ) holds, is taking about 64 minutes to run. If we run that same number of holds in 5 parallel chunks ( splitting the number of records as evenly as possible, but *not* taking into account the holds per bib ), it takes 21.5 minutes. If we use 10 loops, it takes less then 14 minutes.

This patch provides a tremendous speedup for large numbers of holds. However, it causes HoldsQueue.t to fail spectacularly.

Created attachment 123657 [details] [review]
Bug 28833: Speed up holds queue builder via parallel processing

The holds queue builder can take a very long time to run on systems with many holds. For example, a partner with 124,784 unfilled ( not found ) holds, is taking about 64 minutes to run. If we run that same number of holds in 5 parallel chunks ( splitting the number of records as evenly as possible, but *not* taking into account the holds per bib ), it takes 21.5 minutes. If we use 10 loops, it takes less then 14 minutes.

Created attachment 123659 [details] [review]
Bug 28833: Speed up holds queue builder via parallel processing

The holds queue builder can take a very long time to run on systems with many holds. For example, a partner with 124,784 unfilled ( not found ) holds, is taking about 64 minutes to run. If we run that same number of holds in 5 parallel chunks ( splitting the number of records as evenly as possible, but *not* taking into account the holds per bib ), it takes 21.5 minutes. If we use 10 loops, it takes less then 14 minutes.

Created attachment 123671 [details] [review]
Bug 28833: Speed up holds queue builder via parallel processing

The holds queue builder can take a very long time to run on systems with many holds. For example, a partner with 124,784 unfilled ( not found ) holds, is taking about 64 minutes to run. If we run that same number of holds in 5 parallel chunks ( splitting the number of records as evenly as possible, but *not* taking into account the holds per bib ), it takes 21.5 minutes. If we use 10 loops, it takes less then 14 minutes.

Test Plan:
1) Generate a huge number of holds ( a few thousand at the minimum )
2) Run the holds queue builder, use the `time` utility to track how much
   time it took to run
3) Set HoldsQueueParallelLoopsCount to 10
4) Repeat step 2, note the improvement in speed
5) Experiment with other values for HoldsQueueParallelLoopsCount
6) prove t/db_dependent/HoldsQueue.t

Created attachment 123679 [details] [review]
Bug 28833: Speed up holds queue builder via parallel processing

The holds queue builder can take a very long time to run on systems with many holds. For example, a partner with 124,784 unfilled ( not found ) holds, is taking about 64 minutes to run. If we run that same number of holds in 5 parallel chunks ( splitting the number of records as evenly as possible, but *not* taking into account the holds per bib ), it takes 21.5 minutes. If we use 10 loops, it takes less then 14 minutes.

Test Plan:
0) Install the Perl library Parallel::Loops
1) Generate a huge number of holds ( a few thousand at the minimum )
2) Run the holds queue builder, use the `time` utility to track how much
   time it took to run
3) Set HoldsQueueParallelLoopsCount to 10
4) Repeat step 2, note the improvement in speed
5) Experiment with other values for HoldsQueueParallelLoopsCount
6) prove t/db_dependent/HoldsQueue.t

Created attachment 123688 [details] [review]
Bug 28833: Speed up holds queue builder via parallel processing

The holds queue builder can take a very long time to run on systems with many holds. For example, a partner with 124,784 unfilled ( not found ) holds, is taking about 64 minutes to run. If we run that same number of holds in 5 parallel chunks ( splitting the number of records as evenly as possible, but *not* taking into account the holds per bib ), it takes 21.5 minutes. If we use 10 loops, it takes less then 14 minutes.

Test Plan:
0) Install the Perl library Parallel::ForkManager
1) Generate a huge number of holds ( a few thousand at the minimum )
2) Run the holds queue builder, use the `time` utility to track how much
   time it took to run
3) Set HoldsQueueParallelLoopsCount to 10
4) Repeat step 2, note the improvement in speed
5) Experiment with other values for HoldsQueueParallelLoopsCount
6) prove t/db_dependent/HoldsQueue.t

Created attachment 123689 [details] [review]
Bug 28833: Speed up holds queue builder via parallel processing

The holds queue builder can take a very long time to run on systems with many holds. For example, a partner with 124,784 unfilled ( not found ) holds, is taking about 64 minutes to run. If we run that same number of holds in 5 parallel chunks ( splitting the number of records as evenly as possible, but *not* taking into account the holds per bib ), it takes 21.5 minutes. If we use 10 loops, it takes less then 14 minutes.

Test Plan:
0) Install the Perl library Parallel::ForkManager
1) Generate a huge number of holds ( a few thousand at the minimum )
2) Run the holds queue builder, use the `time` utility to track how much
   time it took to run
3) Set HoldsQueueParallelLoopsCount to 10
4) Repeat step 2, note the improvement in speed
5) Experiment with other values for HoldsQueueParallelLoopsCount
6) prove t/db_dependent/HoldsQueue.t

This is an interesting one! 

On one hand, I like the use of a system preference to allow the value to be set on a per-instance basis.

On the other hand, letting a library define an unrestricted number of forks for a script seems potentially dangerous, especially if you have multiple Koha instances on the same server.

I think this change needs more sanity checks.

Some options:
1) A CLI override that doesn't let the $loops be beyond X. This allows a sysadmin to constrain the resource usage
2) Perhaps the size of $bibs_with_pending_requests should correlate with the number of forks up to a maximum. (If you only have 10 bibs but your $loops is 10, it doesn't make sense to have 10 forks of 1 bib each. )
3) Maybe we should limit nproc in /etc/security/limits.d on a per-user basis (but maybe this is best left to local sysadmins and this is a very blunt instrument)

Actually, we should also do a int() check on $loops before feeding it to Parallel::ForkManager, since it is user-provided input. (I suppose a non-number would just cause a fatal error on a $loops > 1 check, but better to not fork than to fail.)

Another option might be to use koha-conf.xml instead of a system preference. 

I think it's important for it to be a configurable value per-instance, but I'm opposed to the idea of library staff being able to determine server resource usage.

The following is totally out of scope but inspired by the above:

What if default values in koha-conf.xml were determined based on a "profile" provided for koha-create? 

By default, we'd have permissive values, so that inexperienced people could have the most power possible through the Web UI, since they might not have a sysadmin to update the backend. 

But we'd have a more restrictive profile that vendors and more experienced people could use to have more locked down/secure Koha instances.

I'm trying to think of an approach that could make everyone happy...

Created attachment 123783 [details] [review]
Bug 28833: (follow-up) Move loops count control to koha conf

Created attachment 123784 [details] [review]
Bug 28833: (follow-up) Only spawn multiple processes if there are more records than loops specified

Created attachment 123785 [details] [review]
Bug 28833: (follow-up) Move loops count control to koha conf

Created attachment 123786 [details] [review]
Bug 28833: (follow-up) Only spawn multiple processes if there are more records than loops specified

Comment on attachment 123786 [details] [review]
Bug 28833: (follow-up) Only spawn multiple processes if there are more records than loops specified

Review of attachment 123786 [details] [review]:
-----------------------------------------------------------------

::: C4/HoldsQueue.pm
@@ +201,4 @@
>      # Split the list of bibs into groups to run in parallel
>      if ( $loops > 1 ) {
>          my $bibs_per_chunk = ceil( scalar @$bibs_with_pending_requests / $loops );
> +        $bibs_per_chunk = @$bibs_with_pending_requests if @$bibs_with_pending_requests <= $bibs_per_chunk;

I don't understand this logic. 

scalar @$bibs_with_pending_requests = 10
$loops = 10
$bibs_per_chunk =  1

$bibs_per_chunk would still be 1.

Maybe I'm confused but I can't think of a case where $bibs_per_chunk could be greater than scalar @$bibs_with_pending_requests.

--

I was thinking we'd do the $loops would be changed to $max_loops, and $loops would be based on an arbitrary ratio. Perhaps 1000 bibs per 1 loop. 

So $loops = scalar @$bibs_with_pending_requests / 1000. 

If $loops < 1, then we don't fork. If $loops > 1, we double-check that we're not exceeding our maximum number of loops. 

For example:
$max_loops = 4 
scalar @$bibs_with_pending_requests = 10000
$loops = 10

We throttle $loops down from 10 to 4, because 4 is our maximum. 

Likewise, if @$bibs_with_pending_requests = 2000, we're only using 2 forks instead of our maximum of 4, so we're not using more resources than necessary. 

--

Alternatively, we could have an arbitrary threshold of 1000. Below that and we use $loops = 1. Above that, we use the $loops value supplied from config. 

I'm just trying to avoid scenarios where you have 50 Koha instances each spinning up 10 child processes to process 2 requests each. 

But maybe I'm just prematurely optimizing. I suppose most instances will have the default of 1 anyway.

Comment on attachment 123689 [details] [review]
Bug 28833: Speed up holds queue builder via parallel processing

Review of attachment 123689 [details] [review]:
-----------------------------------------------------------------

::: C4/HoldsQueue.pm
@@ +210,5 @@
> +
> +        DATA_LOOP:
> +        foreach my $chunk (@chunks) {
> +            my $pid = $pm->start and next DATA_LOOP;
> +            _ProcessBiblios($chunk);

I notice that the other _ProcessBiblios has arguments for $branches_to_use and $transport_cost_matrix. I'm guessing those should be included here too?

Created attachment 123996 [details] [review]
Bug 28833: Speed up holds queue builder via parallel processing

The holds queue builder can take a very long time to run on systems with many holds. For example, a partner with 124,784 unfilled ( not found ) holds, is taking about 64 minutes to run. If we run that same number of holds in 5 parallel chunks ( splitting the number of records as evenly as possible, but *not* taking into account the holds per bib ), it takes 21.5 minutes. If we use 10 loops, it takes less then 14 minutes.

Test Plan:
0) Install the Perl library Parallel::ForkManager
1) Generate a huge number of holds ( a few thousand at the minimum )
2) Run the holds queue builder, use the `time` utility to track how much
   time it took to run
3) Set HoldsQueueParallelLoopsCount to 10
4) Repeat step 2, note the improvement in speed
5) Experiment with other values for HoldsQueueParallelLoopsCount
6) prove t/db_dependent/HoldsQueue.t

(In reply to David Cook from comment #17)
> Comment on attachment 123689 [details] [review] [review]
> Bug 28833: Speed up holds queue builder via parallel processing
> 
> Review of attachment 123689 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: C4/HoldsQueue.pm
> @@ +210,5 @@
> > +
> > +        DATA_LOOP:
> > +        foreach my $chunk (@chunks) {
> > +            my $pid = $pm->start and next DATA_LOOP;
> > +            _ProcessBiblios($chunk);
> 
> I notice that the other _ProcessBiblios has arguments for $branches_to_use
> and $transport_cost_matrix. I'm guessing those should be included here too?

Good catch! I've fixed that issue and squashed the patches down for ease of use.

Created attachment 126200 [details] [review]
Bug 28833: Balance the number of holds each chunk contains

Created attachment 126202 [details] [review]
Bug 28833: Speed up holds queue builder via parallel processing

The holds queue builder can take a very long time to run on systems with many holds. For example, a partner with 124,784 unfilled ( not found ) holds, is taking about 64 minutes to run. If we run that same number of holds in 5 parallel chunks ( splitting the number of records as evenly as possible, but *not* taking into account the holds per bib ), it takes 21.5 minutes. If we use 10 loops, it takes less then 14 minutes.

Test Plan:
0) Install the Perl library Parallel::ForkManager
1) Generate a huge number of holds ( a few thousand at the minimum )
2) Run the holds queue builder, use the `time` utility to track how much
   time it took to run
3) Set HoldsQueueParallelLoopsCount to 10
4) Repeat step 2, note the improvement in speed
5) Experiment with other values for HoldsQueueParallelLoopsCount
6) prove t/db_dependent/HoldsQueue.t

Created attachment 126216 [details] [review]
Bug 28833: Speed up holds queue builder via parallel processing

The holds queue builder can take a very long time to run on systems with many holds. For example, a partner with 124,784 unfilled ( not found ) holds, is taking about 64 minutes to run. If we run that same number of holds in 5 parallel chunks ( splitting the number of records as evenly as possible, but *not* taking into account the holds per bib ), it takes 21.5 minutes. If we use 10 loops, it takes less then 14 minutes.

Test Plan:
0) Install the Perl library Parallel::ForkManager
1) Generate a huge number of holds ( a few thousand at the minimum )
2) Run the holds queue builder, use the `time` utility to track how much
   time it took to run
3) Set HoldsQueueParallelLoopsCount to 10
4) Repeat step 2, note the improvement in speed
5) Experiment with other values for HoldsQueueParallelLoopsCount
6) prove t/db_dependent/HoldsQueue.t

As expected, since this has been in the sign off queue for a while, the patch no longer applies 8-(...

Is there a way to generate a large number of holds? Either an SQL query or a script.


kohadev-koha@kohadevbox:koha(bz28833)$ git bz apply 28833

Bug 28833 - Speed up holds queue builder via parallel processing

126216 - Bug 28833: Speed up holds queue builder via parallel processing

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 28833: Speed up holds queue builder via parallel processing
Using index info to reconstruct a base tree...
M	C4/HoldsQueue.pm
M	debian/templates/koha-conf-site.xml.in
M	etc/koha-conf.xml
M	misc/cronjobs/holds/build_holds_queue.pl
Falling back to patching base and 3-way merge...
Auto-merging misc/cronjobs/holds/build_holds_queue.pl
CONFLICT (content): Merge conflict in misc/cronjobs/holds/build_holds_queue.pl
Auto-merging etc/koha-conf.xml
CONFLICT (content): Merge conflict in etc/koha-conf.xml
Auto-merging debian/templates/koha-conf-site.xml.in
CONFLICT (content): Merge conflict in debian/templates/koha-conf-site.xml.in
Auto-merging C4/HoldsQueue.pm
CONFLICT (content): Merge conflict in C4/HoldsQueue.pm
error: Failed to merge in the changes.
Patch failed at 0001 Bug 28833: Speed up holds queue builder via parallel processing

Created attachment 167349 [details] [review]
Bug 28833: Speed up holds queue builder via parallel processing

The holds queue builder can take a very long time to run on systems with many holds. For example, a partner with 124,784 unfilled ( not found ) holds, is taking about 64 minutes to run. If we run that same number of holds in 5 parallel chunks ( splitting the number of records as evenly as possible, but *not* taking into account the holds per bib ), it takes 21.5 minutes. If we use 10 loops, it takes less then 14 minutes.

Test Plan:
0) Install the Perl library Parallel::ForkManager
1) Generate a huge number of holds ( a few thousand at the minimum )
2) Run the holds queue builder, use the `time` utility to track how much
   time it took to run
3) Set HoldsQueueParallelLoopsCount to 10
4) Repeat step 2, note the improvement in speed
5) Experiment with other values for HoldsQueueParallelLoopsCount
6) prove t/db_dependent/HoldsQueue.t

Thanks Kyle for making the patch apply!

Is there an easy way to generate a large number of holds? Either an SQL query or a script.

David

(In reply to David Nind from comment #25)
> Thanks Kyle for making the patch apply!
> 
> Is there an easy way to generate a large number of holds? Either an SQL
> query or a script.
> 
> David

This script will generate 100 holds:

#!/usr/bin/perl
use t::lib::TestBuilder;
my $builder = t::lib::TestBuilder->new;

for ( my $i = 0 ; $i < 100 ; $i++ ) {
    my $hold = $builder->build( { source => 'Reserve' } );
}

Hope this helps!

Test notes:

 > This script will generate 100 holds:
> 
> #!/usr/bin/perl
> use t::lib::TestBuilder;
> my $builder = t::lib::TestBuilder->new;
> 
> for ( my $i = 0 ; $i < 100 ; $i++ ) {
>     my $hold = $builder->build( { source => 'Reserve' } );
> }
> 

I ran this script 10+ times. I noticed that in addition to holds it also makes new patrons, branches, bibs and itemtypes, probably other stuff too. cool!

I could see 1000+ records in the reserves table, but if I went to a patron record where a hold was created by the script the holds table wouldn't load, with error 500. I wasn't able to build the holds queue.

Also the bib detail page returned the error:
DBIC result _type  isn't of the _type BiblioMetadata at /kohadevbox/koha/catalogue/detail.pl line 105.
 at /usr/lib/x86_64-linux-gnu/perl-base/Carp.pm line 289

I think the next step needs to be to reindex for the new bibs, so I tried:
koha-rebuild-zebra -f --force -v kohadev

Something is not working to reindex the bibs created by the script though, once it gets past the ktd default bibs it returns errors like
.error retrieving biblio 3000 at /kohadevbox/koha/misc/migration_tools/rebuild_zebra.pl line 706.

Am I missing a step in the reindexing part? What step/s are required after generating the test data?

I was able to install Parallel::ForkManager and use the time utility, so I will be able to test again once I can resolve the data issue. Thanks!

The patch no longer applies for me 8-(...

git bz apply 28833

Bug 28833 - Speed up holds queue builder via parallel processing

167349 - Bug 28833: Speed up holds queue builder via parallel processing

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 28833: Speed up holds queue builder via parallel processing
Using index info to reconstruct a base tree...
M	C4/HoldsQueue.pm
M	debian/templates/koha-conf-site.xml.in
M	etc/koha-conf.xml
M	misc/cronjobs/holds/build_holds_queue.pl
Falling back to patching base and 3-way merge...
Auto-merging misc/cronjobs/holds/build_holds_queue.pl
CONFLICT (content): Merge conflict in misc/cronjobs/holds/build_holds_queue.pl
Auto-merging etc/koha-conf.xml
Auto-merging debian/templates/koha-conf-site.xml.in
Auto-merging C4/HoldsQueue.pm
CONFLICT (content): Merge conflict in C4/HoldsQueue.pm
error: Failed to merge in the changes.
Patch failed at 0001 Bug 28833: Speed up holds queue builder via parallel processing

Is this one still relevant Kyle?

Created attachment 173045 [details] [review]
Bug 28833: Speed up holds queue builder via parallel processing

The holds queue builder can take a very long time to run on systems with many holds. For example, a partner with 124,784 unfilled ( not found ) holds, is taking about 64 minutes to run. If we run that same number of holds in 5 parallel chunks ( splitting the number of records as evenly as possible, but *not* taking into account the holds per bib ), it takes 21.5 minutes. If we use 10 loops, it takes less then 14 minutes.

Test Plan:
0) Install the Perl library Parallel::ForkManager
1) Generate a huge number of holds ( a few thousand at the minimum )
2) Run the holds queue builder, use the `time` utility to track how much
   time it took to run
3) Set HoldsQueueParallelLoopsCount to 10
4) Repeat step 2, note the improvement in speed
5) Experiment with other values for HoldsQueueParallelLoopsCount
6) prove t/db_dependent/HoldsQueue.t

Created attachment 173046 [details] [review]
Bug 28833: Speed up holds queue builder via parallel processing

The holds queue builder can take a very long time to run on systems with many holds. For example, a partner with 124,784 unfilled ( not found ) holds, is taking about 64 minutes to run. If we run that same number of holds in 5 parallel chunks ( splitting the number of records as evenly as possible, but *not* taking into account the holds per bib ), it takes 21.5 minutes. If we use 10 loops, it takes less then 14 minutes.

Test Plan:
0) Install the Perl library Parallel::ForkManager
1) Generate a huge number of holds ( a few thousand at the minimum )
2) Run the holds queue builder, use the `time` utility to track how much
   time it took to run
3) Set HoldsQueueParallelLoopsCount to 10
4) Repeat step 2, note the improvement in speed
5) Experiment with other values for HoldsQueueParallelLoopsCount
6) prove t/db_dependent/HoldsQueue.t

Created attachment 173262 [details] [review]
Bug 28833: Speed up holds queue builder via parallel processing

The holds queue builder can take a very long time to run on systems with many holds. For example, a partner with 124,784 unfilled ( not found ) holds, is taking about 64 minutes to run. If we run that same number of holds in 5 parallel chunks ( splitting the number of records as evenly as possible, but *not* taking into account the holds per bib ), it takes 21.5 minutes. If we use 10 loops, it takes less then 14 minutes.

Test Plan:
0) Install the Perl library Parallel::ForkManager
1) Generate a huge number of holds ( a few thousand at the minimum )
2) Run the holds queue builder, use the `time` utility to track how much
   time it took to run
3) Set HoldsQueueParallelLoopsCount to 10
4) Repeat step 2, note the improvement in speed
5) Experiment with other values for HoldsQueueParallelLoopsCount
6) prove t/db_dependent/HoldsQueue.t

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

This works well and I see no regressions.

Passing QA

Wow, old style database update, reformatted to use the newer template on review.
I verified that the holds queue is still built running the script, but didn't test the parallel processing. Trusting the process :)

Pushed for 24.11!

Well done everyone, thank you!

added release notes

Enhancement, no 24.05.x backport

Changed status to needs documenting - new system preference that needs documenting.