Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Performance improvements #1093

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

chrisai-dev
Copy link

Questions Answers
Description? Improved both cold and hot query performance by 2-15x
Type? improvement
BC breaks? no
Deprecations? no
Fixed ticket? Fixes PrestaShop/Prestashop#{issue number here}.
How to test? Please indicate how to best verify that this PR is correct.

Changes:

  • id_shop filter will be only added if multishop feature is enabled
  • product visibility filter is removed from the base query and added to the outer
  • category_product table join on the outer query was removed
  • removed all unused selects/joins from base query such as id_manufacture, price, on_sale etc. and added them as joins to $filterToTableMapping.

Performance:
There are the execution times for the queries generated by the module.
Cold means the faceted blocks are not yet cached, and warm means the blocks are cached, only the base query is running.

  • category 1:
    cold: 412ms -> 74ms
    warm: 31ms -> 2ms
  • category 2:
    cold: 673ms -> 258ms
    warm: 46ms -> 14ms
  • category 3:
    cold: 506ms -> 105ms
    warm: 36ms -> 5ms
  • category 4:
    cold: 81ms -> 23ms
    warm: 4ms -> 1ms

Test setup is a dev machine with ryzen 7700x, nvme ssd, 32gb ram, I expect better improvements on weaker configs.

@kpodemski
Copy link
Contributor

Hello @chrisai-dev

Thank you for your pull request. Very interesting results. Could you share more details about the number of products in a tested category, features, etc.?

Also, it would be great to understand better this part:

removed all unused selects/joins from base query such as id_manufacture, price, on_sale etc. and added them as joins to $filterToTableMapping.

The rest is fully understandable.

@Hlavtox
Copy link
Contributor

Hlavtox commented Nov 28, 2024

We will check it out in few days, if its as you say, amazing

@SharakPL
Copy link

id_shop filter will be only added if multishop feature is enabled

I'm not sure that's wise. What if there are still multishop products in the database, but the user decided to disable multishop temporarily? Won't this make the query include the products from all shops?

@chrisai-dev
Copy link
Author

id_shop filter will be only added if multishop feature is enabled

I'm not sure that's wise. What if there are still multishop products in the database, but the user decided to disable multishop temporarily? Won't this make the query include the products from all shops?

@SharakPL, Once you have enabled multishop, and uploaded a product, you cannot disable the feature. No need to add this filter condition for a single store.

Hello @chrisai-dev

Thank you for your pull request. Very interesting results. Could you share more details about the number of products in a tested category, features, etc.?

Also, it would be great to understand better this part:

removed all unused selects/joins from base query such as id_manufacture, price, on_sale etc. and added them as joins to $filterToTableMapping.

The rest is fully understandable.

Hi @kpodemski,

So it's been a while and I've deleted the test results, but, the store roughly had 4k products, 1k categories, the ps_category_product table had about 22000 rows. I tested very large categories with about 1k product, mid sized ones with 100ish, and a small one with 10.

// We add basic select fields we will need to matter what
$this->setSelectFields(
[
'id_product',
'id_manufacturer',
'quantity',
'condition',
'weight',
'price',
'sales',
'on_sale',
'date_add',
]
);

So this is the part where the fields for the base query are declared. The only field that is "needed no matter what", is id_product, the rest is only needed for specific queries. But these fields had no mappings to tables (

$filterToTableMapping = [
)
since they were part of the base query, I added them.

This is the function that computes the joins based on the query

protected function computeJoinConditions(array $filterToTableMapping)

I only tested on 8.1.x, don't know if this will affect compatibility with <= 1.7.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants