Page MenuHomePhabricator

Image metadata cache is needed to avoid multiple file operation
Closed, ResolvedPublic

Description

Author: alarm

Description:
Currently image metadata is retrieved every time image apears on the page, so
file operations (file_exists, getimagesize) are called too often.
Image metadata cache can hold this information and reuse it every time when needed.


Version: 1.4.x
Severity: enhancement

Details

Reference
bz1686

Event Timeline

bzimport raised the priority of this task from to Medium.Nov 21 2014, 8:16 PM
bzimport set Reference to bz1686.
bzimport added a subscriber: Unknown Object (MLST).

alarm wrote:

Changes in Image.php and SpecialUpload.php files

Changes in Image.php sets metadata to cache on first use an retrieves on next
uses.
Changes in SpecialUpload.php deletes metadata on image upload

attachment 1686-Image.php-SpecialUpload.php.txt ignored as obsolete

We'll want to move metadata into the image tables for 1.5, but
this might be a helpful hack for 1.4 for the meantime. A few
comments:

If you can post diffs in -u format that'd be helpful; these are more
legible (including context) and are easier to apply to modified
files.

Use tabs for indentation to keep consistency with existing code.

Memcached has a key length limit of 255 bytes; we have a title
limit of 255 bytes. A long image name thus may not fit; consider
using a hashed name in the key.

It might be best to explicitly support the shared image source
directory (commons); for instance by checking two memcached
keys (shared and local).

Does this interact correctly with generated thumbnails? What
about image deletions and reversions?

Remember to remove your test 'echo's before submitting patches.
:)

The [$i++] stuff is somewhat opaque; consider using a dictionary.
($cachedValues = array( 'name' => $this->name, ... )

alarm wrote:

A better fix

Removed echos (shame for me for leaving those :), tabified, implemented cache
invalidation on delete/undelete/revert, better support of commons

Attached:

alarm wrote:

(In reply to comment #2)

<..>
Does this interact correctly with generated thumbnails? What
about image deletions and reversions?
<..>

It does show thumbnails correctly, so I guess that it should not impact them
anyway (except that original image is not accessed for metadata retrieving).
However, there are few file_exists calls for thumbnails, but I'm not sure I know
enough about thumbnail lifetime to add some caching here.

This is committed to HEAD, it really should be backported to REL1_4. The
severity of this bug is "enhancement" but actually it's the major performance
problem in Wikipedia at the moment, apparently causing frequent timeouts.

zigger wrote:

(In reply to comment #5)
Set Version & Keywords accordingly.

alarm wrote:

Fix merged into 1.4 branch

Merger same fix (with Tim's adjustments) into 1.4 branch

Attached: