Repository navigation
Perf: Fewer and lighter SQL queries for uncached category pages - #1346
Conversation
| * @param int[] $idFeatures | ||
| * @param int $idLang | ||
| */ | ||
| public function loadFeatureValues(array $idFeatures, $idLang) |
There was a problem hiding this comment.
it removes an N+1: getFeaturesBlock() ran one feature value query per displayed feature. Measured on 42 features, cold page: 42 queries / ~9.5 ms of SQL → 1 query / ~2.3 ms. getFeatureValues() keeps its signature for the other callers (Converter) and only loads what isn't loaded yet.
With only nleft >= and nright <= on the browsed category, the range read on the category index covers every following category, so MySQL computed the population from all the shop products and filtered the subtree last. Bounding nleft by the category nright is redundant in the nested set but makes the index range match the subtree.
The stock_available join went through product_attribute_combination, which repeated each combination stock row once per attribute. Joining it on product_attribute selects the same rows without the duplicates. The quantity summed to order the out of stock products last no longer counts a combination once per attribute: it is now the actual stock of the product.
Each attribute group and each feature facet ran its own count query, and each one recomputed the population of the category. The facets without selected value, and the AND features, share the current filters: their values are now counted in one query per facet type. A facet with a selected value keeps its own query, as it ignores its own selection. The values of the displayed features are also loaded in one query.
The "Not available" count filters on quantity and out_of_stock in the same operation, and each item got its own stock_available join on the same row. The stock fields are now flagged to share the join of their operation: the unique key of stock_available guarantees a single row. Operations on other tables keep one join per item, needed when they require several values.
7bbd38d to
f49a1cc
Compare
Review — ps_facetedsearch PR #1346 · Perf: Fewer and lighter SQL queries for uncached category pages
Environment
Method
ResultsHTTP (best of 3)
The controls stay flat, so the gains come from the module. The PR's times repeat within 3% between its two SQL bench, module only (best of both rounds,
|
| Scenario | Products | dev | PR | Δ |
|---|---|---|---|---|
| A. big cat, no filter | 100 000 | 21.68 s | 10.93 s | −50% |
| B. big cat, 1 feature | 5 573 | 2.36 s | 1.64 s | −30% |
| C. big cat, 2 features | 309 | 0.634 s | 0.537 s | −15% |
| D. big cat, 3 features | 17 | 0.436 s | 0.410 s | −6% |
| E. big cat, 2 feat + colour | 84 | 0.652 s | 0.604 s | −7% |
| F. big cat, 2 feat + in stock | 309 | 3.05 s | 1.30 s | −57% |
| G. leaf cat, no filter | 4 166 | 0.820 s | 0.396 s | −52% |
| H. leaf cat, 1 feature | 253 | 0.602 s | 0.370 s | −39% |
| I. leaf cat, not available | 666 | 6.58 s | 1.01 s | −85% |
| J. leaf cat, colour + in stock | 0 | 2.36 s | 1.21 s | −49% |
| K. big cat, colour + in stock | 24 000 | 16.44 s | 8.39 s | −49% |
| L. big cat, colour + size | 0 | 2.07 s | 1.83 s | −12% |
| M. big cat, feature + not available | 863 | 8.08 s | 4.60 s | −43% |
Almost all of the time saved is in facet building; product listing times barely change. The availability scenarios
(F, I, J, K, M) gain the most, which fits the stock join moving from pac to pa. Narrow multi-feature filters
(C–E) were already cheap and gain little.
Correctness
The record covers 560 cases on each side. 546 of them could be compared:
- Product totals: 0 differences.
- Product ids, in order: 0 differences, under all 8 sorts and both out-of-stock-last settings.
- AND-mode feature facet: identical.
- Facet counts: 2 cases differ, the same price-filter case with each out-of-stock-last setting. Details below.
Facet counts with a price filter: dev is wrong, this PR is right
On /101-bench-cat-1?q=Price-€-10-200, 1 878 products are listed on both sides. On dev, though, every
attribute and feature facet is counted as if the price filter were not there:
| Facet value | dev | PR | SQL count over the 1 878 listed products |
|---|---|---|---|
| Size 1 | 1 666 | 626 | 626 |
| Size 2 | 1 666 | 834 | 834 |
| Colour 2 | 2 083 | 1 043 | 1 043 |
| Bench attr. group 102, every value | 4 166 | 1 878 | 1 878 |
| F1V6 | 253 | 122 | — |
On dev, then, a value can claim more products than the page lists: 4 166 against 1 878. The category,
availability, condition and extras facets were already correct on both sides. I confirmed this on the storefront
HTML, not only through the CLI harness, and the SQL ground truth matches the PR exactly.
I haven't traced which part of the change fixes it: computing the counts up front in countFacetValues() or the
MySQL.php join changes. Either way it is a visible behaviour change that the description doesn't mention.
Could you:
-
add it to the description;
-
add a test (unit or Behat) that pins the attribute/feature counts while a price filter is active, so that
neitherdevnor a later refactor can regress it.Two findings from testing this PR on PrestaShop 9.2.0 (100k products, MariaDB 11.4), compared with
dev@98fa494. Neither one is blocking, and neither needs a code change. In both cases the PR's behaviour is the correct one. What is missing is the description and a test.
1. Facet counts under a price filter: dev is wrong, this PR fixes it (undocumented)
What happens. When a price filter is active, dev counts attribute and feature values as if the price filter were not there. This PR counts them within the price range.
Example. On /101-bench-cat-1?q=Price-€-10-200, 1 878 products are listed on both sides:
| Facet value | dev |
this PR | SQL count over the 1 878 listed products |
|---|---|---|---|
| Size 1 | 1 666 | 626 | 626 |
| Colour 2 | 2 083 | 1 043 | 1 043 |
| A Material value | 4 166 | 1 878 | 1 878 |
On dev, then, a value can claim more products than the page lists (4 166 against 1 878). The category, availability, condition and extras facets are correct on both sides. Only attribute and feature facets are affected. I confirmed this on the storefront HTML, and the PR's numbers match a direct SQL count exactly.
Is it blocking? No. This is a pre-existing bug on dev, and the PR fixes it, most likely as a side effect of counting all facets up front in countFacetValues(). I haven't traced which part of the change fixes it.
What to do:
- Mention the fix in the description and changelog: it is a visible change to the counts merchants and customers see.
- Add a test (unit or Behat) that checks attribute/feature counts while a price filter is active. The fix is accidental, so nothing stops a later refactor from bringing the bug back.
2. The stock-ordering change is correct, but the description doesn't match what it does
What happens. As documented, the stock sum used for ordering used to count each combination once per attribute, and it is now the real sum. To check this I seeded products with negative stock and with combinations of different attribute counts. Then I compared the whole ordered product list of a 4 166-product category, under 60 combinations of filter, sort and out-of-stock-last setting:
- Product totals, product sets and facet counts are identical.
- Where the order changes, it is identical again once the affected products are removed. Nothing else moves.
- The affected products end up where their real stock puts them. The PR is right and
devis wrong.
The description is inaccurate in two ways:
- The trigger is narrower than described. The description says the order changes for "a product mixing combinations in stock and combinations with a negative stock". In fact it only changes when those combinations have different numbers of attributes.
- When all combinations have the same attribute count, the old sum is just a multiple of the real one, with the same sign, so nothing moves.
- I had 166 mixed-stock products with 3 attributes on every combination in the tested category, and none of them moved.
- So the trigger is a product mixing in-stock and negative-stock combinations with different attribute counts. The back-office combination generator gives every combination of a product the same attribute groups, so this mostly happens with combinations created or imported one by one.
- The quantity sort changes too, even with
PS_LAYERED_FILTER_SHOW_OUT_OF_STOCK_LASToff.product.quantity.descorders on the same SUM, so the same products move there. Hummingbird doesn't offer this sort in its menu, so it only matters to themes or modules that request it.
Is it blocking? No. The new behaviour is a fix, since the old sum was wrong, and it affects few products.
What to do: update the "Behaviour change" section, for example:
PS_LAYERED_FILTER_SHOW_OUT_OF_STOCK_LASTand the quantity sort order on the sum of the stock of the product combinations. That sum counted each combination once per attribute; it is now the real sum. The order changes for products whose combinations have different numbers of attributes.
kpodemski
left a comment
There was a problem hiding this comment.
To me it looks good, it would be great to have @Hlavtox green light as well. Thank you @LameuleFR
|
Thanks for the review, the ABBA runs and the 560-case record are very useful. 1. Counts under a price filter. Root cause found: This PR fixed it by accident for attributes and features only, because their counts now run before the loop over the blocks; facets placed after the weight block were still wrong. Commit 04f7e9f "Restore the price filter after an empty weight block" moves the restore before the early return, and 2. Stock ordering. Right on both points, the "Behaviour changes" section now uses your wording, including the quantity sort. |
| // we add this one specific category ID, otherwise, we will add everything using nleft and nright | ||
| if (Configuration::get('PS_LAYERED_FULL_TREE')) { | ||
| $this->getSearchAdapter()->addFilter('nleft', [$category->nleft], '>='); | ||
| $this->getSearchAdapter()->addFilter('nleft', [$category->nright], '<='); |
There was a problem hiding this comment.
Hello guys, just one question, what this line does?
There was a problem hiding this comment.
Logically this changes nothing, in the nested set, every subcategory already has its nleft between the category's nleft and nright. The extra condition is only there for MySQL. Without an upper bound on nleft, the index range runs to the end of the tree, so MySQL was starting from all of the shop's products instead of just the subtree.
There was a problem hiding this comment.
Oh, now I understand.
@LameuleFR Can you add some comment for next generations? :D Something like:
// Explicitly bound nleft on both sides to limit the index scan
// to the current category subtree. The upper bound is logically
// redundant but helps MySQL use a much smaller index range.
There was a problem hiding this comment.
Done in 988e00d, on both places using this bound (Search.php and the categories block in Block.php).
|
@Hlavtox comment added in 988e00d. @PululuK @Touxten @kpodemski the push dismissed your approvals, could you re-approve so we can merge? CI is green. |
|
PR merged, well done! Message to @PrestaShop/committers: do not forget to milestone it before the merge. |
Base 6187d3b. Apporte notamment la PR PrestaShop#1346 de performance, le support des conditions produit de PrestaShop 9.2, le correctif PHP 8.5 et le passage de webpack a esbuild. Rebase sans conflit, les 14 commits locaux rejoues tels quels : la liste des fichiers divergents du §2 reste exacte, et src/Adapter/MySQL.php est identique a upstream, ce qui laisse entrer les optimisations SQL sans retouche. Compte de tests attendu actualise, l'amont en a ajoute deux. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Changes
Search.php,Block.phpcategories block): addnleft <= nrightof the browsed category next tonleft >= nleft/nright <= nright. Redundant in the nested set, but with onlynleft >=the index range covers every following category, so MySQL drove the population from all the shop products (10k products, 33k category rows) instead of the subtree. Listing query of a 50-product category: 70 ms → 0.6 ms.MySQL.phpmapping ofquantity/out_of_stock): joinstock_availableonpa.id_product_attributeinstead of going throughproduct_attribute_combination, which repeated each combination stock row once per attribute.Block.php): the groups/features without selected value (and the AND features) share the same filters, so their values are counted in one query instead of one query per group/feature. A facet with a selected value keeps its own query, as it ignores its own selection. Feature values are loaded in one query instead of one per feature (DataAccessor::loadFeatureValues()).quantityandout_of_stockitems of one operation now share theirstock_availablejoin (shareOperationJoinmapping flag) instead of joining the same row twice. Only these stock fields are flagged: the unique key ofstock_availableguarantees a single row; operations on other tables keep one join per item.Block::getWeightRangeBlock()): when no product of the population has a weight, the block returned before restoring the price and weight filters it had removed from the population. Every facet computed after it was then counted without the price filter.Results
Test catalog: 10k products, 31.7k combinations, 9 attribute groups, 42 features (1084 values), 152 brands, filter template with 58 filters; facet block cache emptied before each cold request.
Warm pages (facet blocks cached): 263 ms → about 205 ms on the big category (listing query, change 1).
The rendered pages (product list, every facet value and count) were compared with
devon 15 URLs, including availability filters, AND features, attribute + availability and the root category: identical.Behaviour changes
dev, when the template has a weight filter and no product of the page has a weight, every facet placed after the weight filter in the template is counted as if the price filter were not set (a value can claim more products than the page lists). They are now counted within the price range (change 5, covered byBlockTest::testGetFiltersBlockWithoutWeight).PS_LAYERED_FILTER_SHOW_OUT_OF_STOCK_LASTand the quantity sort order on the sum of the stock of the product combinations. That sum counted each combination once per attribute; it is now the real sum. The order changes for products whose combinations have different numbers of attributes.How to test