[4.x] Improve collection Stache watcher performance #9302
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
JohnathonKoster
changed the title
|
I'm also testing on the Cool Writings /articles listing. It paginates to the first 10 items. With the Stache watcher disabled there's no difference - which is expected. I think the drastic difference is due to you using Windows. (I'm sorry!) Still, small code change results in a not-nothing performance increase. 👍 |
jasonvarga
approved these changes
Jan 30, 2024
|
This PR broke the custom stache store in my Advanced SEO addon. I get unlimited duplicate ID's. Regenerating just adds another duplicate. Do I have to change something on my side or is this a bug or breaking change? 
|
|
I'll revert it. Shouldn't have been a breaking change. Sorry about that. |
jasonvarga
added a commit
that referenced
this pull request
Feb 1, 2024
This reverts commit a18568a.
|
Weirdly enough, this PR completely crashes my staging site but works locally. At least, I think this error is connected to this PR. Downgrading Statamic to v4.46 fixes the issue. Error: Stack Trace: |
|
Would you be able to manually apply the revert on your staging site just to see if that causes the error to go away? You can probably use a composer patch. |
|
Yeah, I tried that already. The issue didn't go away. I wasn't sure if the composer patch didn't apply correctly or what else was going on. Because I can't reproduce this locally. So it makes it a little hard to debug. |
|
Correcting myself. I re-deployed with the composer patch and the site works now. So it's for sure this PR that causes the error. Just weird that it doesn't cause the site to go havoc locally to this extend. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR optimizes the Stache watcher by limiting the scope of the Collection Stache watcher to just the immediate files within the
content/collectionsdirectory. The changes allow for any store to disable recursive file retrieval (currently only enabled on the collection store).From testing, the existing behavior will load all file paths within the
content/directory when checking for Collection changes, and then load them all again for each individual collection that is checked.The following #'s are not super rigorous, and are just general observations from viewing a single article page in the browser (site based on Cool Writings). All numbers are after warming the Stache separately before running the site.
Baseline numbers without any extra collection entries: bounce between 750ms to 1.3s
These numbers remain fairly consistent event when increasing the number of file based entries to 20K+. Site is running on Windows via. WSL without any environment optimizations and using
php artisan serve.Note 1: While working through this, I had the thought that it would be possible to disable the Stache watcher for certain collections, and can look into that if there is interest (i.e., disable it on very large collections with infrequent updates, but keep it enabled for smaller ones, etc.)
Note 2: The back/forth changes related to removing the string operations inside the collection store were to preserve any existing behavior that relies purely on the store's
getFilterItemmethod. Removing these would improve things very, very slightly, but would definitely break behavior at this time (hooray existing test coverage).