Allow filesystem to eager load select paths #3630
Conversation
Update filemanager to use this functionality instead of attempting to eager load the entire contents. The old behavior was problematic with remote filesystems containing many files. Related to #2564
| if ($file->UploadDestination->adapter != 'local' && $file->UploadDestination->exists()) { | ||
| if(!array_key_exists($file->upload_location_id, $carry)) { |
There was a problem hiding this comment.
Would it be better to swap these lines? I think most of the times the files would be sharing upload destination, so checking whether we already have the info before going into UploadDestination details should be more effective. Also might be worth checking if UploadDestination is not null - for edge cases when the folder record has been removed, but the file record stays - just so it wouldn't bomp out
There was a problem hiding this comment.
I think this is fine the way it is, that line 298 is just the old line 306 (https://github.com/ExpressionEngine/ExpressionEngine/pull/3630/files/6135f94929ff29b3d01e8a8b44d200d8a65e8b75#diff-cd5b889f5e6fcff6954cdafc0347f4ecdc532ad6e03ce5c5d115ce956fc32644L306) and the UploadDestination relationship gets eager loaded so the performance impact should be trivial. We would also have to do this same check later when $carry hasn't been set yet and I think this way is a little more readable.
| $paths[] = $file->getAbsoluteManipulationPath('thumbs'); | ||
| } | ||
|
|
||
| $uploadDestination->getFilesystem()->eagerLoadPaths($paths); |
There was a problem hiding this comment.
do we need to check if eagerLoadPath is available method?
There was a problem hiding this comment.
The filesystem has an implementation for eagerLoadPaths() so we don't need to check here, but inside that function it checks to see whether the current adapter has an implementation before calling it
|
Good catch Yuri, this latest push should resolve that issue |
There was a problem hiding this comment.
@bryannielsen on my test, I has S3 directory with 5 images and a subfolder. Without your changes, the load time is ~3 sec. With your changes, it's 5-6 sec. Tested multiple times, numbers staid the same. So IDK if it really makes any good, at least without hundreds of files in the directory (which is rather edge case).
I don't think we should make it better for 5% of people and worse for everyone else.
Also - unrelated - your code is missing some spaces (see PSR-12 notes on Files tab)
|
@intoeetive did you upgrade to the latest Cloud Files? The performance should be better and more consistent in 1.1.0+ whether you have 5 or 5,000 files in a bucket. Without this change and the updated version of Cloud Files we can have control panel timeouts on the file listing page. The experience may be slightly degraded with older versions of Cloud Files that aren't setup to run parallel requests. I think I can make a change to eagerLoadPaths() so that if an adapter does not support this we do the old eager load behavior of listing the entire directory. Which will preserve better load times for smaller buckets but still break things on large buckets. |
|
@bryannielsen I updated to CP 1.1 but the picture did not change. I then tried uploading more files (~80) and the load time increased for current 7.dev (4 sec) but still remains 5-6 sec when using this branch. |
|
My tests were consistent with Yuri's. I have 13 images in an S3 bucket. (Cloud Files 1.1.0).
|
|
Interesting, I'll add mine here too for comparison 28 files in S3 @robinsowell maybe we can bother you for another data point |
|
My results don't show a lot of difference between the two, but the older version is a touch faster on average. Mostly focused on my largest directory- around 200 image files. 'Finish' old version ~ 1.8 New was faster a few times, tried in FF and FF incognito. So.... IDK. |
|
Just pondering, I keep browser cache disabled when testing. I wonder if that messes with the results. |
|
@bryannielsen totally unrelated to testing this, I put a channel form on the frontend to test some wygwam thing and I'm getting a php memory error- my memory set to 128. |
|
Circling back, it looks like it's only happening with my wygwam field and only in the channel entry form. And it happens in the currently released v7 as well. SO nothing to do with this pr. Looks like a wygwam issue. |
Eager load an entire destination if using large pages or only one destination otherwise load specific files. Improve performance of remote filesystem sync by moving mimetype filtering from initial dispatch to individual sync jobs and use eager loading in job.
|
@bryannielsen there's now a code conflict |
Update filemanager to use this functionality instead of attempting to eager load the entire contents. The old behavior was problematic with remote filesystems containing many files. Related to #2564
This change allows adapters to implement functionality in an
eagerLoadPaths($paths = [])method when applicable.CachedAdapter contains a lot of duplicate code from Flysystem's
Cached\CachedAdapterwhich it extends because the parent's private properties do not lend themselves to a simpler subclass.