-
Notifications
You must be signed in to change notification settings - Fork 25
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
feat: implement CacheItemsTrait for better performance handling #70
Conversation
Note for anyone testing, there are several YAML changes. The intention was this release we'll make a new major version rather than a minor. |
Will test this in the morning but at first glance, using a new Asset Store looks like it will have the biggest performance gains and essentially is what I have implemented custom already on most of my sites. Also for sites that have a ton of files or a lot of users, this issue is causing some problems as well, mainly in the asset admin or dialogs when viewing an individual images details. silverstripe/silverstripe-framework#11272 Last thing I noticed in some of the other silverstripe repos are pull requests to not use self:: for calling a class' static method. silverstripe/silverstripe-cms#2955 |
Testing currently on one of our larger sites, with over 20k users, 17k files, 22k pages. Overall seems to be working great. Initial load in asset admin still a bit slow but don't know if there will ever be anything that can be done for that when trying to load metadata for 25+ files at once. Will monitor for the next day or so and let you know if I see any issues |
One thing that I am not 100% sure if this is configurable or not is if there is a way to disable the flushable portion of this cache. Main reason why is I am using Redis to store the cache, but hosting on heroku - when they restart the server daily or we deploy a code change, this cache is then flushed even though we don't need it to. @wilr - do you know if there is a way to disable that. I think that would be a huge help for people on hosting platforms like heroku |
Figured out my issue with the PublicCDN adapter, was due to other configuration changes on adding cache headers to the public filesystem. If you are good with the configuration option on the flush then this should be good to go. Will continue to monitor through out the day. |
So we tested this out on another site as well yesterday afternoon that has about 10k image assets. All has been working fine and as expected. Would say this is good to go. |
Merged now @obj63mc I'm happy it's a good initial step forward without making any major philosophical changes. Further work could be (I'm happy to hear people's thoughts as a trade off between 'correct' and 'fast')
Given that it probably doesn't change over time it could be saved to the db. That would cut down 3 queries per file on a cold cache. |
As reported by #67, #48 and others the performance of this module on flysystem 3 is bad, very bad. This is my attempt at overhauling the performance of the module to get better results out of the box.
In the original upgrade we picked up
jgivoni/flysystem-cache-adapter
thinking it would go a job but actually caused more headaches - turned out easier to implement something tailored to our needs.The new CacheItems integrations natively with the existing CacheInterface within Silverstripe so works out of the box but can also be used with memcached, redis etc.
Performance wise, warm cache it makes 0 calls to AWS. On a cold cache it makes 3N calls for published files (doesObjectExistV2, readObject, fetchFileMetadata).