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

Improve cart performance #32058

Open
wants to merge 1 commit into
base: 8.1.x
Choose a base branch
from

Conversation

MattKelvin
Copy link

Questions Answers
Branch? develop
Description? When you have different combinations of the same product in cart, the cache don't seem to work as expected. I think we should add the product attribute id in the cache key and add in the query a new condition with the product attribute id in order to retrieve the real quantity (otherwise, we get the sum of all the product so the condition is never valid).
Type? bug fix
Category? CO
BC breaks? no
Deprecations? no
How to test? Start from a Prestashop 8.0.3, active the profiler. Choose a product with combinations and add three different one. Go to your cart and check the profiler. You will see something like "Stopwatch SQL - 422 queries" and a lot of double query "SELECT SUM(quantity) FROM ps_cart_product WHERE id_product = XX AND id_cart = XX LIMIT XX". Now go on my branch, refresh the profiler the sql queries should be better (around 250)
Fixed ticket? None
Related PRs None
Sponsor company /

Hello,

It's my first PR, I tried to do my best but let me know if I did something wrong. What do you think of this change ?

Thank you :)

@MattKelvin MattKelvin requested a review from a team as a code owner April 5, 2023 14:24
@prestonBot
Copy link
Collaborator

Hello @MattKelvin!

This is your first pull request on the PrestaShop project. Thank you, and welcome to this Open Source community!

@prestonBot
Copy link
Collaborator

Hi, thanks for this contribution!

I found some issues with the Pull Request description:

  • Your pull request does not seem to fix any issue, consider creating one (see note below) and linking it by writing Fixes #1234.

Would you mind having a look at it? This will help us understand how interesting your contribution is, thank you very much!

About linked issues

Please consider opening an issue before submitting a Pull Request:

  • If it's a bug fix, it helps maintainers verify that the bug is effectively due to a defect in the code, and that it hasn't been fixed already.
  • It can help trigger a discussion about the best implementation path before a single line of code is written.
  • It may lead the Core Product team to mark that issue as a priority, further attracting the maintainers' attention.

(Note: this is an automated message, but answering it will reach a real human)

@prestonBot prestonBot added develop Branch Bug fix Type: Bug fix labels Apr 5, 2023
Copy link
Contributor

@matthieu-rolland matthieu-rolland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's my first PR, I tried to do my best but let me know if I did something wrong. What do you think of this change ?

Thank you :)

You did great, thanks to you 👍

Just to be sure, this is a bug fix or a performance improvement?

The way I understand your description: the performance is suboptimal because of this bug, but it didn't break any software behavior or calculation, am I right?

@MattKelvin
Copy link
Author

MattKelvin commented Apr 5, 2023

For me it's a bug fix but I dug more and my PR break application of specific price here 😭

https://github.com/PrestaShop/PrestaShop/blob/8.0.x/classes/Product.php#L3762

Can I update this PR when I think I have a good fix or should I open a new one ?

Thank you :)

@mflasquin
Copy link
Contributor

For me it's a bug fix but I dug more and my PR break application of specific price here sob

https://github.com/PrestaShop/PrestaShop/blob/8.0.x/classes/Product.php#L3762

Can I update this PR when I think I have a good fix or should I open a new one ?

Thank you :)

If you want you can put your PR as draft, add a new commit, and then change the status to make it reviewable 😉

@MattKelvin MattKelvin changed the title Improve cart performance [WIP] Improve cart performance Apr 7, 2023
@MattKelvin MattKelvin marked this pull request as draft April 7, 2023 09:11
@MattKelvin MattKelvin changed the title [WIP] Improve cart performance Improve cart performance Apr 17, 2023
@MattKelvin MattKelvin marked this pull request as ready for review April 17, 2023 11:20
@MattKelvin
Copy link
Author

Hello hello !

So I tried to improve this PR.

What we need : the quantity per product without taking care of the combinations in order to allow the application (or not) of the specific price.

What I think : we already have this information when we get the products in Product->getProducts() and that we just have to group the quantity per id_product.

I don't know why we needed the retieve the quantity here (https://github.com/PrestaShop/PrestaShop/pull/32058/files#diff-2af4b21c1e4a7fa69015d9f74895abe3135c783d4ad2b87913f754b49ae5106bL326). Because if the quantity doesn't match the quantity present in rowData, is not normal.. But I think we could kept this code and use

($this->cacheAdapter->retrieve($cacheId) != (int) $cartQuantity)

instead of

($cartQuantity = $this->cacheAdapter->retrieve($cacheId) != (int) $quantity)

and the cache should do its job properly.

@kpodemski
Copy link
Contributor

Hello @MattKelvin

There are some issues with your PR after recent commits because automatic tests failed. Almost everything related to shopping cart fails, I think you have a typo in your code the } is in a bit odd place.

@kpodemski kpodemski added the Waiting for author Status: action required, waiting for author feedback label Apr 24, 2023
@MattKelvin
Copy link
Author

Hi @kpodemski

Thank you ! Are you sure ?

I think I had to fix something else. Now I'am waiting for the tests 🤞

@prestashop-issue-bot prestashop-issue-bot bot removed the Waiting for author Status: action required, waiting for author feedback label Apr 27, 2023
tleon
tleon previously approved these changes May 9, 2023
@MattKelvin
Copy link
Author

Hello !

Can someone tell me what to do with that ? :D

@MattKelvin MattKelvin changed the base branch from 8.0.x to 8.1.x December 8, 2023 13:17
@MattKelvin MattKelvin dismissed tleon’s stale review December 8, 2023 13:17

The base branch was changed.

@MattKelvin MattKelvin requested a review from a team as a code owner December 8, 2023 13:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug fix Type: Bug fix develop Branch
Projects
Status: Ready for review
Development

Successfully merging this pull request may close these issues.

None yet

6 participants