search

mikebronner / laravel-model-caching

Eloquent model-caching made easy.
MIT License
2.26k stars 217 forks source link

Performance issue with `checkCooldownAndRemoveIfExpired` #209

Closed salkhwlani closed 5 years ago

salkhwlani commented 5 years ago

Describe the bug

I was testing the performance of the website with a huge number of requests in sametime but I found very critical issue with performance because of checkCooldownAndRemoveIfExpired function

its increase the response time from < 1s to > ~9s for each request when the server tried to serve > 400 requests per second, my test was done using different settings and using load balance for the web server and cache servers

after removing checkCooldownAndRemoveIfExpired from source code and use only model cache the server serves more than 2000 request per second without any issues or effect in response time

by the way, I don't use cool-down in my code

Environment

Perfomrnace with checkCooldownAndRemoveIfExpired

image

image

Perfomance without checkCooldownAndRemoveIfExpired

image

mikebronner commented 5 years ago

Thanks for submitting this in detail! I will take a further look into this, hopefully this weekend. :)

mycarrysun commented 5 years ago

I can confirm - I found the same thing by analyzing the cache misses and that it is looking for the cooldown data even if it isn't there.

Are we able to add this to the config file somehow?

I may write up a PR if I have some time.

mikebronner commented 5 years ago

Yes, a PR would help out tremendously. I haven't had a chance to analyze this issue yet. :(

mycarrysun commented 5 years ago

@mikebronner Just added - let me know what you think. The tests are failing somewhere before my tests run so not sure what's going wrong there.

Says: 

1) GeneaLabs\LaravelModelCaching\Tests\Integration\CachedBuilder\WhereJsonContainsTest::testWithInUsingCollectionQuery
Illuminate\Database\QueryException: SQLSTATE[08006] [7] timeout expired (SQL: select tablename from pg_catalog.pg_tables where schemaname = 'public')
mikebronner commented 5 years ago

Hi @yemenifree and @mycarrysun, would you guys mind testing release 0.4.10 to see if that fixes it? Please see the updated readme for enabling cache cool-down if you wish to use it. If not, you shouldn't have to make any changes. Please keep me updated as to how your performance testing goes. Thanks for all your help!

mikebronner commented 5 years ago

@mycarrysun I have updated the cachecooldown property on models to reflect seconds instead of a boolean, as we discussed yesterday.

mycarrysun commented 5 years ago

Awesome! What version is this in?

mikebronner commented 5 years ago

0.4.11: https://github.com/GeneaLabs/laravel-model-caching/commit/eeb78e0a2b0dc1ffdfe2d2669c5d3bc3d2274c6b

mikebronner commented 5 years ago

Closing this issue for now. Please let me know if there is still a performance impact! Thanks for your guys's help on this!

mycarrysun commented 5 years ago

Sorry for the late reply - just upgraded to 0.4.11 and is working as expected!

mikebronner commented 5 years ago

@mycarrysun Awesome! Thanks for following up. :)