For some reason a blank drupal install uses this function on the icon 'misc/favicon.ico'. When using xdebug tracing I saw preg_match was called 346 times in this function. This is because this loop is done through an array with all known extensions:

  foreach ($mapping as $ext_preg => $mime_match) {
    if (preg_match('!\.(' . $ext_preg . ')$!i', $filename)) {
      return $mime_match;
    }
  }

The patch changes this to a lookup instead of a loop which is quite some faster:

  $last_dot = strrpos($filename, '.');
  if ($last_dot !== FALSE) {
    $ext = drupal_strtolower(drupal_substr($filename, $last_dot+1, drupal_strlen($filename)-$last_dot));
    if (isset($mapping[$ext])) {
      return $mapping[$ext];
    }
  }
CommentFileSizeAuthor
#7 473652-optimize-file-get-mimetype.patch42.11 KBDamien Tournoud
#3 mapping.patch1.69 KBcatch
file_get_mimetype_loop.patch661 bytesR.Muilwijk
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

The last submitted patch failed testing.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

I posted #473632: file_get_mimetype() is slow almost simultaneously which has the hack of moving that mimetype first in the loop (and benchmarks showing that it's faster).

If we can fix those fails this would be a committable solution.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Needs work » Needs review
FileSize
mapping.patch1.69 KB

OK so there's a few problems with this:

1. The mimetype mapping uses multipe extensions for array keys separated with pipes - so isset() won't work - hence those test faisl.

2. Finding the last dot in the filename won't work for "pcf.Z" extensions (and nor will strtolower).

So we have a couple of options:

1. run the isset first, then do the loop with preg_match() as fallback
2. Put commonly used mimetypes early in the array.

If we go for #1, the we should use http://uk3.php.net/manual/en/function.strrchr.php and drupal_substr() - a bit more concise that way.

Attaching my hack (option #2) from the other issue since that fixes the critical path performance issue even if it doesn't scale to other extensions.

Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented
Assigned: R.Muilwijk » Damien Tournoud

I'll work on something.

R.Muilwijk’s picture

R.Muilwijk CreditAttribution: R.Muilwijk commented

@catch, hmmz forgot to run the tests because simpletest didn't install.

@all, maybe we should also have a look at some cleaner solutions then using the extension. What about a solution like:

if (file_exists($filename)) { // and is not a url should be added
  if (function_exists('finfo_file')) {
    // get info with finfo http://nl2.php.net/manual/en/ref.fileinfo.php, php 5.3
  }
  else if (function_exists('mime_content_type')) {
    // get info with mime_content_type()
  }
}

// do our extension based stuff here

This would remove the need for using extension based mime types locally and fix the preg_match()'es though two file call's aren't fast either but at least a lot cleaner!

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Needs review » Needs work

R.Muilwijk - can you work up a patch for yours? Let's see how it works, and benchmark it.

Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented
Status: Needs work » Needs review
FileSize
473652-optimize-file-get-mimetype.patch42.11 KB

The attached patch uses a more efficient mapping strategy, and move the default mapping to a separate file. When the mapping is overridden, there is no point in loading the large default mapping.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Needs review » Reviewed & tested by the community

This saves us the 5ms on every page load.

It removes array keys like 'ps|ai|eps' => 'application/postscript', so we don't need to do string parsing on hundreds of items.

And it comes with a test.

RTBC.

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD.

drewish’s picture

drewish CreditAttribution: drewish commented

i can see the rationale behind this change but i gotta say this seems pretty hacky especially when the time comes to add new mimetypes like in: #370958: FLV Flash Video Mimetypes

edited to use the correct issue number

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

aaron’s picture

aaron CreditAttribution: aaron commented
Status: Closed (fixed) » Active

Re-opening per #531476: Add favicon mimetype theme setting, as we probably want to roll this one back once that goes in. I figure it's easier to use the original issue so we have the discussion going into this. If anyone objects, we can start a new issue instead.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

The theme setting patch is in. As long as this function stays outside the critical path, I think it'd be alright to put it back how it was.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Issue tags: -Performance

No longer a performance issue.

Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented
Assigned: Damien Tournoud » Unassigned

  • Dries committed cbd2226 on 8.3.x 
    - Patch #473652 by Damien Tournoud, catch: removed unneeded loop from...

  • Dries committed cbd2226 on 8.3.x 
    - Patch #473652 by Damien Tournoud, catch: removed unneeded loop from...

  • Dries committed cbd2226 on 8.4.x 
    - Patch #473652 by Damien Tournoud, catch: removed unneeded loop from...

  • Dries committed cbd2226 on 8.4.x 
    - Patch #473652 by Damien Tournoud, catch: removed unneeded loop from...

  • Dries committed cbd2226 on 9.1.x 
    - Patch #473652 by Damien Tournoud, catch: removed unneeded loop from...