Contrib.social

Performance issues due to multiple calls of the image_path_flush()

Open on Drupal.org →
Created on 25 February 2021, over 3 years ago
Updated 23 May 2024, about 1 month ago

Problem/Motivation

The Drupal\crop\Entity\Crop::postSave() calls the image_path_flush() which flushes all cached images (image styles instances). If the site has tens of image styles and tens of crop types it leads to performance issues.

At the moment the Drupal\crop\Entity\Crop::postSave() is called for each updated/created Crop entity instance - so during a new media file creation the image_path_flush() is called as many times as many crop styles are attached to the media file type.

For example, a site has 20 image styles + 5 crop types attached to a media file entity. So during a file creation the image_path_flush() is called 5 times, the file system is scanned ~100 times (5 * 20) to flush cached image styles.

Here's a screenshot from New Relic that reflects the described issue:

Steps to reproduce

  1. Install modules: crop, image_widget_crop, media.
  2. Add several crop types
  3. Add several image styles that are using the crop types
  4. On the "Manage form display" (admin/structure/media/manage/image/form-display) page select the "ImageWidget crop" widget add set several Crop Types
  5. Set a breakpoint at the Drupal\crop\Entity\Crop::postSave() in your IDE
  6. Add a new Media image and see how many times the image_path_flush() is called

Proposed resolution

  • Don't call an image styles flush during a saving of a new Crop entity. As I understand it doesn't make sense since there's no cached image styles at this moment.
  • Flush only image styles that are related to the current Crop type.

Remaining tasks

  1. Prepare a patch.

User interface changes

Nothing.

API changes

Nothing.

Data model changes

Nothing.

🐛 Bug report
Status

Needs review

Version

2.0

Component

Code

Created by

🇬🇪Georgia Chalk

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • Comment 4 months ago →
    🇺🇸United States generalredneck

    This is a simple and creative fix.

    The only situation where this "might" fail is removing the image_path_flush($this->uri->value); on instantiation. Looking through the commit history, there is no mention of why they did it on entity creation as well, but it may happen if a crop is added to a field where an existing file entity is stored. This may happen in the case where someone is using a module to update a file with Media Entity File Replace or IMCE → , or if the file entity is programmatically changed somehow (migration?). In these cases a particular style may already have a derivative, but I would guess it would be up to that module to clean them up? I'm grasping at straws honestly, but I want us to test those usecases.

    As it is, using with focal point and none of these situations, this works amazingly well and is a vast improvment performance wise, ESPECIALLY on S3 and Pantheon Filesystems.

  • Comment 4 months ago →
    🇺🇸United States generalredneck

    FYI I would have post a new relic stack trace, except that the improvements were so fast, new relic didn't keep any of the trace data. We went from 30+ seconds down to ~800ms with this fix on media creation with around 150 image styles and focal point installed.

  • Comment about 1 month ago →
    🇨🇴Colombia jedihe

    I can confirm this patch improves the time to update a media entity (incl. manual crops, with image_widget_crop) vs. not using the patch.

  • Comment about 1 month ago →
    🇨🇴Colombia jedihe

    I found that triggering the flush only for crop entity updates allows for an outdated image derivative to be served for this scenario:

    1. Site using Crop API + Image Widget Crop.
    2. An image style ImgSt configured to use only one effect (Manual Crop), using crop type CropA.
    3. The entity with the image field is saved, but no crop CropA is defined (widget set to not require it)
    4. The derivative for ImgSt is viewed/loaded (saves file)
    5. The entity is saved again, this time applying a crop for CropA. This creates a crop entity for CropA + this image.
    6. Loading the image derivative does not reflect the just-applied crop for CropA.
    7. Re-save the entity (make sure to do any kind of change to crop CropA). This updates the crop entity for CropA + this image.
    8. Loading the image derivative finally reflects the just-applied crop for CropA.

    This effectively means, requiring $update == TRUE forces the user/editor to save a new crop twice (first creates it, second updates it) in order to see already-generated derivatives properly updated.

    I'm attaching a patch that triggers the flush both for create and update scenarios of the crop entity. I just removed " && $update".

    Note: I tried to reproduce the above scenario in a simplytest.me site, but couldn't. Since I don't have a way to verify that simplytest.me actually applied patch #1 (I definitely added it to the build), I decided to trust what I see in the site I'm working on which is consistent with what I read in the code.

contrib.social Blog FAQ Discussions
Production build 0.69.0 2024