Enable website config cache #2355
Conversation
I want to keep v19 and v20 a close as possible together ... if its not breaking anything, we should consider to add this to v19. |
|
enabling website cache on a multiwebsite setup will make the cache size grow so that's why I thought not to enable it in the LTS where people may check less? |
fballiano
force-pushed
the
enable_website_config_cache
branch
from
a266c26
to
77c161e
Compare
|
Is it possible to make this configurable in the backend? System > Configuration > Advanced/System > Advanced Cache Settings? If the default is set to No, then there will be less issue with v19. |
fballiano
force-pushed
the
enable_website_config_cache
branch
from
77c161e
to
c4f5bf2
Compare
|
Actually, thinking about it, it could be enabled for everybody (v19 too) because at the end of the day we're talking only about configuration cache, not all the caches, what do you think @kiatng? |
|
I agree with @kiatng. it needs an option in Backend, No by Default. Comment with explanation what happens if it is set to Yes in performance, warning the cache increases. PR available for both versions. We basically have the Magento version, but with the possibility of changing the behavior based on an option. |
|
I'm not sure I agree, this would be a highly technical config that most probably nobody will enable but it totally should be, al least for multistores setup (for single stores there's no difference in having it enabled or disabled but having it enabled doesn't create any problem). Without it, there are multiple places that keep reading the xml files at every requests, that should not happen in any installation. The "problem" about cache size I was referring to is extremely limited in my opinion, we're talking about config cache only, not the whole cache. Maybe a store with 10-20 websites will have a cache increase that maybe won't fit they're redis (although that is to be seen) but if they have that big of a project they will also recognize that they're redis is running out of memory and adjust it (again, I don't even think that will happen). IMHO It is much worse to constanly impact IO on file_system and this setting should be on at least for OM20. I'd like to have @sreichel opinion on that. but stores and websites should be on by default. |
I'd like to pass the ball to @colinmollenhour, who has a good knowlegde about Magentos caching. (i'll test too, but this could take some days) |
|
I still very much prefer configurable approach with a No default. I happen to have a plan to update production to PHP8 and the latest OM by end of year. I have 2 concerns:
This is because I do not know much about cache, I do not know for certain that this PR won't break production. |
|
We've it in production since 1 month, zero problems, addtocart (which is obviously not cached by varnish) went from 400msec to 200msec, it's half the time. The fact that, with config cache enabled, it keeps loading the xml files, it is a bug that should be fixed. I believe this should be on by default as it is already kinda configurable enabling/disabling "config cache". |
|
Great find!
I agree, this is arguably a bug fix and not just an optimization.. Are we talking ~5MB per website? For single-website users this is negligible space. For sites with a large number of websites where this could make a difference I would expect them to have some decent testing procedures in place and a good cache backend with some extra capacity and monitoring in place. For those using the file backend it should be completely inconsequential and for those using Redis it is easily worth provisioning the extra RAM, in my opinion. Regarding v20 I would definitely keep it as-is. Regarding v19 I don't really have a good sense as to who is using v19 and why so will defer to others although I would add that at some point all of the extra work required to maintain v19 or even have these type of discussions for every PR is just wearing on the project. It seems we need a clear definition of what goes into v19 and what doesn't. IMO it should be security issues only as it seems v19 users main goal is for everything to remain 100% stable while being protected from security vulnerabilities. If they are interested in all sorts of other improvements just upgrade to v20, right? I'd like to somehow see the v19 discussions separated from the original PR so that PRs for improvements aren't slowed by the v19 discussion. Perhaps after a v20 PR is merged a second PR can be opened for v19 by any maintainer or community member that wants the PR in v19 and then discussion regarding v19 can continue there? |
|
Here a cache size before/after this change: Before: After: With: $app = Mage::app();
$ids = $app->getCache()->getIds();
$cache = [];
foreach ($ids as $id)
$cache[$id] = floor(strlen($app->loadCache($id)) / 1024).'k';
natsort($cache);
$cache = array_reverse($cache); |
|
From what I can see, CONFIG_GLOBAL_WEBSITE_WXX, which has the same value as CONFIG_GLOBAL_STORES_XX, appears in addition, as was natural. Does anyone have an idea why the letter "W" appears in front of the country code? Shouldn't it be WEBSITE_XX, where XX is the country code, similar to STORES_XX? |
|
This is our store names (FR...) and website names (WFR...). |
|
Now I understand, they are the names established by you. |
This PR seems to be a big performance improvement for multiwebsite setups. @luigifab and me, while debugging some performance issues about the "addtocart" feature, discovered that "website based configs" seems not to be cached (while "store based configs" are).
I'll try to explain, when adding to cart we found out that that
Mage_Catalog_Model_Product_Attribute_Backend_Groupprice_Abstract::_getWebsiteCurrencyRates()was called but wasn't using the cache to extract the values that it needed, in fact if you check blackfire you'll see it keep loading the configuration xml (and this is with hot cache):After a big debugging session it all came out to enabling website config cache in
app/code/core/Mage/Core/Model/Config.php. I've no idea why it was disabled (git blame tells that code is 13 years old) but enabling it result in a conspicuous performance improvement, which is mostly visible only in multiwebsite environments.After applying the patch all "website level configurations" get cached and you can see that, the same blackfire test as before, doesn't call the xml anymore and it's so much faster:
A small issue with this PR (reason why I targeted for v20) is that the cache size will increase depending on how many websites the project has.
Related Pull Requests
#2351
Manual testing scenarios (*)
This is tricky. One of the possible tests is:
Mage::getModel("catalog/product")->load(SOMEPRODUCTID)with and without the patch (multiple times, so that the cache is hot)I had a much more complicated test but it should be possible to verify the problem with this one.