File sorting performance

[rasteiner]

Description

Sorting files in a pages section is unnecessarily slow: Flipping 2 images in a pages section showing 60 images takes over 20 seconds on my test server. During this time the panel is unresponsive.

Expected behavior
Sorting files in the panel should be an interactive experience.

Screenshots

To reproduce

1. Install plainkit
2. Add a files section to a page blueprint:

sections:
  files:
    layout: cards
    limit: 100

3. Add about 60 images, their size doesn't matter
4. Flip any 2 images and wait

Your setup

Kirby Version
4.0.1

Additional context
The waiting time is spent in two places which can be optimized.

1. Even when switching the place of 2 adjacent images, the "changeSort" commit action is applied to every image in the collection, while it would be enough to only change the two affected files. Adding a check like

   		if($this->sort()->isNotEmpty() && $this->sort()->toInt() === $sort) {
   			return $this;
   		}
   

   to changeSort() in FileActions.php in the best case reduces the 20 seconds to 10.2 seconds (of course putting, for example, the last image to the start still affects all of the images). The slow part in commit() is the creation of $old = $this->hardcopy(), in there it's the applying of the blueprints method that takes almost all time (e.g. 186ms out of 187ms).
2. The other half is spent in producing a response for the API call which is then promptly discarded by the frontend. The easiest would be to just return a true from the API route, this reduces the 10.2s to 0.2s. However, I've dug a bit deeper and the problem in the File API model seems to be the options property, which makes up 99% of the time spent: in particular it's the canChangeTemplate permission which is slow.

Besides the workarounds listed here above, both issues seem to point to the blueprints method (one via hardcopy, the other via canChangeTemplate). That also explains why the time is equally split between the two paths.

I don't really understand everything that happens in File::blueprints(), but I wonder if the result couldn't be cached "by parent", rather than as instance variable for a single File. This expects a parent to always have the same blueprint during a request, and therefore also the same accepted file blueprints. I think this is a safe assumption?

Here's an example implementation, just for the tests (I know the code isn't very Kirby like, but it shows the speedup from 20 seconds to 0.2 seconds): (my additions are marked with a "ADDED THIS" comment)

	public function blueprints(string $inSection = null): array
	{
		if ($inSection === null && $this->blueprints !== null) {
			return $this->blueprints; // @codeCoverageIgnore
		}

		// always include the current template as option
		$template  = $this->template() ?? 'default';
		$templates = [$template];
		$parent    = $this->parent();

		// ADDED THIS:
		static $cache = new \WeakMap();
		if($inSection === null && $cache->offsetExists($parent)) {
			return $cache->offsetGet($parent);
		}

		// what file templates/blueprints should be considered is
		// defined bythe parent's blueprint: which templates it allows
		// in files sections as well as files fields
		$blueprint = $parent->blueprint();

		$fromFields = function ($fields) use (&$fromFields, $parent) {
			$templates = [];

			foreach ($fields as $field) {
				// files or textare field
				if (
					$field['type'] === 'files' ||
					$field['type'] === 'textarea'
				) {
					$uploads = $field['uploads'] ?? null;

					// only if the `uploads` parent is the actual parent
					if ($target = $uploads['parent'] ?? null) {
						if ($parent->id() !== $target) {
							continue;
						}
					}

					$templates[] = $uploads['template'] ?? 'default';
					continue;
				}

				// structure field
				if ($field['type'] === 'structure') {
					$fields    = $fromFields($field['fields']);
					$templates = array_merge($templates, $fields);
					continue;
				}
			}

			return $templates;
		};

		// collect all allowed templates…
		foreach ($blueprint->sections() as $section) {
			// if collecting for a specific section, skip all others
			if ($inSection !== null && $section->name() !== $inSection) {
				continue;
			}

			//  …from files sections
			if ($section->type() === 'files') {
				$templates[] = $section->template() ?? 'default';
				continue;
			}

			//  …from fields
			if ($section->type() === 'fields') {
				$fields    = $fromFields($section->fields());
				$templates = array_merge($templates, $fields);
			}
		}

		// make sure every template is only included once
		$templates = array_unique(array_filter($templates));

		// load the blueprint details for each collected template name
		$blueprints = [];

		foreach ($templates as $template) {
			// default template doesn't need to exist as file
			// to be included in the list
			if ($template === 'default') {
				$blueprints[$template] = [
					'name'  => 'default',
					'title' => '– (default)',
				];
				continue;
			}

			if ($blueprint = FileBlueprint::factory('files/' . $template, null, $this)) {
				try {
					// ensure that file matches `accept` option,
					// if not remove template from available list
					$this->match($blueprint->accept());

					$blueprints[$template] = [
						'name'  => $name = Str::after($blueprint->name(), '/'),
						'title' => $blueprint->title() . ' (' . $name . ')',
					];
				} catch (Exception) {
					// skip when `accept` doesn't match
				}
			}
		}

		$blueprints = array_values($blueprints);

		// sort blueprints alphabetically while
		// making sure the default blueprint is on top of list
		usort($blueprints, fn ($a, $b) => match (true) {
			$a['name'] === 'default' => -1,
			$b['name'] === 'default' => 1,
			default => strnatcmp($a['title'], $b['title'])
		});

		// no caching for when collecting for specific section
		if ($inSection !== null) {
			return $blueprints; // @codeCoverageIgnore
		}

		// ADDED THIS:
		$cache->offsetSet($parent, $blueprints);

		return $this->blueprints = $blueprints;
	}

[distantnative]

@rasteiner Thanks for looking into this so closely and coming up with suggestions. I think those lot of sense. Looking closer into whether overlooking any edge cases with the caching by file parent.

[rasteiner]

I'm back from the holidays, thanks for looking into this! I've added a comment to the code to clarify what I meant with "not very Kirby like".