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

Optimize specific price computing by executing the query only when needed #8212

Merged
merged 5 commits into from Aug 25, 2017

Conversation

Projects
None yet
4 participants
@jocel1
Contributor

jocel1 commented Aug 1, 2017

Questions Answers
Branch? develop
Description? Optimize specific price by removing a lot of useless queries
Type? improvement
Category? CO
BC breaks? no
Deprecations? no
Fixed ticket http://forge.prestashop.com/browse/BOOM-3701
How to test? verify specific price stuff is properly displayed
@jocel1

This comment has been minimized.

Show comment
Hide comment
@jocel1

jocel1 Aug 1, 2017

Contributor

https://blackfire.io/profiles/compare/e358127c-8cbd-4ccb-98a3-c982ed7d13cb/graph

It's not really visible on this profiling, but it removes a lot of nasty queries.
On my multithreaded benchmarks with high concurrency, the 1.7.2.0 with this fix is almost up to 25% faster (and faster than 1.6 as well)

Contributor

jocel1 commented Aug 1, 2017

https://blackfire.io/profiles/compare/e358127c-8cbd-4ccb-98a3-c982ed7d13cb/graph

It's not really visible on this profiling, but it removes a lot of nasty queries.
On my multithreaded benchmarks with high concurrency, the 1.7.2.0 with this fix is almost up to 25% faster (and faster than 1.6 as well)

@jocel1

This comment has been minimized.

Show comment
Hide comment
@jocel1

jocel1 Aug 1, 2017

Contributor

1 7 2 0-optim

Contributor

jocel1 commented Aug 1, 2017

1 7 2 0-optim

@aleeks aleeks changed the base branch from 1.7.2.x to develop Aug 2, 2017

@aleeks

This comment has been minimized.

Show comment
Hide comment
@aleeks

aleeks Aug 2, 2017

Contributor

I moved it on develop, you need a re-apply your commit, maybe a rebase doesn't work because it'es not the good branch!

Regards

Contributor

aleeks commented Aug 2, 2017

I moved it on develop, you need a re-apply your commit, maybe a rebase doesn't work because it'es not the good branch!

Regards

@jocel1 jocel1 changed the base branch from develop to 1.7.2.x Aug 2, 2017

@aleeks aleeks changed the base branch from 1.7.2.x to develop Aug 2, 2017

@aleeks

This comment has been minimized.

Show comment
Hide comment
@aleeks

aleeks Aug 2, 2017

Contributor

Hello @jocel1
It's not really a bug fix so i move this PR on developbranch, you need to rebase from develop!
Thank you!

Contributor

aleeks commented Aug 2, 2017

Hello @jocel1
It's not really a bug fix so i move this PR on developbranch, you need to rebase from develop!
Thank you!

Show outdated Hide outdated classes/SpecificPrice.php
Show outdated Hide outdated classes/SpecificPrice.php
$real_quantity
)
{
if (self::$_no_specific_values !== null) {

This comment has been minimized.

@eternoendless

eternoendless Aug 21, 2017

Member

Since there's a foreach just after, I think it would be better to check for is_array(self::$_no_specific_values) && !empty(self::$_no_specific_values)

@eternoendless

eternoendless Aug 21, 2017

Member

Since there's a foreach just after, I think it would be better to check for is_array(self::$_no_specific_values) && !empty(self::$_no_specific_values)

This comment has been minimized.

@jocel1

jocel1 Aug 21, 2017

Contributor

actually it checks if the variable has already been initialized (!== null). If it's initialized, it's an array.

@jocel1

jocel1 Aug 21, 2017

Contributor

actually it checks if the variable has already been initialized (!== null). If it's initialized, it's an array.

This comment has been minimized.

@eternoendless

eternoendless Aug 21, 2017

Member

Okay then

@eternoendless

eternoendless Aug 21, 2017

Member

Okay then

Show outdated Hide outdated classes/SpecificPrice.php
Show outdated Hide outdated classes/SpecificPrice.php
Show outdated Hide outdated classes/SpecificPrice.php
Show outdated Hide outdated classes/SpecificPrice.php
(int)$id_customer.'-'.(int)$real_quantity);
}
/**

This comment has been minimized.

@eternoendless

eternoendless Aug 21, 2017

Member

You forgot the method's description here 😉

@eternoendless

eternoendless Aug 21, 2017

Member

You forgot the method's description here 😉

Show outdated Hide outdated classes/SpecificPrice.php
@eternoendless

This comment has been minimized.

Show comment
Hide comment
@eternoendless

eternoendless Aug 21, 2017

Member

I think we can safely integrate this PR into 1.7.2.x. It's not a bug fix but it's a significant performance improvement, and it doesn't look that risky to me.

Member

eternoendless commented Aug 21, 2017

I think we can safely integrate this PR into 1.7.2.x. It's not a bug fix but it's a significant performance improvement, and it doesn't look that risky to me.

@jocel1 jocel1 changed the base branch from develop to 1.7.2.x Aug 21, 2017

@eternoendless eternoendless added this to the 1.7.2.2 milestone Aug 22, 2017

@eternoendless eternoendless changed the title from CO: Optimize specific price computing by executing the query only whe… to Optimize specific price computing by executing the query only when needed Aug 22, 2017

Show outdated Hide outdated classes/SpecificPrice.php
// Note that the variableName from the DB needs to match the function args name
// I.e. if the computeKey args are converted at some point in camelCase, we will need to introduce a
// snakeCase to camelCase conversion of $variableName
foreach (array_keys(self::$_no_specific_values) as $variableName) {

This comment has been minimized.

@eternoendless

eternoendless Aug 22, 2017

Member

Please change new self to static :)

@eternoendless

eternoendless Aug 22, 2017

Member

Please change new self to static :)

This comment has been minimized.

@jocel1

jocel1 Aug 22, 2017

Contributor

actually I wonder if there's a point exposing those internal cache variables as protected, and since there's no point overloading the variable with the same name, I'm changing static / SpecificPrice internal cache variable access to self

@jocel1

jocel1 Aug 22, 2017

Contributor

actually I wonder if there's a point exposing those internal cache variables as protected, and since there's no point overloading the variable with the same name, I'm changing static / SpecificPrice internal cache variable access to self

This comment has been minimized.

@eternoendless

eternoendless Aug 22, 2017

Member

That's actually part of the bigger issue of "is there a point to allowing overrides really?"

@eternoendless

eternoendless Aug 22, 2017

Member

That's actually part of the bigger issue of "is there a point to allowing overrides really?"

// been computed yet
$key = null;
} else {
$key = self::computeKey(

This comment has been minimized.

@eternoendless

eternoendless Aug 22, 2017

Member

Method calls should be static, don't you think?

@eternoendless

eternoendless Aug 22, 2017

Member

Method calls should be static, don't you think?

This comment has been minimized.

@jocel1

jocel1 Aug 22, 2017

Contributor

mmm for me it should remains self. We don't want to encourage overloading of such internal functions (especially if we want to introduce later an API).
I actually think it would be great to declare those internal functions as final

@jocel1

jocel1 Aug 22, 2017

Contributor

mmm for me it should remains self. We don't want to encourage overloading of such internal functions (especially if we want to introduce later an API).
I actually think it would be great to declare those internal functions as final

@eternoendless

This comment has been minimized.

Show comment
Hide comment
@eternoendless
Member

eternoendless commented Aug 25, 2017

Thank you @jocel1

@eternoendless eternoendless merged commit c162b87 into PrestaShop:1.7.2.x Aug 25, 2017

1 of 2 checks passed

codacy/pr Not so good... This pull request quality could be better.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment