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.
Created attachment 28623 [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
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
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
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.
I've increased bug's importance. OPAC response time under few seconds (2-3-4?...) is critical. Some would disagree
(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 ?
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).