Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem
During a load-test on a client site we ran into a performance problem: When uploading a file into the media library, the consequent request became very slow under high load (up to ~25seconds).
Steps to reproduce
- When uploading a file via the media library, the file is moved into temp store
- The media library upload form uses the FileRepository::move() method to move the file to the final, permanent location
- filerepository::move() invokes hook_file_move, resulting in image_move hook flushing the image style for the source image
- Image style flushing clears the theme registry and invalidates cache-tags of the complete image style:
// Clear caches so that formatters may be added for this style. \Drupal::service('theme.registry')->reset(); Cache::invalidateTags($this ->getCacheTagsToInvalidate());
The problem does not appear when using a regular file upload, e.g. via file widget, without the media-library. This is because the regular file upload does not use FileRepository::move() but directly uses FileSystem::move()
Possilbe resolution
One of:
- Convert media library to not use FileRepository::move()
- Improve image_move() to not flush an image style if source is temporary - can there be image styles for temporary files?
- Improve ImageStyle::flush() to not clear the theme registry if a path is given
The problem seems to be caused by #2833129: hook_image_style_flush doesn't get the $path passed to Drupal\image\Entity\ImageStyle::flush() method
| Comment | File | Size | Author |
|---|
Issue fork drupal-3439981
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #2
fagoTraced it down to be caused by #2833129: hook_image_style_flush doesn't get the $path passed to Drupal\image\Entity\ImageStyle::flush() method - before this change it was returning earlier in the method, thus never clearing theme registry when $path is set
Comment #3
fagoAfter some more research I found out the change was introduced by #2833129: hook_image_style_flush doesn't get the $path passed to Drupal\image\Entity\ImageStyle::flush() method in Drupal 10.2 - a new optional $path parameter was added to image style flush hook. While doing so the flush() method was re-factory wrongly such that the theme registry + cache tags are cleared also when $path is given.
So both should be addressed such that no cache or theme registry is flushed when $path is given. We've reproduced that doing so and addressing the related bug #3439990: With 10.2 image styles might get wrongly flushed solves the performance problem. It brings those slow file upload requests down from ~25seconds to normal <2seconds.
Related change record: https://www.drupal.org/node/3373248
Comment #4
petar_basic CreditAttribution: petar_basic at drunomics commentedHere is a patch to prevent flushing theme registry + cache tags if the path is provided.
Comment #5
fagoComment #6
fagoComment #8
fagoConverted the patch into a MR so the pipeline runs and slightly improved the comment.
Comment #9
needs-review-queue-bot CreditAttribution: needs-review-queue-bot as a volunteer commentedThe Needs Review Queue Bot tested this issue.
While you are making the above changes, we recommend that you convert this patch to a merge request. Merge requests are preferred over patches. Be sure to hide the old patch files as well. (Converting an issue to a merge request without other contributions to the issue will not receive credit.)
Comment #10
acbramley CreditAttribution: acbramley at PreviousNext commentedThis should be rebased against 11.x first, also we don't need to include usernames in the MR titles. Hiding the patch.
And of course, we need tests :)
Comment #11
fagoNot sure what a test would do here? Maybe a unit test making sure the theme-registry service is not invoked when we call $imageStyle->flush($path);
Comment #12
acbramley CreditAttribution: acbramley at PreviousNext commented@fago yeah having a look at what the theme registry reset does, I guess you could check that those specific cids still exist in cache. Ideally we'd have a test cache tag invalidator that could record the tags invalidated and we could assert things weren't invalidated but I can't see anything like that in core atm.
Comment #16
ericgsmith CreditAttribution: ericgsmith at Catalyst IT commentedAdded a new test method to the existing Image Style unit test to check that the registry and cache invalidation is not called if flush is called with a path. Checking the number of invocations seemed simpler than trying to dig through cache ids.
Hid the original MR after creating a new branch off 11.x - I see now according to https://www.drupal.org/docs/develop/git/using-gitlab-to-contribute-to-dr... I should have pinged you to update it the MR - sorry about that, no open discussions on the original so hopefully all good.
Comment #17
ericgsmith CreditAttribution: ericgsmith at Catalyst IT commentedComment #18
RoSk0Looks good to me, thanks!
Comment #19
catchIt's possible to assert the number of calls to cache tag invalidation in performance tests - would that be enough to produce a test that fails without the fix?
Comment #20
alexpottI think the test coverage we have here is good enough for a pretty important bug fix. Sorry about committing the issue that caused the regression.
Committed and pushed 7b24d54842 to 11.x and 7546eb2a72 to 10.3.x and 02b8bc5462 to 10.2.x. Thanks!