Fix entity loading in SearchApiFieldTrait

Yes, since we can't know how an entity implements its link functionality, we have to load the whole entity for that case. If you want to avoid it, just disable "link to entity" and use "Rewrite results" instead (link + tokens).

For this use case in general, it would be great if the Views integration would respect the Solr index setting to "pull data from solr". This is a common need with advanced uses of Solr, and the lack of an easy way to enforce that in Views will create a lot of testing complexity.

We can't really know about such backend-specific options, but in general, avoiding entity loads wherever possible is of course the goal of this issue.
A general "do not load items" option, e.g., in the query settings might also make sense, though, and could be a big help for such use cases, you're right. Would be a separate issue, though. (And I've not thought this through yet, so there might be smaller or larger obstacles to adding that.)
For a custom-code solution in the meantime, you could try overriding the datasource plugin with one for which you can manually disable/circumvent item loading temporarily. Just an idea, though.

One problem the SearchApiFieldTrait::preRender() method definitely has is that it will call Index::getPropertyDefinitions() once for each result, each field and each encountered property while retrieving the field's values, which potentially multiplies to a staggering amount for a call to a non-trivial method, which each time will retrieve the properties from the entity type and all its bundles and then run them through the enabled processors that might change this. Didn't really spot that when implementing, but this method is actually not that cheap (even though all of the information retrieved is actually cached at a lower level already), and calling it this much can therefore alone cost quite a lot of time.

The (hopefully) simple solution here is, of course, to just cache those properties. There are three options for how/where to do that:

1. Caching just within the method (so, still one call per field and datasource, just not per result row)
2. Static cache within the index (just one uncached call per datasource)
3. Static + persistent cache within the index

They are in order of increasing potential performance gain, but also increasing complexity and, thus, potential conflicts with other code. Attached are implementations for the first two options (adding a persistent cache should really just be a last ressort). It would be great if some of the people with performance problems could give those a try and try to benchmark the performance gained for both cases.
The patch for 2), if we decide for that, would still need a bit of fiddling to determine in which scenarios the static cache should really be cleared. However, this will probably only lead to actual problems very rarely (and, possibly, in the tests), so this one should be good enough for initial evaluation.

In any case, this is just a potential hotfix for anyone encountering problems. The method would still need an overhaul to tackle the more substantial underlying problems discussed in the IS.

@ daften: Just because it's critical for your site doesn't make the issue critical.

Forgot to set to "Needs review".
Also, I'm now tackling the larger issue of actually overhauling the preRender() method. Let's hope for the best …

As suspected, running into a caching problems in the tests with the index-level caching. Easy to fix, though: the attached revision should at least prevent the highest-impact potential side effects. Though even getting into that situation is probably rather rare in actual use – it's a typical test thing, where too much happens in a single "page request"/process.

And here is a much-needed test specifically for the preRender() method (not including any of the previous patches).
If we come up with an improved version of the method, we can also add assertions that no unnecessary entity loads occur. Those would fail now, of course, though.

(As you can see, this already uncovered a small problem in the current code.)

This implements the second suggested improvement – avoiding to load properties that are already covered by what's on the result row object. It also fixes a problem I noticed with the $row->_relationship_objects array only being keyed by property path, not combined property path (e.g., body instead of entity:node/body), meaning that a datasource-independent property with the same name would overwrite those objects, leading to wrong data being displayed.
(Patch without the AggregatedFields doc comment fixes this time – sorry about that!)

(PS: The additional information in the remaining @todo comment is of course temporary, and only as a reminder for me. Please ignore.)

This adds part of the first suggestion, using multi-loads wherever possible for nested properties.
To do this, I needed to split the giant foreach ($values as $row) loop into two. I used this opportunity to move both of the new loops into their own method, to make the code a bit cleaner (hopefully).

Multi-loading for result items still missing.

This broke viewing of Field API fields (with Field API rendering) and, thus, the tests.
This revision should fix that.

This adds multi-loading for the result items. However, the effect is pretty much cancelled by \Drupal\search_api\Plugin\views\query\SearchApiQuery::addResults(), which has hard-coded single-loading of all result items, unfortunately. Since fixing that is out-of-scope for this issue, I've opened #2898327: Keep SearchApiQuery::addResults() from loading all result items individually as a follow-up.

Thanks a lot for reviewing!

This is a huge patch, so really hard to properly review

Yes, sorry about that. Maybe I should have saved the refactoring to helper methods for a second step?
Anyways, that's exactly why I wanted to have proper test coverage beforehand – with this in place, I'm pretty confident we're not breaking anything. However, there's of course still a lot of possibilities for errors – as your first remark demonstrates. Yes, there is of course a continue; missing, thanks a lot for noticing! This is probably a rather rare case, and will in most cases also not have massive consequences (the unnecessary load would probably be from static cache in most cases), but still, without the continue;, there's no point in that block at all.

Your second remark, though, is purely taste, and I personally dislike using empty() in cases where I know the variable (or whatever) is set. If there's a majority of people feeling that using empty() is better, I'd be open to still change it, but then we might want to go through the whole module changing this. I'm sure it's used in a lot of places already.

Would still be great if someone with actual performance problems with a search view could give these patches (here and #10/#13) a whirl and tell us how they affect performance for them. I'm pretty certain all of them will improve performance, but it's hard to say by how much.

Some more adjustments to also fix the problem reported in #2856915: Allow the display of more than one processor-generated fields/properties per type in Views.

Thanks a lot for reviewing!
Would have been great to get actual tests (especially from the people who paid good money for this), but apparently it's not happening and I'm tired of waiting. Even if this ends up not doing anything for performance, it still fixes some actual problems and improves test coverage.

The attached patch combines #13 and #25. If the test bot agrees, I'll commit it.

Committed.
Thanks again, everyone!

OK, cheered too early, this lead to an uncaught exception in some cases: #2910918: Form for configurable fields is broken (Edit: Corrected link – thanks, Joris!).