In #2711205: Enable Post Query processors. Post Query processors were added, but looking at the code these are executed too frequently.
The code is in DefaultFacetManager::processFacets()
:
public function processFacets($facetsource_id = NULL) {
if (empty($facetsource_id)) {
foreach ($this->facets as $facet) {
$this->processFacets($facet->getFacetSourceId());
}
}
elseif (empty($this->processedFacetSources[$facetsource_id])) {
$this->updateResults($facetsource_id);
$this->processedFacetSources[$facetsource_id] = TRUE;
}
foreach ($this->facets as $facet) {
foreach ($facet->getProcessorsByStage(ProcessorInterface::STAGE_POST_QUERY) as $processor) {
// ... execute post query processors ...
}
}
}
If we have a couple of facets and ::processFacets()
is called without arguments, then it will loop over all facets and call itself again. This will then check each facet if it has been processed already, and it will run the post query processors for _all_ facets
(not only the facet being processed) every time, even if the facet was already processed before and doesn't need to be processed again.
This is quite problematic. If a site has 20 facets then the processors will be executed 20 times for all facets, meaning that the postQuery()
method will in fact be called 400 times.
Then finally when the initial loop ends, it will continue to the end of the method and will run the post query processors again, which brings the total to 420 times. This is altogether too much.
I think this bug has probably been enabled because there is not a clear separation of the different processor stages. To me it seems bizarre that when I call ::processFacets()
that it is assumed that the facets have already passed the `ProcessorInterface::STAGE_PRE_QUERY` stage, that is will "process" the facets which doesn't appear to be a processing stage on its own, and then it will execute `ProcessorInterface::STAGE_POST_QUERY` but not `ProcessorInterface::STAGE_BUILD` and `ProcessorInterface::STAGE_SORT`. Probably "process" and "process" mean two different things :)
Comment | File | Size | Author |
---|---|---|---|
#11 | 2920846-11.patch | 12.37 KB | borisson_ |
Comments
Comment #2
borisson_That is a problem.
Comment #3
borisson_I haven't tested this yet, but this should reduce the calls to the post processors.
I'll try testing this tomorrow, but I'm stopping for the day.
Comment #4
borisson_I've tested the patch in #3 on a page with 7 facets. There doesn't seem to be any difference with/without the patch. There's a minimal difference but not enough to actually make a difference. Back to the drawing board.
Before:
After:
Comment #5
borisson_This turned into quite the cleanup, this is not a big improvement, not even noticeable really. But is a small improvement. The readability is improved.
Comment #7
borisson_Comment #8
borisson_I ran boom to check if this was an actual improvement.
Before:
After:
So this is a huge improvement. This is with 6 defined and 2 shown facets. With that setup it is +10% improvement. (if I did that maths correctly)
Comment #9
Erik Frèrejean@borisson_,
I'm happy to review this but I'll have to setup an environment for that . The site I'm talking about has such huge facets that we've had to implement various other workarounds to keep the speed anyway near acceptable.
Comment #10
borisson_Discussed this with @StryKaizer, in the patch in #7 we remove the facetsource id argument from the
processFacets
. This is bad when there's multiple facets sources defined that aren't always shown.We should get this argument back, and make sure it's called from all implementations. Back to needs work.
Comment #11
borisson_I think this fixes all of that.
Comment #13
borisson_Committed and pushed, thanks for brining this up and for the very detailed issue summary.
Comment #14
pfrenssenAwesome, thanks!
Comment #15
borisson_Oh, looks like I forgot to credit you. 🤦
Issues as detailed as this one deserve the credit. Sorry for not getting you in for the commit message. I can revert + recommit if you want?
Comment #16
pfrenssenNo need! I'm happy this is fixed, and honestly my part in this was very limited, you did all the work.