[Koha-devel] Restructuring C4 (work continued)
Ian Walls
ian.walls at bywatersolutions.com
Thu Sep 15 13:35:32 CEST 2011
Paul,
Excellent! I'd been meaning to do a complete workup on the system through
NYTProf when I got the time, and it looks like you've beat me too it. Can
you post the full reports somewhere online, with some kind of guide as to
what scripts are which, which system preferences you had, and what input
params you passed?
I've got NYTProf on NEKLS test system (with their permission) and find that
that's an excellent resource for testing pages under various kinds of load.
It helped me fine the root of bug 6801 (which I still need to patch :/ )
Cheers,
-Ian
On Wed, Sep 14, 2011 at 5:27 PM, Paul Poulain <paul.poulain at biblibre.com>wrote:
> Hi,
>
> I've continued my work in the train, here is a result of my work.
>
> I've checked a lot of the packages in C4 and checked for dependancies
> (sub packages used)
> With the check_subs.pl script, it was quite easy to find which subs were
> called and see if it's needed.
>
> What I propose next is to file a bug for each package with my findings
> and my suggestions. Then, write a patch.
> I also plan to investigate some of the most used pages (like
> circ/circulation.pl, circ/return.pl, detail.pl, and most opac pages.
> Note that the check_subs can be also use to check package dependancies
> for perl scripts as well: on line 39, set "dirToCheck" to the directory
> you want to check. I've checked circ and opac, and have already
> discovered some interesting informations.
> If you have some comment, want to make another suggestion or give a
> hand, you're welcomed ;-)
>
> mainpage.pl loads the following .pm
> ==== C4::Auth.pm ====
> == use Members.pm ==
> only to call GetMemberDetails and fill USER_INFO parameters
> USER_INFOS are used in a large variety of place.
> * In a strange way, like in : acqui/acqui-home.pl:
> $template->{VARS}->{'USER_INFO'}[0]->{'borrowernumber'} );
> Q: don't we have another way to retrieve loggedin librarian number ?
> * in opac-reserve, to display who we place a request for. Again,
> this could/should be moved to opac-reserve, and we use GetMemberDetails
> in this sub
>
> CONCLUSION = we should be able to get rid of use Members.pm in Auth.pm
> == use C4::Context.pm ==
> package that can't be avoided and loads nothing specific
> == use C4::Output.pm ==
> = use C4::Budgets =
> just to call Budget::GetCurrency in the Output::FormatNumber sub.
> The Output::FormatNumber sub is used only in a few places, related to
> C4::Budgets
> CONCLUTION => move FormatNumber outside from C4::Output into
> C4::Budgets and we will get rid of this dependancy
> = use C4::Languages =
> to call a lot of sub here, no possibility to do something
> = use C4::Dates =
> to call format_date, in Date::FormatDate in C4::Output::FormatData
> sub that is called only in aqbudgetperiod.pl and should easily be
> removed. it contains only
> $$data_hashref{$_} = format_date( $$data_hashref{$_} ) for
> grep{/date/} keys (%$data_hashref);
> that can be inlined in aqbudgetperiod (and it would be better to use
> the OOo date tools)
> = use C4::Templates =
> needed, it's the package that smoothen the transition between
> H::T::P and T::T
>
> == use C4::Branch ==
> = use C4::Koha = just to call get_infos_of once, we could get rid
> with this sub by inlining and remove the C4::Koha dependancy
>
> == use C4::VirtualShelves ==
> * this package is needed in case of an OPAC query (on every opac
> page we display the public & private lists). I suspect it's not needed
> in staff interface. Anyone to confirm/argue ?
> * We use only GetRecentShelves sub,
> * we put everything in the session (set_shelves_userenv) & context
> ($session->param('barshelves', $barshelves);) I think there is no use
> for this. Anyone to confirm/argue ?
> * also note that the following line
> my ($total, $pubshelves) =
> C4::Context->get_shelves_userenv(); # an anonymous user has no
> 'barshelves'...
> (C4::Auth, line 301 and the following) seems strange.
> = use C4::Circulation =
> and that's the beginning of circular-painfull-loading-everything.
> In C4::VirtualShelves, there is no use of anything from
> C4::Circulation. I've commented the line, made a lot of tests, and saw
> no problem at all.
>
> ==== C4::Koha ====
> This loading is useless. I've commented the line and saw no problem.
>
> ==== C4::AuthoritiesMarc; ====
> This part is useless here, we do nothing with the authorities retrieved.
>
> ==== C4::NewsChannels ====
> == use C4::Context ==
> nothing to say, needed to access the database
> == use C4::Dates ==
> nothing to say, needed to format the date the news has been published.
>
> <<<< END OF mainpage specifics >>>>
> Continued to check some of the major .pm
>
> ==== Member.pm ====
> Member.pm, (that is loaded by Auth.pm at the moment) load the following
> packages:
> == use Reserves.pm ==
> Reserves.pm is used only to call
> my @itemswaiting =
> C4::Reserves::GetReservesFromBorrowernumber(
> $patroninformation->{'borrowernumber'},'W' );
> in C4::Members::patronflags sub
> I'm not sure we do anything with the result of
> GetReservesFromBorrowernumber:
> C4::Members, line 518 contains :
> my @itemswaiting =
> C4::Reserves::GetReservesFromBorrowernumber(
> $patroninformation->{'borrowernumber'},'W' );
> my $nowaiting = scalar @itemswaiting;
> if ( $nowaiting > 0 ) {
> my %flaginfo;
> $flaginfo{'message'} = "Reserved items available";
> $flaginfo{'itemlist'} = \@itemswaiting;
> $flags{'WAITING'} = \%flaginfo;
> }
> return ( \%flags );
> I've some doubts about the need for this code, i've commented
> it, placed a hold, make it waiting for pickup, I see everything as
> usual, I saw no difference. It will be worth a bug entry and a patch to
> test soon !
>
> I've moved the use C4::Members to a require C4::Member just
> before the GetReservesFromBorrowernumber call
> NYTProf before commenting :
> Profile of mainpage.pl for 1.24s (of 1.54s), executing
> 102597 statements and 25016 subroutine calls in 212 source files and 55
> string evals.
> NYTProf after commenting :
> Profile of mainpage.pl for 1.25s (of 1.55s), executing
> 102489 statements and 24984 subroutine calls in 212 source files and 55
> string evals.
> (the duration is strange. But I was with my laptop on the
> battery, so that may explain. The number of statements&subroutines are
> lowered though)
>
> == use C4::Overdues.pm ==
> this is loaded only to call checkoverdues in
> C4::Members::GetMemberDetails
> The result is stored in the big member detail hash returned :
> my ( $odues, $itemsoverdue ) =
> checkoverdues($patroninformation->{'borrowernumber'});
> if ( $odues && $odues > 0 ) {
> my %flaginfo;
> $flaginfo{'message'} = "Yes";
> $flaginfo{'itemlist'} = $itemsoverdue;
> foreach ( sort { $a->{'date_due'} cmp $b->{'date_due'} }
> @$itemsoverdue )
> {
> $flaginfo{'itemlisttext'} .=
> "$_->{'date_due'} $_->{'barcode'} $_->{'title'}
> \n"; # newline is display layer
> }
> $flags{'ODUES'} = \%flaginfo;
> It's used only in circulation.pl, return.pl and SIP.pm
> My opinion is that we could have a dedicated sub for that in
> Circulation.pm (that is always loaded in circulation.pl & return.pl),
> retrieve the result from here when needed and reduce the size of
> GetMemberDetails (that would not return everything)
>
> == use C4::Biblio.pm ==
> this is loaded just to call C4::Biblio::GetBiblioFromItemNumber in
> C4::Members::GetMemberAccountRecords to retrieve title and author
> This can be easily circumvented with a correct LEFT join a few
> lines before.
> Instead of
> SELECT *
> FROM accountlines
> WHERE borrowernumber
> write :
> SELECT accountlines.*,biblio.title,biblio.author FROM accountlines
> LEFT JOIN items USING(itemnumber) LEFT JOIN biblio USING(biblionumber)
> WHERE borrowernumber
> The use C4::Biblio can be removed !
> == use C4::Account.pm ==
> this is loaded to call manualinvoice twice.
> the manualinvoice is used only when adding or renewing a patron.
> So it should be efficient to move from a use C4::Account to a require
> C4::Account where applicable
>
> ==== Accounts.pm ====
> == use C4::Stats ==
> this package contains only one sub (updatestats) and no dependancies
> == use C4::Members ==
> loaded just to call C4::Members::GetMember once in the
> C4::Accounts::returnlost sub.
> This sub seems totally unused in Koha (dead code)
> grep -R "returnlost" * shows only the sub header itself. Could be
> removed.
> == use C4::Items ==
> loaded to call ModItem twice.
> The 1st call is in C4::Accounts::returnlost, that can be removed
> (see below)
> The 2nd call is in C4::Accounts::chargelostitem, that is rarely
> used. Switching to require C4::Items should be possible
> == use C4::Circulation ==
> This loading is similar to C4::Items with
> C4::Circulation::MarkIssueReturned sub :
> The 1st call is in C4::Accounts::returnlost, that can be removed
> (see below)
> The 2nd call is in C4::Accounts::chargelostitem, that is rarely
> used. Switching to require C4::Circulation should be possible
>
> ==== Acquisition.pm ====
> There is a use Debug.pm that is useless in Acquisition.pm
> There is a use Biblio.pm that is useless in Acquisition.pm
> use C4::Debug is written twice !
> == use C4::Suggestions ==
> the ModSuggestion and GetSuggestionFromBiblionumber are called
> when an order is recieved. Could switch to require, both statements are
> close
>
> ==== Biblio.pm ====
> == use C4::Serials ==
> Serials.pm is loaded to delete subscriptions in
> C4::Biblio::DelBiblio. 2 subs are used :
> C4::Serials::GetFullSubscriptionsFromBiblionumber and
> C4::Serials::DelSubscription
> both can be moved to require at the right place
> == use C4::Items ==
> * the check_subs script wrongly report a use of ModItem and
> DelItem, it's a comment in the code :
> # duplication or incorrect data - use AddItem() or ModItem()
> * C4::Biblio uses C4::Items::GetMarcItem twice
> * once in PrepareItemrecordDisplay => this sub sounds at a wrong
> place. It could/should be moved to C4::Items itself.
> PrepareItemrecordDisplay is used by:
> * acqui/addorderiso2709.pl = already loads C4::Items
> * acqui/neworderempty.pl = don't load C4::Items for instance
> * acqui/orderreceive.pl = already loads C4::Items
> * C4/Serials.pm = need more investigation
> * serials/serials-edit.pl = already loads C4::Items
> * once in C4::Biblio::EmbedItemsInMarcBiblio
> Could we remove this sub ? question still pending. it's used
> only in:
> * C4::Biblio::GetMarcBiblio
> * misc/migration_tools/rebuild_zebra.pl
> * tools/export.pl
>
> ==== Items.pm ====
> == use C4::Biblio ==
> Too many things are required, we can't change. The best solution
> would be to remove C4::Items reference in C4::Biblio
> GetOrderFromItemnumber
> == use C4::Reserves ==
> This package is loaded just for C4::Reserves::CheckReserves called
> in C4::Items::GetItemsInfo
> The GetItemsInfo stores the result of CheckReserves in a hash entry,
> count_reserve, that is used only in opac_detail to display the status of
> a hold. We could remove the reserve_count hash entry and inline
> C4::Reserves::CheckReserves directly from opac-detail.pl page
> in opac-detail.pl, instead of
> if( $itm->{'count_reserves'} eq "Waiting"){ $itm->{'waiting'} = 1; }
> write :
> if ( C4::Reserves::CheckReserves(<<parameters>>) eq "Waiting"){
> $itm->{'waiting'} = 1; }
>
> == use C4::Acquisition ==
> This package is loaded to call
> * C4::Acquisition::ModOrder = recent addition, when you move an item
> to another biblio, you move the order if it's relevant. This sub is
> rarely called and could be switched to require
> * C4::Acquisition::GetOrderFromItemnumber = same comment, used a few
> lines before ModOrder
>
> ==== Reserves.pm ====
> Reserves.pm is really crapy and should probably be rewritten entirely.
> A grep -R "use C4::Reserves;" *|wc -l says that 33 scripts are using
> this package.
>
> There is a use Search.pm that is useless in Reserves.pm
> == use C4::Biblio ==
> loaded to call GetBiblioItemData and GetBiblioData:
> * GetBiblioData is called in AddReserve, to prepare and send a
> mail depending on the syspref emailLibrarianWhenHoldIsPlaced
> * GetBiblioItemData is used twice in the same sub
> C4::Reserves::GetReservesFromBiblionumber. Those informations are
> retrieved just to get title,author,... and could be added into the
> previous SQL query :
> instead of:
> SELECT biblioitemnumber
> FROM reserveconstraints
> WHERE biblionumber = ?
> AND borrowernumber = ?
> AND reservedate = ?
> write:
> SELECT reserveconstraints.biblionumber,biblio.*,biblioitems.* FROM
> reserveconstraints
> LEFT JOIN biblio USING(biblionumber) LEFT JOIN biblioitems
> USING(biblionumber)
> WHERE biblionumber = ? AND borrowernumber = ? AND
> reservedate = ?
> That should do the job, and the dependancy could be removed.
> Note: C4::Reserves::GetReservesFromBiblionumber is used in various
> places, for example opac-reserve.pl or in catalogue/detail.pl, but it
> seems we don't use all of the result computed in the sub. So maybe we
> can just clean GetReservesFromBiblionumber. It must be checked better
>
> == use C4::Members ==
> loaded to call:
> * C4::Members::GetMember is called in
> * C4::Reserves::AddReserve, to prepare and send a mail
> depending on the syspref emailLibrarianWhenHoldIsPlaced
> * C4::Reserves::CanItemBeReserved. This call is just to
> retrieve patron category and branch. The CanItemBeReserved is called
> only in request & opac-reserve & ILSDI pages. an option would be to send
> categorycode and branchcode from here (and change the API of the sub)
> * C4::Reserves::_koha_notify_reserve, to retrieve the Email
> (GetMember is always called, but the email is then retrieved from it or
> from GetFirstValidEmailAddress (see below, it's a dirty code...)
> * C4::Members::GetFirstValidEmailAddress
> GetFirstValidEmailAddress is called to send emails to patron that places
> a reserve, I don't see how to remove this dependancy...
> * C4::Members::GetMemberDetails is called in
> C4::Reserves::CheckReserves This sub is probably the crappiest in Koha.
> It mixes many things, it should be entirely rewritten
>
> Conclusion for C4::Reserves: I think we should try to isolate as much as
> possible this package.
>
> --
> Paul POULAIN
> http://www.biblibre.com
> Expert en Logiciels Libres pour l'info-doc
> Tel : (33) 4 91 81 35 08
>
> _______________________________________________
> Koha-devel mailing list
> Koha-devel at lists.koha-community.org
> http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-devel
> website : http://www.koha-community.org/
> git : http://git.koha-community.org/
> bugs : http://bugs.koha-community.org/
>
--
Ian Walls
Lead Development Specialist
ByWater Solutions
Phone # (888) 900-8944
http://bywatersolutions.com
ian.walls at bywatersolutions.com
Twitter: @sekjal
-------------- next part --------------
An HTML attachment was scrubbed...
URL: </pipermail/koha-devel/attachments/20110915/33a45764/attachment-0001.htm>
More information about the Koha-devel
mailing list