Many slow queries for lookup up crop of a image_style + file

I also saw with blackfire profiler profiling some jsonapi routes that for each image style that is exposed the following db call is executed:

4300 means your json api response includes 4300 image style links? I guess a lot of nodes multiplied by a lot of image styles, (response image styles?)

It's basically 10 crop types and 2 different resulting image sizes. So at the end 20 image styles.

I know it's a lot but this happens if you apply the patch from #2825812 and serialize a node with paragraphs which have media entities which you include.

I can probably reduce this to half the styles but the problem still exists.

I discovered what @yobottehg describes in #2, thanks to the Blackfire.io profile that @yobottehg reported in #3014232 — see #3014232-18: [regression] ResourceTypeRepository is significantly slower in JSON:API 2, becomes noticeable when handling hundreds of interlinked resources.3.

I'm surprised that the crop is applied during hook_file_url_alter(). Why not extend File entities to have a crop field, which would move the processing to occur earlier, and allow the crop to be cached along with the entity? That'd still preserve the non-destructive nature of the Crop module (which seems to be the overarching yet undocumented design goal?).

#8 makes me understand this module even less than I did before. 😵 I really want to, but reading crop.module, \Drupal\crop\Entity\Crop, \Drupal\crop\Entity\CropType, crop.api.php and its README really do not explain this module's architecture, how it slots into existing APIs, how it's meant to be used, and the rationale :(

Also seems like Crop::findCrop() would benefit from some static caching, since both crop's Crop::getCropFromImageStyle( and imageapi_optimize's ImageStyleWithPipeline::transformDimensions() are calling it, resulting in 4374 calls to it for "only" 1254 unique image style + file combinations:

EDIT: source: the Blackfire.io profile posted at #3014232-23: [regression] ResourceTypeRepository is significantly slower in JSON:API 2, becomes noticeable when handling hundreds of interlinked resources.

Possibly a separate static cache for getting the crop type from an image style and then getting the result of that crop type + file, if we don't have the first either yet?

Sounds sensible!

Here is the PoC static cache on the method under discussion. Dropped time about 30-35% withing the method. In my case 450ms -> 300ms.

I think an index on the data will help as well, but I am not sure where to write the schema alter code.
On top of that we should be able to break on the first crop found from a provider. No need to loop the whole effects list if a crop is found.

Applying a patch that does the following:
- Adding an index on type and uri columns in crop_field_data table. This will speed up the database queries.
- Adding a static cache where the crop data gets retrieved from the database. This function is called too often from \Drupal\crop\Entity\Crop::getCropFromImageStyle, because we could have a large number of image styles that share the same crop type.

There is the strange behavior of fetching multitude of crops in a loop and using the last one only. Seems like a logical issue in the original code. If we do an array_pop (when not empty) and fetch only that should spare DB queries in the long run in some cases.

Overall this patch is a huge step forward I am getting around half a second speed up.

The failing tests does not seem related in ANY way and should not block a commit.

I was testing for cold caches in #20, so all caches were disabled (including the entity one), so that might not be relevant that much...
I will test both patches. How to do the validation part? The tests should be enough.

Hey berdir sorry for that late reply ! I will see all of this today this is my main focus ! I will test this patch with iwc by the way and with automated crop too (probably a part of automated crop factory need similar things but the bridge with crop api are ready to use this important improvement)

I make more feedback fast !

@berdir we can think to stable release for crop api 2.x by the way I guess with that patch and other small improvements for media in Code what’s your feeling about that ?

makes me understand this module even less than I did before. 😵 I really want to, but reading crop.module, \Drupal\crop\Entity\Crop, \Drupal\crop\Entity\CropType, crop.api.php and its README really do not explain this module's architecture, how it slots into existing APIs, how it's meant to be used, and the rationale :(

YES *_*, Efforts on the crop module need to be continued and I am currently personally reviewing the priority for contribution and sponsorship. This module must have remained simple and do the job in the best way as Janez and I had planned at the base. The effort at the documentation level must be resolved as soon as possible and I commit myself to realize all that is necessary around this problematic having also the knowledge of certain technical choices at the time of the development of the module we had serious problems and drift with the $ type in the queries for example we'd delete it and decide to find a robust solution on issue XXX.

Sounds good to me. We're using it in a lot of projects and it's working fine for us beside the performance issues, which this will hopefully mostly address.

I'm going to create a ROADMAP issue and a precise cartography of what we need to add in module for stable and robust release (TU / TF included). I just need people to validate that roadmap are realist for our projects.

1. +++ b/src/CropStorage.php
   @@ -2,32 +2,52 @@
   +    if (isset($this->cropsByUri[$uri][$type])) {
   +      return $this->load($this->cropsByUri[$uri][$type]);
   +    }
   +    return NULL;
      }
   

   We can optimize by move this block first with negative test (;'( sorry oncle bob)

   like :

   // Case 1 : We already have cached crop (common case).
       $this->loadCachedCrops($uri, $type);
   
   // Case 2 : Cached crops not exist we get it and re-use loadCachedCrop because test + load are a generic test to load in case 1 and 2 that's same code.
       if (!array_key_exists($uri, $this->cropsByUri)) {
         $query = $this->database->select('crop_field_data', 'cfd');
         $query->fields('cfd', ['type', 'cid']);
         $query->condition('cfd.uri', $uri);
   
         $query->range(0, 1);
   
         $this->cropsByUri[$uri] = $query->execute()->fetchAllKeyed();
   // Case 3 : recovered and newly store query can now load correctly or if not return NULL is returned because impossible to recover CROP caused by internal reasons.
         $this->loadCachedCrops($uri, $type);
       }
   // Case 4 : unable to recover crop we return NULL.
       return NULL;
   

   I focus my effort from readibility and clean codes principles to avoid #9 kind of comment and that's right in the past we was too lax :P

2. +++ b/src/Entity/Crop.php
   @@ -142,22 +151,20 @@ class Crop extends ContentEntityBase implements CropInterface {
   +    if (isset($effects['crop_crop']['type'])) {
   

   isApplicable again ? not empty and 'image' !== $provider (Prepare us to win war counter empire :))

3. +++ b/src/Entity/Crop.php
   @@ -165,13 +172,48 @@ class Crop extends ContentEntityBase implements CropInterface {
   +        if ($provider != 'image') {
   

   Why not adding a static property $notApplicableProvider = ['image']

   and then having a test like if($this->isApplicable($provider) or isApplicable() is provider are in property (not sure). Or test provider in findCrop method (to avoid this test in foreach :/).

4. +++ b/src/Entity/Crop.php
   @@ -165,13 +172,48 @@ class Crop extends ContentEntityBase implements CropInterface {
   +    if (!isset(static::$effectsByImageStyle[$image_style_id])) {
   

   Can we extract this test like : if(this->hasExistingEffects($image_style_id)) { ... }

   That's useful for reability

5. +++ b/src/Entity/Crop.php
   @@ -165,13 +172,48 @@ class Crop extends ContentEntityBase implements CropInterface {
   +        if ($effect->getPluginId() == 'crop_crop') {
   

   Can we add a const into CropEffect to centralize effect name and avoid eventual errors (That change can make easier use of Crop api).

6. +++ b/src/Entity/Crop.php
   @@ -165,13 +172,48 @@ class Crop extends ContentEntityBase implements CropInterface {
   +          $effects[$effect->getPluginId()]['type'] = $effect->getConfiguration()['data']['crop_type'];
   +        }
   +      }
   +      static::$effectsByImageStyle[$image_style_id] = $effects;
   

   Again extract this into separate method look better EG : $this->addCropEffects(array $effects): self

   Possibilbly we can wrap all foreach in extracted method if we can be more clean and clear.

   That have another important improvement that's make Crop object more calistenic and simple ;)

I'm currently in test with IWC.

Hey, @berdir,

I've ran some tests with the latest (#25) patch and I will share some feedback with you.
So, for me, the current state of the patch is definitely not production ready and still needs some work, but as far as optimizations goes - yes, there is definitely an improvement.

So, what was my use case:

- I have installed the module, along side JsonAPI, because I need the cropped images to be accessible directly with given URL.
- I have added 5 crop types with the following Aspect Ratios (not that they matter, but still...) - 1:2, 2:1, 2:2, 2:3 and one without AR /Free crop/
- I have created an Image Style for each Crop Type and applied the cropping effect accordingly.
- I have created a basic node and for the image/media uploaded and then applied cropping to the image for all of the types.
- Lastly, I've accessed my API endpoint and I've successfully obtained all 5 URL's with each crop style applied /so far so good/.

For my surprise, I saw that the cropping was applied only for the first Crop Type loaded /in my case this being 1:2 because they are sorted by name/. For the rest of the image styles, as a result I received the original image with no image style applied.

So, this made me curious and after looking at code changes introduced by the last patch, I saw that the problem lies within CropStorage. Basically, what happens is that by storing the $uri in static cache and with a combination of range(0, 1) you are no longer able to retrieve the crop configuration for any given $type but the first one, while the rest will always fallback to NULL.

For the moment I am not able to provide new patch, but meanwhile I can test pretty fast with a real-case scenario any future patches, until I find time to contribute myself.

Yes, I've tested it before posting the previous comment, so I can confirm that removing the range() will definitely yield correct result.
I just wasn't sure this is the direction that you will be going, since I am not that familiar with the discussion for now.

Also, I guess it is okay to point out the obvious - even with a static cache added, removing range() will still load all of the crop types, which might not be the best idea when, let's say, you have single image rendered on your page with crop style applied and in this case you're interested in loading just a single Crop Type.

But besides that point, there is quite the performance-wise improvement already, so cases like the last one can be further optimized.
Good work.

Applying a new patch that is removing the range().

IMHO we can’t just remove range like this directly without fixed the contextualization of crops variants. If users make n crops for one crop type findCrop will return n crop object without another logic to choose which crop is needed by display.

Based on the existing comments it looks like #36 is robust enough to make it into 2.0?

If I got right from #38, while not the perfect solution, a non-ideal solution is far better than no solution at all. Without this patch, the crop api is not something that can run on a production site with a medium amount of uncached content. Could it be included in the next RC? Further feedback can be used later on to improve things.

IMHO the clean code tweaks won't change anything in the overall solution, and can always be added later on if truly needed.

I can confirm that the patch #36 works pretty well. No side-effect until now.

Based on all the above since #36 and the fact that I've been using the patch from #36 for months now on a high traffic information portal solution with no side effects, I will be moving to RTBC.

Let's tie up a few loose ends before commit:

1. +++ b/crop.module
   @@ -177,12 +177,7 @@ function crop_file_url_alter(&$uri) {
   -    if (!$image_style = ImageStyle::load($image_style)) {
   

   crop.module now has an unused use statement for use Drupal\image\Entity\ImageStyle;

2. +++ b/src/CropStorage.php
   @@ -2,32 +2,49 @@
   +   * Statically cached crops, eyed by uri first and type second.
   

   s/eyed/keyed

3. +++ b/src/CropStorageSchema.php
   @@ -0,0 +1,29 @@
   +use Drupal\Core\Field\FieldStorageDefinitionInterface;
   

   Unused use statement

4. +++ b/src/Entity/Crop.php
   @@ -142,22 +151,20 @@ class Crop extends ContentEntityBase implements CropInterface {
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public static function getCropFromImageStyleId($uri, $image_style_id) {
   

   This method has no parent to inheritdoc from

5. +++ b/src/Entity/Crop.php
   @@ -165,13 +172,48 @@ class Crop extends ContentEntityBase implements CropInterface {
   +   * @return array
   +   *   A list of effects, keyed by plugin ID, each effect has a uuid, provider
   +   *   and in case of crop_crop, the type key.
   

   @return array[]

Fixed above comments and did some more boy scouting on the patch.

#44, this one have some issues with focal point module 8.x-1.0 on drupal 8.7.7. It won't auto generate images before you manually click preview images link (under image). It should be auto generating images on node save. @pdenooijer what was your use case though ?

Best,
v

No, my use case was that the front page was so terribly slow. This was because of the shear amount of slow queries (no index) on the crop table. I did not test this with focal point.

#47 i've figured out. i had to use convert_encoding('UTF-8', 'HTML-ENTITIES') in the views template. In version 8.6.16 i was using fields.field_image.content|render|striptags|trim and now i've switched to
fields.field_image.content|render|striptags|trim|convert_encoding('UTF-8', 'HTML-ENTITIES')

and i wasn't using s3.

Patch in #44 applies against the latest dev version and it is still resolving the problem.
RTBC +1.

Can we please merge this, as it has 2 RTBC now an we are using this with success in production for quite a while as well.

1. +++ b/src/CropInterface.php
   @@ -108,7 +108,22 @@ interface CropInterface extends ContentEntityInterface {
   +   *
   +   * @deprecated use getCropFromImageStyleId instead.
       */
      public static function getCropFromImageStyle($uri, ImageStyleInterface $image_style);
    
   +  /**
   +   * Retrieve crop from given image style.
   +   *
   +   * @param string $uri
   +   *   URI of the image.
   +   * @param string $image_style_id
   +   *   The image style id.
   +   *
   +   * @return \Drupal\crop\CropInterface|null
   +   *   Crop entity used by effect 'crop_crop' or NULL if crop doesn't exist.
   +   */
   +  public static function getCropFromImageStyleId($uri, $image_style_id);
   

   Is there any real reason to add a whole method here? Surely we could just change getCropFromImageStyle to accept either an ImageStyleInterface object, or an ID. Then there's no real need for calling code to deal with this deprecation.

2. +++ b/src/CropStorage.php
   @@ -14,7 +14,7 @@ use Drupal\Core\Entity\Sql\SqlContentEntityStorage;
   +   * Statically cached crops, keyed by uri first and type second.
   

   Nit: Seeing as how we're fixing minor typos here anyway, 'uri' should be 'URI', since it is an acronym.

3. +++ b/src/CropStorage.php
   @@ -34,7 +34,7 @@ class CropStorage extends SqlContentEntityStorage implements CropStorageInterfac
   -    if (!array_key_exists($uri, $this->cropsByUri)) {
   +    if (!isset($this->cropsByUri[$uri])) {
   

   This a subtle, but important-looking, change in behavior. Won't this cause a performance regression, since it will be querying for a crop even if it has previously come back as NULL? Either way, this deserves a comment explaining the rationale, so that a future developer doesn't unwittingly change it back.

4. +++ b/src/Entity/Crop.php
   @@ -159,8 +159,7 @@ class Crop extends ContentEntityBase implements CropInterface {
   -    $crop = FALSE;
   -
   +    $crop = NULL;
   

   What is the rationale for this?

1. +++ b/crop.install
   @@ -68,3 +69,16 @@ function crop_update_8003() {
   +
   +/**
   + * Add index on type and uri.
   + */
   +function crop_update_8005() {
   +  $manager = \Drupal::entityDefinitionUpdateManager();
   +  // Get the current crop entity type definition, ensure the storage schema
   +  // class is set.
   +  $entity_type = $manager->getEntityType('crop')
   +    ->setHandlerClass('storage_schema', CropStorageSchema::class);
   +  // Regenerate entity type indexes.
   +  $manager->updateEntityType($entity_type);
   +}
   

   This absolutely needs a dedicated test.

2. +++ b/src/CropStorage.php
   @@ -2,32 +2,49 @@
   +  /**
   +   * Statically cached crops, keyed by uri first and type second.
   +   *
   +   * @var int[][]
   +   */
   +  protected $cropsByUri = [];
   

   Perhaps this should be using a dedicated implementation of CacheBackendInterface (i.e., MemoryBackend)?

#53.2 For any long-running script that will have to manage crops this could pose an issue.
If there is private cache it should have an API to clear it OR inject a service to contain the cached data.

I am generally +1 for #52.2.

I agree with Berdir.
#51:

1. It is not SOLID to change an interface to accept both an ID and an object, so that's why there is a new method.
2. Your welcome to create an other patch for this, but I don't think this is something to block the current issue.
3. I changed it to isset as there will never be an null value in the array. As this is an performance improvement patch, isset is slightly faster then array_key_exists.
4. The return value of the interface states that is should be \Drupal\crop\CropInterface|null, so returning false is a bug.

#52

1. Entity update is not code of this module, so why test it here? There is nothing special going on but just updating an table with an index.
2. I agree with Berdir, there is no benefit at the moment. There can always be an other iteration on the subject, let's first merge this to fix the performance issue.

Can we please merge this now?

The only thing I think I feel strongly about here is the update test. I agree it is straightforward and unlikely to break, but entity updates are a situation where if something goes wrong for any reason, regardless of whose fault it is, there is a significant risk of data loss. It therefore feels irresponsible to commit this without some sort of assurance that the update path does what it's supposed to do.

That said, I know how painful it is create a fixture for update testing. I've done it before, though, and I'll see if I can do it here too. It really would make me feel a lot better, and if something does go wrong with the update path in the future, it'll be a lifesaver to have a test of it.

I just committed #3104867: Add a dedicated test to ensure the update path works, so it should now be trivial to add an assertion to UpdatePathTest confirming that the index exists as expected. :)

Adjusting credit for commit.

Committed, then reverted due to incorrect credit, then re-committed and pushed to 8.x-2.x. Thanks!

Woop woop, nice :D!

Thanks so much for this!

We had 4x crops on a single huge image go from 55s to 8s on node save. There are many many image styles in this site for responsive images for many different contexts.