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 cache #8409

Merged
merged 13 commits into from Oct 31, 2017

Conversation

Projects
None yet
9 participants
@jocel1
Contributor

jocel1 commented Oct 11, 2017

  • Use multiDelete to invalidate the cache in memcached
  • Implement a lazy LRU to remove from the cache the less used objects
  • Implement in memcached a mecanism which automatically detect when an object is too big
  • Split the big "table => queries" cache into several caches
  • properly delete all the entries from the "table => queries" from multitables queries
Questions Answers
Branch? develop
Description? improve cache efficiency
Type? improvement
Category? CO
BC breaks? no
Deprecations? no
Fixed ticket?
How to test? run test. Use memcached

Important guidelines


This change is Reviewable

@jocel1 jocel1 requested a review from eternoendless Oct 11, 2017

@prestonBot

This comment has been minimized.

Show comment
Hide comment
@prestonBot

prestonBot Oct 11, 2017

Collaborator

Hi!

These(s) commit(s) name(s) seems to be incomplete or malformed, regarding our guidelines:

`Improve cache : - Use multiDelete to invalidate the cache in memcached - Implement a lazy LRU to remove from the cache the less used objects - Implement in memcached a mecanism which automatically detect when an object is too big - Split the big "table => queries" cache into several caches` is malformed or incomplete.

A valid commit name can be, for instance:

BO: Shows company in BO search if B2B is enabled

Would you mind to amend your commits' names?

To do this, open a command line window and use git commit --amend for the commit's name. See GitHub's help page for more information.

Note: this must be done via the command line: you can't do this just by changing the title of the pull-request from the GitHub interface! :)

example

Thank you!

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

Collaborator

prestonBot commented Oct 11, 2017

Hi!

These(s) commit(s) name(s) seems to be incomplete or malformed, regarding our guidelines:

`Improve cache : - Use multiDelete to invalidate the cache in memcached - Implement a lazy LRU to remove from the cache the less used objects - Implement in memcached a mecanism which automatically detect when an object is too big - Split the big "table => queries" cache into several caches` is malformed or incomplete.

A valid commit name can be, for instance:

BO: Shows company in BO search if B2B is enabled

Would you mind to amend your commits' names?

To do this, open a command line window and use git commit --amend for the commit's name. See GitHub's help page for more information.

Note: this must be done via the command line: you can't do this just by changing the title of the pull-request from the GitHub interface! :)

example

Thank you!

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

@jocel1 jocel1 requested a review from Quetzacoalt91 Oct 11, 2017

@tomlev

This comment has been minimized.

Show comment
Hide comment
@tomlev

tomlev Oct 11, 2017

Member

Review status: 0 of 6 files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.


classes/cache/Cache.php, line 136 at r1 (raw file):

     * @param $array
     */
    protected function _deleteMulti($array)

typehint ?


classes/cache/Cache.php, line 177 at r1 (raw file):


    /**
     * @param $value

typehint ?


classes/cache/Cache.php, line 287 at r1 (raw file):

     * Increment the query counter for the given query
     *
     * @param $query

typehint ?


classes/cache/Cache.php, line 319 at r1 (raw file):

        // Get all table from the query and save them in cache
        if ($tables = $this->getTables($query)) {
            foreach ($tables as $table) {

the foreach content could be moved into a separate method to decrease complexity of this one
manipulating arrays is always hard to understand, and should be restricted in very short-scoped methods


classes/cache/Cache.php, line 363 at r1 (raw file):


            if ($tables = $this->getTables($query)) {
                foreach ($tables as $table) {

same as above, could you move this foreach content into a separate method ? code would be easier to read ;)


Comments from Reviewable

Member

tomlev commented Oct 11, 2017

Review status: 0 of 6 files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.


classes/cache/Cache.php, line 136 at r1 (raw file):

     * @param $array
     */
    protected function _deleteMulti($array)

typehint ?


classes/cache/Cache.php, line 177 at r1 (raw file):


    /**
     * @param $value

typehint ?


classes/cache/Cache.php, line 287 at r1 (raw file):

     * Increment the query counter for the given query
     *
     * @param $query

typehint ?


classes/cache/Cache.php, line 319 at r1 (raw file):

        // Get all table from the query and save them in cache
        if ($tables = $this->getTables($query)) {
            foreach ($tables as $table) {

the foreach content could be moved into a separate method to decrease complexity of this one
manipulating arrays is always hard to understand, and should be restricted in very short-scoped methods


classes/cache/Cache.php, line 363 at r1 (raw file):


            if ($tables = $this->getTables($query)) {
                foreach ($tables as $table) {

same as above, could you move this foreach content into a separate method ? code would be easier to read ;)


Comments from Reviewable

@Quetzacoalt91

@PrestaShop PrestaShop deleted a comment from codacy-bot Oct 11, 2017

@tomlev

This comment has been minimized.

Show comment
Hide comment
@tomlev

tomlev Oct 12, 2017

Member

Review status: 0 of 6 files reviewed at latest revision, all discussions resolved, some commit checks failed.


classes/cache/Cache.php, line 319 at r1 (raw file):

Previously, tomlev (Thomas LEVIANDIER) wrote…

the foreach content could be moved into a separate method to decrease complexity of this one
manipulating arrays is always hard to understand, and should be restricted in very short-scoped methods

great, thx


Comments from Reviewable

Member

tomlev commented Oct 12, 2017

Review status: 0 of 6 files reviewed at latest revision, all discussions resolved, some commit checks failed.


classes/cache/Cache.php, line 319 at r1 (raw file):

Previously, tomlev (Thomas LEVIANDIER) wrote…

the foreach content could be moved into a separate method to decrease complexity of this one
manipulating arrays is always hard to understand, and should be restricted in very short-scoped methods

great, thx


Comments from Reviewable

@jocel1

This comment has been minimized.

Show comment
Hide comment
@jocel1

jocel1 Oct 12, 2017

Contributor

I need to add new tests on this one first, the implement was broken, but the issue was not caught by the current tests !

Contributor

jocel1 commented Oct 12, 2017

I need to add new tests on this one first, the implement was broken, but the issue was not caught by the current tests !

@PrestaShop PrestaShop deleted a comment from codacy-bot Oct 12, 2017

@PrestaShop PrestaShop deleted a comment from codacy-bot Oct 12, 2017

@PrestaShop PrestaShop deleted a comment from codacy-bot Oct 12, 2017

@PrestaShop PrestaShop deleted a comment from codacy-bot Oct 12, 2017

@PrestaShop PrestaShop deleted a comment from codacy-bot Oct 12, 2017

@PrestaShop PrestaShop deleted a comment from codacy-bot Oct 12, 2017

@PrestaShop PrestaShop deleted a comment from codacy-bot Oct 13, 2017

@PrestaShop PrestaShop deleted a comment from codacy-bot Oct 13, 2017

@jocel1

This comment has been minimized.

Show comment
Hide comment
@jocel1

jocel1 Oct 14, 2017

Contributor

The latest code version is really for review, it fixes a bunch of existing issues!

Contributor

jocel1 commented Oct 14, 2017

The latest code version is really for review, it fixes a bunch of existing issues!

@xBorderie

This comment has been minimized.

Show comment
Hide comment
@xBorderie

xBorderie Oct 16, 2017

Contributor

"Really" or "ready"? :)

Contributor

xBorderie commented Oct 16, 2017

"Really" or "ready"? :)

@jocel1

This comment has been minimized.

Show comment
Hide comment
@jocel1

jocel1 Oct 16, 2017

Contributor

Really ready :D

Contributor

jocel1 commented Oct 16, 2017

Really ready :D

@prasanthSelvar prasanthSelvar requested review from mbadrani and removed request for mbadrani Oct 16, 2017

@eternoendless

This comment has been minimized.

Show comment
Hide comment
@eternoendless

eternoendless Oct 19, 2017

Member

Reviewed 3 of 6 files at r1, 1 of 3 files at r2, 5 of 5 files at r4.
Review status: all files reviewed at latest revision, 11 unresolved discussions.


classes/cache/Cache.php, line 52 at r4 (raw file):


    /**
     * Max number of queries cached in memcached, for each SQL table

Missing type (int)


classes/cache/Cache.php, line 137 at r4 (raw file):

     * @param array $keyArray
     */
    protected function _deleteMulti($keyArray)

You sign this parameter using array


classes/cache/Cache.php, line 157 at r4 (raw file):


    /**
     * @return mixed

Wrong type (should be int) and missing method description


classes/cache/Cache.php, line 165 at r4 (raw file):


    /**
     * @param mixed $maxCachedObjectsByTable

Wrong type (should be int), missing method description


classes/cache/Cache.php, line 194 at r4 (raw file):


    /**
     * @param bool $value

Missing method description


classes/cache/Cache.php, line 267 at r4 (raw file):

     * @param array $keyArray
     */
    public function deleteMulti($keyArray)

You can sign this parameter using array


classes/cache/Cache.php, line 331 at r4 (raw file):

     * @param array  $result
     *
     * @return void

You can skip this for void functions


classes/cache/Cache.php, line 420 at r4 (raw file):

            $otherTables = $tables;
            unset($otherTables[array_search($table, $tables)]);
            $this->sql_tables_cached[$table][$key] = array('count' => 1, 'otherTables' => $otherTables);

Please prefer one item per line when declaring associative arrays


classes/cache/Cache.php, line 476 at r4 (raw file):


            // sort the array with the query with the lowest count first
            uasort($this->sql_tables_cached[$table], array($this, 'sortByCount'));

You could use an anonymous function here


tests/Unit/UnitTestCase.php, line 32 at r4 (raw file):

    }

}

It doesn't look like this class is used anywhere


tests/Unit/classes/CacheCoreTest.php, line 201 at r4 (raw file):

    }

    public function testCacheInvalidation()

Please describe your tests 😄


Comments from Reviewable

Member

eternoendless commented Oct 19, 2017

Reviewed 3 of 6 files at r1, 1 of 3 files at r2, 5 of 5 files at r4.
Review status: all files reviewed at latest revision, 11 unresolved discussions.


classes/cache/Cache.php, line 52 at r4 (raw file):


    /**
     * Max number of queries cached in memcached, for each SQL table

Missing type (int)


classes/cache/Cache.php, line 137 at r4 (raw file):

     * @param array $keyArray
     */
    protected function _deleteMulti($keyArray)

You sign this parameter using array


classes/cache/Cache.php, line 157 at r4 (raw file):


    /**
     * @return mixed

Wrong type (should be int) and missing method description


classes/cache/Cache.php, line 165 at r4 (raw file):


    /**
     * @param mixed $maxCachedObjectsByTable

Wrong type (should be int), missing method description


classes/cache/Cache.php, line 194 at r4 (raw file):


    /**
     * @param bool $value

Missing method description


classes/cache/Cache.php, line 267 at r4 (raw file):

     * @param array $keyArray
     */
    public function deleteMulti($keyArray)

You can sign this parameter using array


classes/cache/Cache.php, line 331 at r4 (raw file):

     * @param array  $result
     *
     * @return void

You can skip this for void functions


classes/cache/Cache.php, line 420 at r4 (raw file):

            $otherTables = $tables;
            unset($otherTables[array_search($table, $tables)]);
            $this->sql_tables_cached[$table][$key] = array('count' => 1, 'otherTables' => $otherTables);

Please prefer one item per line when declaring associative arrays


classes/cache/Cache.php, line 476 at r4 (raw file):


            // sort the array with the query with the lowest count first
            uasort($this->sql_tables_cached[$table], array($this, 'sortByCount'));

You could use an anonymous function here


tests/Unit/UnitTestCase.php, line 32 at r4 (raw file):

    }

}

It doesn't look like this class is used anywhere


tests/Unit/classes/CacheCoreTest.php, line 201 at r4 (raw file):

    }

    public function testCacheInvalidation()

Please describe your tests 😄


Comments from Reviewable

@eternoendless

This comment has been minimized.

Show comment
Hide comment
@eternoendless

eternoendless Oct 20, 2017

Member

Reviewed 4 of 4 files at r5.
Review status: all files reviewed at latest revision, 2 unresolved discussions.


Comments from Reviewable

Member

eternoendless commented Oct 20, 2017

Reviewed 4 of 4 files at r5.
Review status: all files reviewed at latest revision, 2 unresolved discussions.


Comments from Reviewable

@eternoendless

This comment has been minimized.

Show comment
Hide comment
@eternoendless

eternoendless Oct 20, 2017

Member

:lgtm:


Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from Reviewable

Member

eternoendless commented Oct 20, 2017

:lgtm:


Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from Reviewable

@eternoendless

Thanks for the changes, looks good to me!

@eternoendless eternoendless added this to the 1.7.3.0 milestone Oct 20, 2017

@PrestaShop PrestaShop deleted a comment from codacy-bot Oct 20, 2017

@marionf marionf added QA ✔️ and removed waiting for QA labels Oct 26, 2017

@eternoendless

This comment has been minimized.

Show comment
Hide comment
@eternoendless

eternoendless Oct 30, 2017

Member

@jocel1 could you please rebase?

Member

eternoendless commented Oct 30, 2017

@jocel1 could you please rebase?

jocel1 added some commits Oct 11, 2017

Improve cache :
- Use multiDelete to invalidate the cache in memcached
- Implement a lazy LRU to remove from the cache the less used objects
- Implement in memcached a mecanism which automatically detect when an object is too big
- Split the big "table => queries" cache into several caches
@jocel1

This comment has been minimized.

Show comment
Hide comment
@jocel1

jocel1 Oct 30, 2017

Contributor

done!

Contributor

jocel1 commented Oct 30, 2017

done!

@eternoendless

This comment has been minimized.

Show comment
Hide comment
@eternoendless

eternoendless Oct 30, 2017

Member

Reviewed 1 of 1 files at r6.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

Member

eternoendless commented Oct 30, 2017

Reviewed 1 of 1 files at r6.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@eternoendless

This comment has been minimized.

Show comment
Hide comment
@eternoendless
Member

eternoendless commented Oct 30, 2017

Thank you @jocel1

@codacy-bot

This comment has been minimized.

Show comment
Hide comment
@codacy-bot

codacy-bot Oct 30, 2017

Codacy Here is an overview of what got changed by this pull request:

Issues
======
+ Solved 1
           

Complexity increasing per file
==============================
- classes/cache/CacheMemcache.php  1
- classes/cache/CacheApc.php  1
- classes/cache/CacheXcache.php  1
- classes/cache/CacheMemcached.php  5
- classes/cache/Cache.php  30
         

See the complete overview on Codacy

codacy-bot commented Oct 30, 2017

Codacy Here is an overview of what got changed by this pull request:

Issues
======
+ Solved 1
           

Complexity increasing per file
==============================
- classes/cache/CacheMemcache.php  1
- classes/cache/CacheApc.php  1
- classes/cache/CacheXcache.php  1
- classes/cache/CacheMemcached.php  5
- classes/cache/Cache.php  30
         

See the complete overview on Codacy

@PrestaShop PrestaShop deleted a comment from codacy-bot Oct 30, 2017

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Oct 31, 2017

Contributor

We have approvals, we have QA approvals => LGTM

Contributor

mickaelandrieu commented Oct 31, 2017

We have approvals, we have QA approvals => LGTM

@mickaelandrieu mickaelandrieu merged commit 9f72a8d into PrestaShop:develop Oct 31, 2017

3 checks passed

codacy/pr Good work! A positive pull request.
Details
code-review/reviewable 8 files reviewed
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