Bug 12347

Summary: Search performance improvements
Product: Koha Reporter: Tomás Cohen Arazi <tomascohen>
Component: Architecture, internals, and plumbingAssignee: Galen Charlton <gmcharlt>
Status: CLOSED INVALID QA Contact: Testopia <testopia>
Severity: critical    
Priority: P1 - high CC: f.demians, fridolin.somers, tomascohen
Version: Main   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12788
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 11096    
Bug Blocks:    
Attachments: Bug 12347: Performance improvements in Search.pm
Bug 12347: Unit tests should be updated to new API
Bug 12347: Performance improvements in Search.pm

Description Tomás Cohen Arazi 2014-06-02 18:16:20 UTC
As-of code introduced by bug 11096, the current C4::Search::getRecords and C4::Search::searchResults code creates MARC::Record objects for search results twice. This makes searching slower than before.

Code should be refactored in order to create the MARC::Record objects for the first time, and reuse it afterwards.
Comment 1 Tomás Cohen Arazi 2014-06-02 18:36:45 UTC Comment hidden (obsolete)
Comment 2 Tomás Cohen Arazi 2014-06-02 23:10:35 UTC
Created attachment 28627 [details] [review]
Bug 12347: Unit tests should be updated to new API

This patch updates t/db_dependent/Search.t to acknowledge the API change.
Now getRecords returns MARC::Record objects and thus there's no need to
have separate DOM and GRS-1 test cases (at least for it).

To test:
- Apply both bug's patches
- Run
  prove -v t/db_dependent/Search.t
=> SUCCESS: all tests pass

Best regards
To+

Sponsored-by: Universidad Nacional de Cordoba
Comment 3 Tomás Cohen Arazi 2014-06-02 23:10:45 UTC
Created attachment 28628 [details] [review]
Bug 12347: Performance improvements in Search.pm

Short: some costly stuff is done twice, this patch refactors that to make it
more reasonable.

Long: Bug 11096 introduced a noticeable overhead to the facet building stage
of the getRecords code. This patch leverages it by propagating the MARC::Record
objects created upon facet building, and reusing them in the C4::Search::searchResults
function.

To test:
- Search for a term on master
- Apply the patch
- Same results

Unit tests reflecting API change in a separate  patch.

Regards
To+

Sponsored-by: Universidad Nacional de Cordoba
Comment 4 Tomás Cohen Arazi 2014-06-11 01:28:40 UTC
The hypothesis is wrong in the formulation of the patch, as the MARC::Record objects are not built for all the result set in searchResults. As a consequence the speed improvement is really small.

The only way to improve this is by reverting the facet calcultation algorithm, or finishing the works on bug 11232.
Comment 5 Frédéric Demians 2014-10-27 11:29:25 UTC
I've increased bug's importance. OPAC response time under few seconds (2-3-4?...) is critical. Some would disagree
Comment 6 Fridolin Somers 2014-10-27 14:39:33 UTC
(In reply to Tomás Cohen Arazi from comment #4)
> The hypothesis is wrong in the formulation of the patch, as the MARC::Record
> objects are not built for all the result set in searchResults. As a
> consequence the speed improvement is really small.
> 
> The only way to improve this is by reverting the facet calcultation
> algorithm, or finishing the works on bug 11232.

bug 11232 is pushed master, makes this bug obsolete ?
Comment 7 Tomás Cohen Arazi 2014-10-30 12:31:46 UTC
I think this bug should be closed, and more precise ones should be filled, like:

"We shouldn't create MARC::Record objects out of MARCXML, use the XSLTs instead"

The bug title is also wrong, as it sounds like an omnibus bug instead of a real one. I'm closing it, so we fill new ones for specific things we could improve (because I agree with Frederic).