On sites that only use the default price alterer, it is apparently quicker to just alter and format a price in uc_price() than it is to build a cache key and check it. This is partially because building the cache key involves serializing and hashing the entire price object, which could contain an entire node object and more. The attached patch disables building of the cache key if price caching is disabled.
I used "ab" to benchmark a heavy Views page with a large number of products and prices:
Price caching enabled: 1609.072ms
Price caching disabled, without patching: 1552.418ms
Price caching disabled, with attached patch: 1475.124ms
I have not yet tested this with any contributed price alterers enabled (uc_vat, etc) - it's possible that price caching may be a false economy in more situations.
| Comment | File | Size | Author |
|---|---|---|---|
| #8 | 948324-remove-price-cache.patch | 8.92 KB | longwave |
| #6 | 948324-remove-price-cache.patch | 9.14 KB | longwave |
| uc_price_key_cache.patch | 1.9 KB | longwave |
Comments
Comment #1
Island Usurper commentedSo the 'cache' option is already FALSE by default? Or are there certain situations where it is turned off automatically?
Comment #2
longwaveThe 'cache' option is TRUE by default, and is only controlled by the checkbox on the price handler settings page as far as I can see.
I believe the patch as it stands is worth applying, as if the cache is disabled there is no need to waste CPU cycles building a complex cache key that will never be used, but I think these findings also warrant further investigation into whether the default should be changed to FALSE (and the checkbox description altered appropriately), or indeed if the price cache is needed at all.
Comment #3
longwaveCommitted the original patch. Price caching is still enabled by default. Does not apply to 7.x.
Comment #5
longwaveReopening to look at removing price caching altogether, as supported by Island Usurper in #757604-16: Fatal error: Uncaught exception 'Exception' with message 'Serialization of 'SimpleXMLElement' is not allowed'
Comment #6
longwavePatch attached that removes price caching completely.
Comment #7
YK85 commentedsubscribing
Comment #8
longwaveFinal attempt at a patch for this. I intend to commit this fairly soon if there are no objections; the multicurrency support I am building is not compatible with price caching, and the performance issues noted in #0 still apply.
Comment #9
longwaveCommitted.
Comment #10
hanoiiI am also currently benchmarking a site and uc_price is something of a bottleneck, I wasn't really thinking of removing cache altogether but definitely suggesting modifications.
I have one comment though related to this which might bring a little further modification.
node_load() is also a hit in performance. I have a view with 120 products, in which both sell price and list price as being retrieved and rendered, so node_load() is called 240 times for that view. As node_load() has a static cache inside, it's likely that only 120 is making the performance hit I am noticing (around 1.5s). Considering this, I wonder if we should be loading nodes only to alter prices? Couldn't this be improved somehow, and if we indeed need the node object which is probably the simplest case, can't we rely somehow the object loading to the uc_price() function and in that case, maybe cache the whole thing so node is not needed to just alter a price that it's likely not to change?
I know this is a bit far to be done and probably needs discussion but I do find it an interesting issue to try to fix. I think basically the whole uc_price API might need a bit of rethinking just to make it a little bit simpler to work with.
The main problem I see with what I am suggesting is that we don't only have the node as object but we can have any object that's needed by any alterer (I wonder if there are other alterers out there rather than uc_vat?). So is there a way to give in something in the context and let uc_price() load things as necessary. A cache here I think it's important, what we need is to properly figure out a good fast key to cache and try to put everything that's not needed to be done when the price is cached afterwords.
Just throwing some thoughts for ideas.
Comment #11
longwave@hanoii: In your case isn't it easiest to apply caching to your view output, so all 240 prices only have to be rendered once and then Views doesn't have to call uc_price(), node_load(), etc when loading the page for a second time?
The problem with building the cache key is that we can't be sure what will affect the price - someone might have written a variant on Price Per Role that needs the role (or entire user object) to be part of the cache key, in the case of multicurrency you would need the currency to be part of the cache key, if you have uc_discount with a date-based discount set up in CA then the current time needs to be part of the key, etc. Therefore I think handing off caching to further down the line would work better - is there any use case other than Views where you will be altering and rendering a large number of prices?
Comment #12
longwaveI don't think this will be improved in 6.x now, and I can't see a way of building a sensible cache key when we don't know what will affect the prices. Caching node, view or full page output will always work better than caching just the prices.