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

Improved performance of Product List page #8690

Merged
merged 8 commits into from Aug 8, 2018

Conversation

Projects
None yet
6 participants
@mickaelandrieu
Contributor

mickaelandrieu commented Jan 20, 2018

Questions Answers
Branch? develop
Description? We have more than 160 SQL queries executed on the Product List Page with 7 products, it's TOO much
Type? improvement
Category? BO
BC breaks? yes (an empty function became abstract)
Deprecations? no
How to test? The page should work as good as before, but faster

Blackfire report

perf_improvements

Important guidelines


This change is Reviewable

@eternoendless

This comment has been minimized.

Show comment
Hide comment
@eternoendless

eternoendless Jan 23, 2018

Member

Nice one! 👏


Reviewed 3 of 5 files at r1, 4 of 4 files at r2.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.


classes/Access.php, line 122 at r2 (raw file):

                FROM `'._DB_PREFIX_.'module_access` ma 
                WHERE ma.`id_profile` = "'.$idProfile.'"
            )

Wouldn't this be simpler?

SELECT r.`slug`
FROM `'._DB_PREFIX_.'authorization_role` r
INNER JOIN `'._DB_PREFIX_.'module_access` ma ON ma.`id_authorization_role` = r.`id_authorization_role`
WHERE ma.`id_profile` = "'.$idProfile.'"

src/Adapter/Product/AdminProductDataProvider.php, line 82 at r2 (raw file):

    public function getPersistedFilterParameters()
    {
        static $filters = null;

This cache should be cleanable


src/PrestaShopBundle/Security/Admin/EmployeeProvider.php, line 61 at r1 (raw file):

Previously, mickaelandrieu (Mickaël Andrieu) wrote…

as soon as my build is green, I'll try to introduce Symfony cache for this class only in a specific commit.

Ok but that's a blocker though :)


Comments from Reviewable

Member

eternoendless commented Jan 23, 2018

Nice one! 👏


Reviewed 3 of 5 files at r1, 4 of 4 files at r2.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.


classes/Access.php, line 122 at r2 (raw file):

                FROM `'._DB_PREFIX_.'module_access` ma 
                WHERE ma.`id_profile` = "'.$idProfile.'"
            )

Wouldn't this be simpler?

SELECT r.`slug`
FROM `'._DB_PREFIX_.'authorization_role` r
INNER JOIN `'._DB_PREFIX_.'module_access` ma ON ma.`id_authorization_role` = r.`id_authorization_role`
WHERE ma.`id_profile` = "'.$idProfile.'"

src/Adapter/Product/AdminProductDataProvider.php, line 82 at r2 (raw file):

    public function getPersistedFilterParameters()
    {
        static $filters = null;

This cache should be cleanable


src/PrestaShopBundle/Security/Admin/EmployeeProvider.php, line 61 at r1 (raw file):

Previously, mickaelandrieu (Mickaël Andrieu) wrote…

as soon as my build is green, I'll try to introduce Symfony cache for this class only in a specific commit.

Ok but that's a blocker though :)


Comments from Reviewable

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Jan 24, 2018

Contributor

Wouldn't this be simpler?

Maybe, but is it more performant? Provide a graph please :)

This cache should be cleanable

Ok but that's a blocker though :)

Yes, it's on my free time so we need to be patient ^^

Contributor

mickaelandrieu commented Jan 24, 2018

Wouldn't this be simpler?

Maybe, but is it more performant? Provide a graph please :)

This cache should be cleanable

Ok but that's a blocker though :)

Yes, it's on my free time so we need to be patient ^^

@eternoendless

This comment has been minimized.

Show comment
Hide comment
@eternoendless

eternoendless Jan 26, 2018

Member

Reviewed 1 of 1 files at r3, 3 of 3 files at r4.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.


classes/Access.php, line 122 at r2 (raw file):

Maybe, but is it more performant? Provide a graph please :)

I'm under the impression that a JOIN is better than an IN ( <subquery> ), but looks like MariaDB is smart enough to optimize it on its own to the same query (I'm not sure about MySQL... @jocel1?):

explain SELECT r.`slug`
FROM `ps_authorization_role` r
INNER JOIN `ps_module_access` ma
ON ma.`id_authorization_role` = r.`id_authorization_role`
WHERE ma.`id_profile` = 1;
id select_type table type possible_keys key key_len ref rows Extra
1 SIMPLE ma ref PRIMARY PRIMARY 4 const 216 Using index
1 SIMPLE r eq_ref PRIMARY PRIMARY 4 ps173x.ma.id_authorization_role 1
explain SELECT r.`slug`
FROM `ps_authorization_role` r
WHERE r.`id_authorization_role` IN (
    SELECT ma.`id_authorization_role`
    FROM `ps_module_access` ma
    WHERE ma.`id_profile` = 1
)
id select_type table type possible_keys key key_len ref rows Extra
1 PRIMARY ma ref PRIMARY PRIMARY 4 const 216 Using index
1 PRIMARY r eq_ref PRIMARY PRIMARY 4 ps173x.ma.id_authorization_role 1

Run on MariaDB 10.2.6

--

Anyway, personally, I find the JOIN syntax easier to understand.


config/defines.inc.php, line 29 at r4 (raw file):

/* Debug only */
if (!defined('_PS_MODE_DEV_')) {
    define('_PS_MODE_DEV_', false);

Should be kept to true


src/PrestaShopBundle/Security/Admin/EmployeeProvider.php, line 61 at r1 (raw file):

Yes, it's on my free time so we need to be patient ^^

Sure, no rush! 😎


Comments from Reviewable

Member

eternoendless commented Jan 26, 2018

Reviewed 1 of 1 files at r3, 3 of 3 files at r4.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.


classes/Access.php, line 122 at r2 (raw file):

Maybe, but is it more performant? Provide a graph please :)

I'm under the impression that a JOIN is better than an IN ( <subquery> ), but looks like MariaDB is smart enough to optimize it on its own to the same query (I'm not sure about MySQL... @jocel1?):

explain SELECT r.`slug`
FROM `ps_authorization_role` r
INNER JOIN `ps_module_access` ma
ON ma.`id_authorization_role` = r.`id_authorization_role`
WHERE ma.`id_profile` = 1;
id select_type table type possible_keys key key_len ref rows Extra
1 SIMPLE ma ref PRIMARY PRIMARY 4 const 216 Using index
1 SIMPLE r eq_ref PRIMARY PRIMARY 4 ps173x.ma.id_authorization_role 1
explain SELECT r.`slug`
FROM `ps_authorization_role` r
WHERE r.`id_authorization_role` IN (
    SELECT ma.`id_authorization_role`
    FROM `ps_module_access` ma
    WHERE ma.`id_profile` = 1
)
id select_type table type possible_keys key key_len ref rows Extra
1 PRIMARY ma ref PRIMARY PRIMARY 4 const 216 Using index
1 PRIMARY r eq_ref PRIMARY PRIMARY 4 ps173x.ma.id_authorization_role 1

Run on MariaDB 10.2.6

--

Anyway, personally, I find the JOIN syntax easier to understand.


config/defines.inc.php, line 29 at r4 (raw file):

/* Debug only */
if (!defined('_PS_MODE_DEV_')) {
    define('_PS_MODE_DEV_', false);

Should be kept to true


src/PrestaShopBundle/Security/Admin/EmployeeProvider.php, line 61 at r1 (raw file):

Yes, it's on my free time so we need to be patient ^^

Sure, no rush! 😎


Comments from Reviewable

@jocel1

This comment has been minimized.

Show comment
Hide comment
@jocel1

jocel1 Jan 26, 2018

Contributor

classes/Access.php, line 122 at r2 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

Maybe, but is it more performant? Provide a graph please :)

I'm under the impression that a JOIN is better than an IN ( <subquery> ), but looks like MariaDB is smart enough to optimize it on its own to the same query (I'm not sure about MySQL... @jocel1?):

explain SELECT r.`slug`
FROM `ps_authorization_role` r
INNER JOIN `ps_module_access` ma
ON ma.`id_authorization_role` = r.`id_authorization_role`
WHERE ma.`id_profile` = 1;
id select_type table type possible_keys key key_len ref rows Extra
1 SIMPLE ma ref PRIMARY PRIMARY 4 const 216 Using index
1 SIMPLE r eq_ref PRIMARY PRIMARY 4 ps173x.ma.id_authorization_role 1
explain SELECT r.`slug`
FROM `ps_authorization_role` r
WHERE r.`id_authorization_role` IN (
    SELECT ma.`id_authorization_role`
    FROM `ps_module_access` ma
    WHERE ma.`id_profile` = 1
)
id select_type table type possible_keys key key_len ref rows Extra
1 PRIMARY ma ref PRIMARY PRIMARY 4 const 216 Using index
1 PRIMARY r eq_ref PRIMARY PRIMARY 4 ps173x.ma.id_authorization_role 1

Run on MariaDB 10.2.6

--

Anyway, personally, I find the JOIN syntax easier to understand.

Always prefer JOIN over IN (subquery) if you can, earlier version of MySQL are not able to optimise them efficiently (but MariaDB and recent version of MySQL are able to optimize them)


Comments from Reviewable

Contributor

jocel1 commented Jan 26, 2018

classes/Access.php, line 122 at r2 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

Maybe, but is it more performant? Provide a graph please :)

I'm under the impression that a JOIN is better than an IN ( <subquery> ), but looks like MariaDB is smart enough to optimize it on its own to the same query (I'm not sure about MySQL... @jocel1?):

explain SELECT r.`slug`
FROM `ps_authorization_role` r
INNER JOIN `ps_module_access` ma
ON ma.`id_authorization_role` = r.`id_authorization_role`
WHERE ma.`id_profile` = 1;
id select_type table type possible_keys key key_len ref rows Extra
1 SIMPLE ma ref PRIMARY PRIMARY 4 const 216 Using index
1 SIMPLE r eq_ref PRIMARY PRIMARY 4 ps173x.ma.id_authorization_role 1
explain SELECT r.`slug`
FROM `ps_authorization_role` r
WHERE r.`id_authorization_role` IN (
    SELECT ma.`id_authorization_role`
    FROM `ps_module_access` ma
    WHERE ma.`id_profile` = 1
)
id select_type table type possible_keys key key_len ref rows Extra
1 PRIMARY ma ref PRIMARY PRIMARY 4 const 216 Using index
1 PRIMARY r eq_ref PRIMARY PRIMARY 4 ps173x.ma.id_authorization_role 1

Run on MariaDB 10.2.6

--

Anyway, personally, I find the JOIN syntax easier to understand.

Always prefer JOIN over IN (subquery) if you can, earlier version of MySQL are not able to optimise them efficiently (but MariaDB and recent version of MySQL are able to optimize them)


Comments from Reviewable

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Jan 26, 2018

Contributor

Even if build is green, the pr is not yet finished: as default cache of Symfony is file system and not static array, I need to declare a new adapter.

Contributor

mickaelandrieu commented Jan 26, 2018

Even if build is green, the pr is not yet finished: as default cache of Symfony is file system and not static array, I need to declare a new adapter.

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Jan 27, 2018

Contributor

I've used the Symfony static cache, so we can rely on Symfony services/commands to clear the cache if needed. I've also updated the link of the Blackfire graph.

This time, I've finished the first bash of improvements: thanks Blackfire 👼

Contributor

mickaelandrieu commented Jan 27, 2018

I've used the Symfony static cache, so we can rely on Symfony services/commands to clear the cache if needed. I've also updated the link of the Blackfire graph.

This time, I've finished the first bash of improvements: thanks Blackfire 👼

@Quetzacoalt91

We have a implementation of the Symfony Cache based on PSR-6 here.

Does the app. in your cache keys make the tags available?

/* @var $filter AdminFilter */
if (!$filter) {
return AdminFilter::getProductCatalogEmptyFilter();
$employeeId = $employee->id ?: 0;

This comment has been minimized.

@Quetzacoalt91

Quetzacoalt91 Jan 29, 2018

Member

For people wondering what is the compatibility for a shorthand ternary test: PHP 5.3 required.

@Quetzacoalt91

Quetzacoalt91 Jan 29, 2018

Member

For people wondering what is the compatibility for a shorthand ternary test: PHP 5.3 required.

@eternoendless

This comment has been minimized.

Show comment
Hide comment
@eternoendless

eternoendless Jan 29, 2018

Member

Reviewed 4 of 6 files at r5, 3 of 3 files at r6.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


classes/controller/Controller.php, line 648 at r6 (raw file):

     * @return Symfony\Component\DependencyInjection\ContainerBuilder
     */
    abstract protected function buildContainer();

This is BC break in my opinion


src/PrestaShopBundle/Security/Admin/EmployeeProvider.php, line 68 at r6 (raw file):

    public function loadUserByUsername($username)
    {
        $cacheKey = sha1($username);

Why the sha1?


Comments from Reviewable

Member

eternoendless commented Jan 29, 2018

Reviewed 4 of 6 files at r5, 3 of 3 files at r6.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


classes/controller/Controller.php, line 648 at r6 (raw file):

     * @return Symfony\Component\DependencyInjection\ContainerBuilder
     */
    abstract protected function buildContainer();

This is BC break in my opinion


src/PrestaShopBundle/Security/Admin/EmployeeProvider.php, line 68 at r6 (raw file):

    public function loadUserByUsername($username)
    {
        $cacheKey = sha1($username);

Why the sha1?


Comments from Reviewable

@Quetzacoalt91

This comment has been minimized.

Show comment
Hide comment
@Quetzacoalt91

Quetzacoalt91 Jan 29, 2018

Member

classes/controller/Controller.php, line 648 at r6 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

This is BC break in my opinion

As long as AdminController & FrontController declared the method, I'm confident is saying this won't break the compatibility.


Comments from Reviewable

Member

Quetzacoalt91 commented Jan 29, 2018

classes/controller/Controller.php, line 648 at r6 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

This is BC break in my opinion

As long as AdminController & FrontController declared the method, I'm confident is saying this won't break the compatibility.


Comments from Reviewable

@eternoendless

This comment has been minimized.

Show comment
Hide comment
@eternoendless

eternoendless Jan 29, 2018

Member

Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


classes/controller/Controller.php, line 648 at r6 (raw file):

Previously, Quetzacoalt91 (Thomas N) wrote…

As long as AdminController & FrontController declared the method, I'm confident is saying this won't break the compatibility.

Still needs to be documented in case people extend Controller directly


Comments from Reviewable

Member

eternoendless commented Jan 29, 2018

Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


classes/controller/Controller.php, line 648 at r6 (raw file):

Previously, Quetzacoalt91 (Thomas N) wrote…

As long as AdminController & FrontController declared the method, I'm confident is saying this won't break the compatibility.

Still needs to be documented in case people extend Controller directly


Comments from Reviewable

@Quetzacoalt91

This comment has been minimized.

Show comment
Hide comment
@Quetzacoalt91

Quetzacoalt91 Jan 29, 2018

Member

By the way, I used the cache system provided by Doctrine. If we use the system coming from Symfony, we will have to replace the old one.

Member

Quetzacoalt91 commented Jan 29, 2018

By the way, I used the cache system provided by Doctrine. If we use the system coming from Symfony, we will have to replace the old one.

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Jan 30, 2018

Contributor

This is BC break in my opinion

I'm in favor of document it, but where? We don't have a changelog, didn't we?

Why the sha1?

Cache component doesn't like spaces and special characters like @.

By the way, I used the cache system provided by Doctrine. If we use the system coming from Symfony, we will have to replace the old one.

Except if I'm wrong, Cache component provides Doctrine support: https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Cache/Adapter/DoctrineAdapter.php.

I see no hurry here for this pull request. You asked me to implement something more reliable than static PHP cache and I did it. Introducing Symfony Cache component everywhere in PrestaShop is a massive task where we need to define:

  • what needs to be cached;
  • how many time: during a request, during 1 hour or during 1 week?

For each page of PrestaShop. Looking at the current roadmap, this may be planned and released in 1.7.5.

Contributor

mickaelandrieu commented Jan 30, 2018

This is BC break in my opinion

I'm in favor of document it, but where? We don't have a changelog, didn't we?

Why the sha1?

Cache component doesn't like spaces and special characters like @.

By the way, I used the cache system provided by Doctrine. If we use the system coming from Symfony, we will have to replace the old one.

Except if I'm wrong, Cache component provides Doctrine support: https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Cache/Adapter/DoctrineAdapter.php.

I see no hurry here for this pull request. You asked me to implement something more reliable than static PHP cache and I did it. Introducing Symfony Cache component everywhere in PrestaShop is a massive task where we need to define:

  • what needs to be cached;
  • how many time: during a request, during 1 hour or during 1 week?

For each page of PrestaShop. Looking at the current roadmap, this may be planned and released in 1.7.5.

@eternoendless

This comment has been minimized.

Show comment
Hide comment
@eternoendless

eternoendless Feb 7, 2018

Member

Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


classes/controller/Controller.php, line 648 at r6 (raw file):

I'm in favor of document it, but where? We don't have a changelog, didn't we?

You can put it here:

Screen Shot 2018-02-07 at 17.12.30.png


src/PrestaShopBundle/Security/Admin/EmployeeProvider.php, line 68 at r6 (raw file):

Cache component doesn't like spaces and special characters like @.

Ok, please add a comment explaining that so that other people will know :)


Comments from Reviewable

Member

eternoendless commented Feb 7, 2018

Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


classes/controller/Controller.php, line 648 at r6 (raw file):

I'm in favor of document it, but where? We don't have a changelog, didn't we?

You can put it here:

Screen Shot 2018-02-07 at 17.12.30.png


src/PrestaShopBundle/Security/Admin/EmployeeProvider.php, line 68 at r6 (raw file):

Cache component doesn't like spaces and special characters like @.

Ok, please add a comment explaining that so that other people will know :)


Comments from Reviewable

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Mar 7, 2018

Contributor

We have a implementation of the Symfony Cache based on PSR-6 here.
Does the app. in your cache keys make the tags available?

Good question: regarding the type hinted interface, the answer is no. But as TagAwareAdapterInterface extends the previous one (in the end), it may be the case.

Before introduce tags, we need a strategy for caching in PrestaShop: introducing tags later won't break the API 👍

Contributor

mickaelandrieu commented Mar 7, 2018

We have a implementation of the Symfony Cache based on PSR-6 here.
Does the app. in your cache keys make the tags available?

Good question: regarding the type hinted interface, the answer is no. But as TagAwareAdapterInterface extends the previous one (in the end), it may be the case.

Before introduce tags, we need a strategy for caching in PrestaShop: introducing tags later won't break the API 👍

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu May 28, 2018

Contributor

ping @eternoendless, this PR was already reviewed but it was too risky to be merged in 1.7.4.0 I guess.

Anyway, this is now up to date with develop branch :)

Contributor

mickaelandrieu commented May 28, 2018

ping @eternoendless, this PR was already reviewed but it was too risky to be merged in 1.7.4.0 I guess.

Anyway, this is now up to date with develop branch :)

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Aug 6, 2018

Contributor

ping @eternoendless, this PR was already reviewed but it was too risky to be merged in 1.7.4.0. Let's make them available in develop branch asap to prevent bugs if introduced, don't you think?

Anyway, this is now up to date with develop branch :)

Contributor

mickaelandrieu commented Aug 6, 2018

ping @eternoendless, this PR was already reviewed but it was too risky to be merged in 1.7.4.0. Let's make them available in develop branch asap to prevent bugs if introduced, don't you think?

Anyway, this is now up to date with develop branch :)

@eternoendless

Reviewed 9 of 10 files at r7.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Quetzacoalt91)


src/PrestaShopBundle/Security/Admin/EmployeeProvider.php, line 59 at r7 (raw file):

    /**
     * Fetch the Employee entity that matches the given username.
     * Cache system doesn't supports "@" character, so we rely on a sha1 expression.

This comment is about implementation, so it should go in the appropriate line (just above sha1(...))

@marionf marionf self-assigned this Aug 7, 2018

@marionf marionf added QA ✔️ and removed waiting for QA labels Aug 7, 2018

@marionf marionf removed their assignment Aug 7, 2018

@PierreRambaud PierreRambaud merged commit dcbbd5a into PrestaShop:develop Aug 8, 2018

1 of 3 checks passed

Codacy/PR Quality Review Not up to standards. This pull request quality could be better.
Details
code-review/reviewable 1 discussion left (Quetzacoalt91)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@PierreRambaud PierreRambaud deleted the mickaelandrieu:performant-list-product-page branch Aug 8, 2018

@PierreRambaud

This comment has been minimized.

Show comment
Hide comment
@PierreRambaud

PierreRambaud Aug 8, 2018

Contributor

Thanks @mickaelandrieu and everyone who works on it.

Contributor

PierreRambaud commented Aug 8, 2018

Thanks @mickaelandrieu and everyone who works on it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment