[Bug 17941] New: CanBookBeRenewed is very inefficient/slow

classic Classic list List threaded Threaded
54 messages Options
12
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 17941] New: CanBookBeRenewed is very inefficient/slow

bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17941

            Bug ID: 17941
           Summary: CanBookBeRenewed is very inefficient/slow
 Change sponsored?: ---
           Product: Koha
           Version: master
          Hardware: All
                OS: All
            Status: NEW
          Severity: major
          Priority: P5 - low
         Component: Hold requests
          Assignee: [hidden email]
          Reporter: [hidden email]
        QA Contact: [hidden email]
                CC: [hidden email]

I started investigating why populating the checkout list in the patron view
sometimes took ages (minutes), and on occations even causing the system to
almost halt because of DB overload. It turned out that reason is calls to
C4::Circulation::CanBookBeRenewed. The critical factor is whether an item has
many holds on it, and if there are many items that can fill the hold.

Briefly looking at the code it seems obvious that the time spent grows
exponentially with the number of holds and items.

As one example, checking if an item with 111 holds (and 12 items on the biblio)
can be renewed takes around 25 sec and generates over 18,000 SQL queries.

I can't help to think there must be some missed "early returns" opportunities
in the loops somewhere. For example, if there are more unfulfilled holds than
available items, can the item ever be renewed? If no, return before even begin
to calculate.. However, I know holds are full of complexities and things I
probably haven't thought about so I need to dig into the code more.

I would be very gratefull if anyone knowledgeable about this part of the code
could help me find some ways to optimize this!

--
You are receiving this mail because:
You are the assignee for the bug.
You are watching all bug changes.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 17941] CanBookBeRenewed is very inefficient/slow

bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17941

--- Comment #1 from Jonathan Druart <[hidden email]> ---
Created attachment 59740
  -->
https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=59740&action=edit
Try to recreate CanBookBeRenewed perf issue

--
You are receiving this mail because:
You are the assignee for the bug.
You are watching all bug changes.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 17941] CanBookBeRenewed is very inefficient/slow

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17941

Jonathan Druart <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[hidden email]-c
                   |                            |ommunity.org

--- Comment #2 from Jonathan Druart <[hidden email]> ---
Hi Petter,
I'd be interested to recreate the perf issue.
I have written this quick script, it generates 20 items, 200 patrons and
generate a hold for each patron.

On the second run, I comment everything but the last 2 lines and execute again
the script. It is executed in 1.5s

--
You are receiving this mail because:
You are the assignee for the bug.
You are watching all bug changes.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 17941] CanBookBeRenewed is very inefficient/slow

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17941

--- Comment #3 from Petter Goksøyr Åsen <[hidden email]> ---
Hi Jonathan. Thanks for looking into this!
We were actually working on this problem right now, so I have some information.
The code in question is quite complicated, so I'm not sure I understand it, but
it seems that the pathological cases are when most of the items are checked
out, so it needs to check every permutation of item, hold, and borrower to
check if it can be renewed. But I'm not 100% sure about it.

Rather than trying to understand how all the holds logic work in order to fix
this properly, we have made a patch now, that greatly improves the speed. This
is done by memoizing idempotent DB calls (GetItem, GetMembers and so on),
during the scope of CanBookBeRenewed. It helps a lot, because sometimes the
same item is fetched again and again, 100s to even 1000s of times from the DB!

In one case, by using the patch, the display of checkout tab with 5 items goes
from around 20 seconds to 3 seconds,

We do belive this patch is a good step forward, but we will test it extensivly
in staging environment before we submit it.

I'll try see if I can get some interesting result from your script.

--
You are receiving this mail because:
You are the assignee for the bug.
You are watching all bug changes.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 17941] CanBookBeRenewed is very inefficient/slow

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17941

--- Comment #4 from Jonathan Druart <[hidden email]> ---
(In reply to Petter Goksøyr Åsen from comment #3)
> We do belive this patch is a good step forward, but we will test it
> extensivly in staging environment before we submit it.

No do not wait, release as soon as you can :)

--
You are receiving this mail because:
You are watching all bug changes.
You are the assignee for the bug.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 17941] CanBookBeRenewed is very inefficient/slow

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17941

--- Comment #5 from Petter Goksøyr Åsen <[hidden email]> ---
(In reply to Jonathan Druart from comment #4)

> No do not wait, release as soon as you can :)

OK, for the adventurous:
https://gist.github.com/boutros/c3a63904c788d0a6336023042ce22cde

We recently hired a very experienced Perl guy, so he's behind this, not me:)
But it's very simple actually: just by decorating subroutines with ":
PureFunction" - you get memoization for free!

The actuall PureFunctions module is maybe not for the faint of heart, but it's
very small, and I do belive it works out very beautifull. And as this is not a
persistent cache, but only valid during the scope of the request, we don't have
to deal with cache invalidation and such problems.

The solution can easily be extended to other Getter-methods, as long as they
are idempotent (which they should be).

--
You are receiving this mail because:
You are watching all bug changes.
You are the assignee for the bug.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 17941] CanBookBeRenewed is very inefficient/slow

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17941

--- Comment #6 from Jonathan Druart <[hidden email]> ---
Hum, are you sure this is Plack-compatible?

--
You are receiving this mail because:
You are watching all bug changes.
You are the assignee for the bug.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 17941] CanBookBeRenewed is very inefficient/slow

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17941

--- Comment #7 from Petter Goksøyr Åsen <[hidden email]> ---
(In reply to Jonathan Druart from comment #6)
> Hum, are you sure this is Plack-compatible?

It should be. I've been testing under Plack today without experiencing any
issues, and it certainly seems to work fine. But I haven't profiled for memory
leaks yet.

--
You are receiving this mail because:
You are watching all bug changes.
You are the assignee for the bug.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 17941] CanBookBeRenewed is very inefficient/slow

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17941

Francesco Rivetti <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[hidden email]

--- Comment #8 from Francesco Rivetti <[hidden email]> ---
Hi! I can't think of any issue with plack or apache.

The only problem I had implementing it is the race with Exporter:

Attribute::Handlers apply attributes at CHECK time, while Exporter at BEGIN
time. I found a easy way to convince A::H to replace the methods at BEGIN time,
but I believe the order in the @ISA must be preserved.

But I don't consider this a proper fix, I just lack the experience to
understand the whole logic behind it yet.

--
You are receiving this mail because:
You are watching all bug changes.
You are the assignee for the bug.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 17941] CanBookBeRenewed is very inefficient/slow

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17941

--- Comment #9 from Petter Goksøyr Åsen <[hidden email]> ---
Some adittional evidence from testings Fransesco's patch:

Here are the stats of cache hits/misses from one call to /svc/checkouts (on a
patron with 5 checkouts, 2 of them which has many holds)

                                              hit   miss
C4::Circulation::GetBranchItemRule            4537  21
C4::Reserves::_OnShelfHoldsAllowed            13    116
C4::Reserves::IsAvailableForItemLevelRequest  0     129
C4::Circulation::CanBookBeRenewed             0     5
C4::Items::GetItem                            4845  33
C4::Members::GetMember                        4075  584
C4::Reserves::CheckReserves                   0     53
C4::Reserves::CanItemBeReserved               0     129
C4::Reserves::IsItemOnHoldAndFound            24    239

As you can see, we save almost 15 thousand DB-queries here!

We are most probably pushing this into production today.

--
You are receiving this mail because:
You are the assignee for the bug.
You are watching all bug changes.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 17941] CanBookBeRenewed is very inefficient/slow

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17941

Alex Sassmannshausen <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |alex.sassmannshausen@ptfs-e
                   |                            |urope.com

--- Comment #10 from Alex Sassmannshausen <[hidden email]> ---
Hello,

This looks like a really interesting hack! Thanks for sharing :-)

In terms of using it in Koha, I have some questions:

- Documentation for Attributes mentions it is experimental and the internal API
may change.  We may need to consider whether this is a risk for Koha?  Does
anyone know what the status is on that?
(http://perldoc.perl.org/attributes.html)

- I too, like Jonathan, worry slightly about Plack implications.  It may just
be that I don't quite understand how long the cache exists — but it seems to me
that in a Plack context the cache that is created by memoization here would not
be busted.  This would be problematic as follows:

 + GetMember memoizes a call to a specific borrowernumber
 + Another user in Plack updates the borrower details
 + Next call to GetMember returns memoized result, not the updated result.

How is the cache expired?

It could be that Plack in CGI compatibility mode does the expiring for us, so
it might work well in practice — Petter could you confirm whether you tested
this kind of use-case or similar ones?

To be specific, some of the calls that are Memoized are only Idempotent in the
context of no other db updates happening.  In CGI context, this is no problem.
In long-living Plack processes this *may* be a problem.

Anyway, like I say — love the optimization! :-D

Alex

--
You are receiving this mail because:
You are the assignee for the bug.
You are watching all bug changes.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 17941] CanBookBeRenewed is very inefficient/slow

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17941

--- Comment #11 from Francesco Rivetti <[hidden email]> ---
(In reply to Alex Sassmannshausen from comment #10)
> - Documentation for Attributes mentions it is experimental and the internal
> API may change.  We may need to consider whether this is a risk for Koha?
> Does anyone know what the status is on that?
> (http://perldoc.perl.org/attributes.html)

yes, I read it as some attributes might change, or added. I used uppercase
naming to avoid future conflicts.

> - I too, like Jonathan, worry slightly about Plack implications.  It may
> just be that I don't quite understand how long the cache exists — but it
> seems to me that in a Plack context the cache that is created by memoization
> here would not be busted.  This would be problematic as follows:
>
>  + GetMember memoizes a call to a specific borrowernumber
>  + Another user in Plack updates the borrower details
>  + Next call to GetMember returns memoized result, not the updated result.
>
> How is the cache expired?

I'm working on documenting that hack a bit better, but the whole system relies
on:

local $CACHE = $CACHE//{};

this is a bit of perl trickery here, because the "local" will make a new
"version" of $CACHE while the scope it is in is still alive.

on the other end, the new "version" will use the same "instance" if available,
which means that you can be sure that only one cache is shared but it won't
live longer than the life of the first pure function life.

in other words, when the { local $CACHE = $CACHE//{}; .... } block goes out of
scope, it will revert to the previous version of CACHE, which will (and MUST
be) undef at plack time.

> To be specific, some of the calls that are Memoized are only Idempotent in
> the context of no other db updates happening.  In CGI context, this is no
> problem.  In long-living Plack processes this *may* be a problem.

You are absolutely right, and it is designed to be safe in this case.

you can actually push even further.

sub hybrid {
  pure(); # all happening inside this function will be memoized _temporary_
  db_change();
  pure(); # a new cache will be used here, with new data from db
}

or, in case you need it you could:

sub hybrid {

  K::U::PF::pure { # start a new cache
    for (...) {
      pure($_);
    }
  }; # cache is destroyed

  db_change();

  pure(); # new cache
}

I'm also adding another attribute which will throw an error in case like this

sub db_changes : ImpureFunction {
  ...
}

sub pure : PureFunction {
  db_changes(); # db_changes is marked as not pure, and will complain
}

and while we are in topic, any suggestions for the "Impure" function?

--
You are receiving this mail because:
You are the assignee for the bug.
You are watching all bug changes.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 17941] CanBookBeRenewed is very inefficient/slow

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17941

--- Comment #12 from Francesco Rivetti <[hidden email]> ---
Created attachment 60181
  -->
https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=60181&action=edit
local transformation that will greatly improve the performance

--
You are receiving this mail because:
You are the assignee for the bug.
You are watching all bug changes.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 17941] CanBookBeRenewed is very inefficient/slow

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17941

--- Comment #13 from Francesco Rivetti <[hidden email]> ---
I found few more spot where some optimization was easy to add, and should
definitely make it faster. I still think using pure functions helps in general,
but this is probably a better and easier fix.

--
You are receiving this mail because:
You are the assignee for the bug.
You are watching all bug changes.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 17941] CanBookBeRenewed is very inefficient/slow

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17941

Francesco Rivetti <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Assignee|[hidden email]-commun |[hidden email]
                   |ity.org                     |

--
You are receiving this mail because:
You are watching all bug changes.
You are the assignee for the bug.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 17941] CanBookBeRenewed is very inefficient/slow

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17941

--- Comment #14 from Francesco Rivetti <[hidden email]> ---
Created attachment 60184
  -->
https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=60184&action=edit
BZ 17941 typo

--
You are receiving this mail because:
You are watching all bug changes.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 17941] CanBookBeRenewed is very inefficient/slow

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17941

--- Comment #15 from Francesco Rivetti <[hidden email]> ---
Created attachment 60185
  -->
https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=60185&action=edit
BZ 17941 don't compute priority more than needed

--
You are receiving this mail because:
You are watching all bug changes.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 17941] CanBookBeRenewed is very inefficient/slow

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17941

--- Comment #16 from Francesco Rivetti <[hidden email]> ---
Created attachment 60186
  -->
https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=60186&action=edit
BZ 17941 avoid scanning the full cartesian product

when a item match a borrower, there is no point in checking the
other borrowers

--
You are receiving this mail because:
You are watching all bug changes.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 17941] CanBookBeRenewed is very inefficient/slow

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17941

--- Comment #17 from Francesco Rivetti <[hidden email]> ---
as per Joubu request on irc, sending the patches again via git-bz

--
You are receiving this mail because:
You are watching all bug changes.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 17941] CanBookBeRenewed is very inefficient/slow

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17941

Francesco Rivetti <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |Needs Signoff

--
You are receiving this mail because:
You are watching all bug changes.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 17941] CanBookBeRenewed is very inefficient/slow

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17941

Francesco Rivetti <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #60181|0                           |1
        is obsolete|                            |

--
You are receiving this mail because:
You are watching all bug changes.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 17941] CanBookBeRenewed is very inefficient/slow

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17941

--- Comment #18 from Francesco Rivetti <[hidden email]> ---
Created attachment 60188
  -->
https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=60188&action=edit
Bug 17941 Adding PureFunctions functionality

--
You are receiving this mail because:
You are watching all bug changes.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 17941] CanBookBeRenewed is very inefficient/slow

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17941

Francesco Rivetti <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #59740|0                           |1
        is obsolete|                            |
  Attachment #60184|0                           |1
        is obsolete|                            |
  Attachment #60185|0                           |1
        is obsolete|                            |
  Attachment #60186|0                           |1
        is obsolete|                            |

--- Comment #19 from Francesco Rivetti <[hidden email]> ---
Created attachment 60189
  -->
https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=60189&action=edit
Bug 17941 avoid scanning the full cartesian product

when a item match a borrower, there is no point in checking the
other borrowers

--
You are receiving this mail because:
You are watching all bug changes.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 17941] CanBookBeRenewed is very inefficient/slow

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17941

--- Comment #20 from Francesco Rivetti <[hidden email]> ---
Created attachment 60190
  -->
https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=60190&action=edit
Bug 17941 don't compute priority more than needed

--
You are receiving this mail because:
You are watching all bug changes.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 17941] CanBookBeRenewed is very inefficient/slow

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17941

--- Comment #21 from Francesco Rivetti <[hidden email]> ---
Created attachment 60191
  -->
https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=60191&action=edit
Bug 17941 typo

--
You are receiving this mail because:
You are watching all bug changes.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 17941] CanBookBeRenewed is very inefficient/slow

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17941

--- Comment #22 from Francesco Rivetti <[hidden email]> ---
Created attachment 60192
  -->
https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=60192&action=edit
Bug 19741 more typos

--
You are receiving this mail because:
You are watching all bug changes.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 17941] CanBookBeRenewed is very inefficient/slow

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17941

Francesco Rivetti <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #60188|0                           |1
        is obsolete|                            |

--
You are receiving this mail because:
You are watching all bug changes.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 17941] CanBookBeRenewed is very inefficient/slow

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17941

--- Comment #23 from M. Tompsett <[hidden email]> ---
Comment on attachment 60189
  --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=60189
Bug 17941 avoid scanning the full cartesian product

Review of attachment 60189:
 --> (https://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=17941&attachment=60189)
-----------------------------------------------------------------

Love the optimization, but two tiny code changes would improve readability.

::: C4/Circulation.pm
@@ +2683,5 @@
> +            ITEM: foreach my $i (@itemnumbers) {
> +                my $item = GetItem($i);
> +                next if IsItemOnHoldAndFound($i);
> +                for my $b (@borrowernumbers) {
> +                    my $borr = $borrowers{$b}//= C4::Members::GetMember(borrowernumber => $_);

Could we avoid $_? It is harder to read. Here it should be $b.

@@ +2687,5 @@
> +                    my $borr = $borrowers{$b}//= C4::Members::GetMember(borrowernumber => $_);
> +                    next unless IsAvailableForItemLevelRequest($item, $borr);
> +                    next unless CanItemBeReserved($b,$i);
> +
> +                    push @reservable;

The old code is pushing item numbers into the reservable array. This seems to
be using an implicit $_ type behaviour, which I believe is $b. This isn't code
equivalent, and explicit is always easier for debugging later. Though, I don't
think it will matter, which is probably why it was optimized this way. Still,
could we be explicit? It's easier to read.

--
You are receiving this mail because:
You are watching all bug changes.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 17941] CanBookBeRenewed is very inefficient/slow

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17941

--- Comment #24 from M. Tompsett <[hidden email]> ---
Comment on attachment 60191
  --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=60191
Bug 17941 typo

Review of attachment 60191:
 --> (https://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=17941&attachment=60191)
-----------------------------------------------------------------

::: C4/Circulation.pm
@@ +2688,4 @@
>                      next unless IsAvailableForItemLevelRequest($item, $borr);
>                      next unless CanItemBeReserved($b,$i);
>  
> +                    push @reservable, $i;

Ah, you caught it. Great!

--
You are receiving this mail because:
You are watching all bug changes.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 17941] CanBookBeRenewed is very inefficient/slow

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17941

--- Comment #25 from M. Tompsett <[hidden email]> ---
Comment on attachment 60192
  --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=60192
Bug 19741 more typos

Review of attachment 60192:
 --> (https://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=17941&attachment=60192)
-----------------------------------------------------------------

::: C4/Circulation.pm
@@ +2684,4 @@
>                  my $item = GetItem($i);
>                  next if IsItemOnHoldAndFound($i);
>                  for my $b (@borrowernumbers) {
> +                    my $borr = $borrowers{$b}//= C4::Members::GetMember(borrowernumber => $b);

Sweet! Let's get some sign off. :)

--
You are receiving this mail because:
You are watching all bug changes.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
12
Loading...