Skip to content
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

Fix incorrect cache usage by available_properties (big cache use, slo… #86

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mrbrdo
Copy link
Contributor

@mrbrdo mrbrdo commented Oct 2, 2024

…w reads)

Spree::Filters::PropertyPresenter includes all ProductProperty for all Products, which can be very many. This would be serialized and stored in the cache by Rails.cache.fetch. As only uniq_values are actually relevant and being used, all the ProductProperty's should not be stored in cache. The number of these properties can be very significant (in the 1000s), taking up a lot of cache and making the cache read very slow (over 1 second with solid_cache).

This fixes two issues, which were for me quite big:

  • very bad performance on reading the property filters from cache, which can actually be slower than the queries themselves (in my case I have observed 1.5s just for the cache read)
  • incredibly high cache space use due to storing many unneeded records in the cache - due to web crawlers, I've actually had 100GB of disk filled with fragment caches within a day or two (there might be other culprits, but this one surely contributed)

@damianlegawiec I'm not sure how this is handled with api v2, but you might want to check in case you use cache there. PropertyPresenter is not designed well to be cached directly, as it was being used now.

The solution could be better by redesigning Spree::Filters::PropertiesPresenter/PropertyPresenter, but I didn't want to fiddle too much with the core gem in this case.
I think the same issue could be there with OptionsPresenter, but I didn't have time to check that right now.
Also I think these available-properties cache entries are being created for product searches as well (each search term with its own cache key), which is probably a bad idea as it will eat up the cache space very quickly as well and the usefulness of that is very limited.

…w reads)

Spree::Filters::PropertyPresenter includes all ProductProperty for all Products, which can be very many.
This would be serialized and stored in the cache by Rails.cache.fetch.
As only uniq_values are actually relevant and being used, all the ProductProperty's should not be stored in cache.
The number of these properties can be very significant (in the 1000s), taking up a lot of cache and making
the cache read very slow (over 1 second with solid_cache).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant