DBFile::getIsImage() is slow

[lozcalver]

Module version(s) affected

6.1

Description

DBFile::getIsImage() is called in a number of different places when outputting images, but it is surprisingly slow and hampers performance on image-heavy pages. Given that real world servers often have restricted IOPS, I think this is worth looking at.

I’ve been benchmarking this by removing the contents of the main Page.ss to minimise any other influences, and replacing it with this:

<% loop $List('SilverStripe\Assets\Image').Limit(50) %>
	{$ScaleWidth('500')}<br />
<% end_loop %>

Loaded once to make sure all resamples have been completed.

Base benchmark:

After replacing the contents of DBFile::getIsImage() with return true (not a proposed solution!):

How to reproduce

<% loop $List('SilverStripe\Assets\Image').Limit(50) %>
	{$ScaleWidth('500')}<br />
<% end_loop %>

Benchmark with XHProf

Possible Solution

I’m not sure yet... I don’t think we should rely on just the file extension, as users may not have the silverstripe/mimevalidator module installed, and assets could come from other sources anyway.

Heading further down the rabbit hole, Flysystem checks whether the file exists before eventually calling this method: https://github.com/thephpleague/mime-type-detection/blob/06fd1559cc50ad3300f5ce5c16ad9a53d52ba989/src/FinfoMimeTypeDetector.php#L74-L77. Apparently that internally then looks at chunks of the file data to figure it out, which might explain the slowdown

We could store the mime type in the database, but that would need to be in a major version I think.

We could cache the result of the mime type check, but my concerns would be: (A) how big might that cache end up being? and (B) presumably we’d need to flush it whenever the file hash changed (or at the very least include the file hash in the key)?

Perhaps this isn’t solvable without significant effort, just wanted to raise it.

Additional Context

No response

Validations

• Check that there isn't already an issue that reports the same bug
• Double check that your reproduction steps work in a fresh installation of silverstripe/installer (with any code examples you've provided)

PRs

• ENH Cache mime type info to reduce memory usage #702

[GuySartorelli]

We could store the mime type in the database, but that would need to be in a major version I think.

Why do you think that would need to be in a major version? That implies this would break some functionality or APIs, but off the top of my head I can't see how? Wouldn't this be the same as the cache suggestion, except we're caching it in the database instead of in the caching service?

[lozcalver]

Yeah that’s a fair point. I think my main concern if we made that change is whether we can be confident that people will be using those specific APIs when they interact with assets. E.g. someone uploads an image, we write the mime type to the database, then they replace it with a PDF and manually update the FileFilename column or something that bypasses whatever logic we put in place to update the mime type column. Obviously we can’t help if people are doing raw SQL queries, but I’m not sure how much API surface we’d need to cover to be “safe” with that if that makes sense?

Maybe I’m just being too cautious...

[GuySartorelli]

E.g. someone uploads an image, we write the mime type to the database, then they replace it with a PDF and manually update the FileFilename column or something that bypasses whatever logic we put in place to update the mime type column

Yeah, that's valid. Though I think the same risk exists with any other persistent cache. e.g. we put a hook in onBeforeWrite() and someone manually updates the file in the filesystem and uses raw SQL to update the database.

Whether we're caching the mime type in the database or somewhere else, there's risk the cache will get stale. I'd say if we're worried about that risk for any type of cache, then we just shouldn't cache it at all outside of a major release - either that or add it as a configuration option in a minor with the default set to false.

[lozcalver]

After trying a few things, I think the best solution to this would be a unified cache of file info. We currently have a cache for dimensions and storing whether manipulations have failed (e.g. due to OOM errors) on InterventionBackend. There are other things that could be cached too, for example FlysystemAssetStore::getMetaData(), which could then effectively enable caching mime type info if we add that to the returned metadata.

My vision for this would be a cache where a File record is the key, and metadata for the file (and all variants) is stored in the value. So a cache value for a single File record might look something like:

[
  'file' => [
    'dimensions' => [123, 456],
    'mimetype' => 'image/jpeg',
    'filesize' => 12345,
  ],
  'variants' => [
    'FitMaxWzYwLDYwXQ' => [
      'dimensions' => [12, 34],
      'mimetype' => 'image/jpeg',
      'filesize' => 123,
    ],
    'FitMaxWzYwLDYwXQ_ExtRewriteWyJwbmciLCJ3ZWJwIl0' => [
      'dimensions' => [12, 34],
      'mimetype' => 'image/webp',
      'filesize' => 123,
    ],
    'FitMaxXvYdLEYwPQ' => [
      'error' => [
        'reason' => 'unknown',
        'ttl' => 300,
      ],
    ],
]

The hard parts would be:

• Designing an API for this so we’re not juggling plain arrays across multiple classes
• Dealing with non-File resources – do we/how do we support this?