Bug 12347 - Search performance improvements
Summary: Search performance improvements
Status: CLOSED INVALID
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P1 - high critical (vote)
Assignee: Galen Charlton
QA Contact: Testopia
URL:
Keywords:
Depends on: 11096
Blocks:
  Show dependency treegraph
 
Reported: 2014-06-02 18:16 UTC by Tomás Cohen Arazi
Modified: 2019-06-27 09:24 UTC (History)
3 users (show)

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


Attachments
Bug 12347: Performance improvements in Search.pm (4.50 KB, patch)
2014-06-02 18:36 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 12347: Unit tests should be updated to new API (13.20 KB, patch)
2014-06-02 23:10 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 12347: Performance improvements in Search.pm (4.61 KB, patch)
2014-06-02 23:10 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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).