Problem/Motivation
Several problems with media calculations:
Each link to a file found tries to get the media field definition \Drupal\entity_mesh\Repository::getMediaFileByEntityFile
public function getMediaFileByEntityFile(FileInterface $file): ?MediaInterface {
$media_types_with_tile_fields = $this->getMediaFieldTypeWithFileFields();
if (empty($media_types_with_tile_fields)) {
return NULL;
}
...
protected function getMediaFieldTypeWithFileFields() {
$medias_type_reference_to_file = [];
$media_types = $this->entityTypeManager->getStorage('media_type')->loadMultiple();
foreach ($media_types as $media_type) {
$fields = $this->entityFieldManager->getFieldDefinitions('media', (string) $media_type->id());
foreach ($fields as $field) {
if (in_array($field->getType(), ['file', 'image'])) {
$medias_type_reference_to_file[] = [
'field_name' => $field->getName(),
'field_type' => $media_type->id(),
];
}
}
}
return $medias_type_reference_to_file;
}
The problem is that it loads all media types and definitions each time the method is called (once per link).
But the most problematic code is in the method itself \Drupal\entity_mesh\Repository::getMediaFileByEntityFile, as it introduces a condition on target_id, it can use an index
public function getMediaFileByEntityFile(FileInterface $file): ?MediaInterface {
$media_types_with_tile_fields = $this->getMediaFieldTypeWithFileFields();
if (empty($media_types_with_tile_fields)) {
return NULL;
}
$media_query = $this->entityTypeManager->getStorage('media')->getQuery();
$media_query->accessCheck(FALSE);
$or_condition = $media_query->orConditionGroup();
foreach ($media_types_with_tile_fields as $media_type_file_field) {
$or_condition->condition($media_type_file_field['field_name'] . '.target_id', $file->id());
}
$media_query->condition($or_condition);
$results = $media_query->execute();
if (empty($results)) {
return NULL;
}
return $this->entityTypeManager->getStorage('media')->load(reset($results));
}
Example of query generated:
SELECT base_table.mid
FROM media AS base_table
LEFT JOIN media__field_media_image ON ...
LEFT JOIN media__field_media_document ON ...
LEFT JOIN media__field_media_audio_file ON ...
LEFT JOIN media__field_media_video_file ON ...
-- ...one JOIN for each file field
WHERE media__field_media_image.field_media_image_target_id = :fid
OR media__field_media_document.field_media_document_target_id = :fid
OR media__field_media_audio_file..._target_id = :fid
OR ...
Steps to reproduce
Try to process nodes from the entity mesh processing batch with multiple media on the same content, it will be slow, and it will take several seconds for each node.
Proposed resolution
As the field map does not change, save it on a property to avoid recalculating it, it will be recalculated once for execution, not by link:
\Drupal\entity_mesh\Repository::getMediaFieldTypeWithFileFields
protected function getMediaFieldTypeWithFileFields() {
// The media field map does not change during a run; build it once.
if ($this->mediaFileFieldsCache !== NULL) {
return $this->mediaFileFieldsCache;
}
Do not recalculate files that were already found
public function getMediaFileByEntityFile(FileInterface $file): ?MediaInterface {
$fid = $file->id();
if (array_key_exists($fid, $this->fileToMediaCache)) {
return $this->fileToMediaCache[$fid] ?: NULL;
}
Do not recalculate as some bundles could share the same field name.
$media_storage = $this->entityTypeManager->getStorage('media');
$media = NULL;
$queried_fields = [];
foreach ($media_types_with_tile_fields as $media_type_file_field) {
$field_name = $media_type_file_field['field_name'];
// Several media bundles may share a field name; query each once.
if (isset($queried_fields[$field_name])) {
continue;
}
Instead of using a big query with multiple JOINS in the WHERE condition, just make smaller queries, filtering by target_id as the field is indexed, and if it is found, jump out of the foreach.
$results = $media_storage->getQuery()
->accessCheck(FALSE)
->condition($field_name . '.target_id', $fid)
->range(0, 1)
->execute();
if (!empty($results)) {
$media = $media_storage->load(reset($results));
break;
}
Saving it on the property to avoid recalculating it:
$this->fileToMediaCache[$fid] = $media ?: FALSE;
return $media ?: NULL;
}
Issue fork entity_mesh-3616244
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #2
eduardo morales albertiComment #3
eduardo morales albertiComment #5
eduardo morales albertiComment #6
eduardo morales albertiComment #7
eduardo morales albertiComment #8
lpeidro commentedHello Edu:
Reviewed all the changes in the MR, they look correct and will definitely improve performance.
Just one suggestion. Here in the file-to-media cache we are storing the full Media entity:
$this->fileToMediaCache[$fid] = $media ?: FALSE;I would store the mid instead of the entity. A hard reference here pins the entity even after $storage->resetCache(). Reloading the entity costs almost nothing: entity loads are memory-cached per request, so a load() on a cached mid won't rebuild the entity from scratch.
Thanks
Comment #9
eduardo morales alberti@lpeidro Thanks for the review. I don't think storing the mid instead of the entity would be an improvement here, and I think it would actually be slightly worse.
Currently, on a cache hit:
src/Repository.php
With the mid, since the interface must return ?MediaInterface, we have to load the entity again on every hit:
That load() goes through EntityStorageBase::load() -> loadMultiple() -> key building -> memory cache lookup. Cheap, but it is strictly more work than an array lookup.
The two parts of the argument also cancel each other out:
- If nobody called resetCache(), the media storage memory cache still holds the same object, so storing the mid frees nothing and we paid the extra load().
- If resetCache() did run, we do free the object, but then load() is not "almost nothing": it hits the database again (base + data + field tables) and rehydrates the entity.
There is no resetCache() call anywhere in the module, and entity_mesh.repository is a normal per-request service, so we are in the first case.
On array size specifically: an array slot holding an object is a zval with a pointer, the same footprint as a slot holding an int, so the array itself does not shrink either way.
There is a real optimization nearby, but it is a wider change. The only consumerneeds three scalars (src/EntityRender.php):
We load a full Media entity just to read those and discard it, on every cache miss. Both values are already in the field's dedicated table (which has a bundle column), so the entity load could be skipped entirely:
Example:
I would not do it in this MR, though: getMediaFileByEntityFile() is public API (RepositoryInterface.php:188) and returns ?MediaInterface, so it needs a new method plus a deprecated wrapper and test changes, it only covers configurable fields with dedicated tables, and it bypasses the Entity API. The gain is also secondary: the main bottleneck was the orConditionGroup query without a range plus rebuilding the field map on every call, which this MR already fixes. Skipping the entity load is minor on normal runs and only measurable with thousands of distinct files, so I would profile the queue worker before committing to it.
Comment generated with the help of IA
Comment #11
eduardo morales albertiFixed!