Views results cache has full entities in it

Not that we have entity load cache, I would think we can just cache ids.

We have to cache basically the result coming from the DB which might be just entity IDs but could be arbitrary additional values.

We should though throw away the loaded entities and reload them on cache loading.

We could do something like that.

Yes, I would be pretty happy with this approach. I think it would need to be like this instead though, so we don't assign the view result to NULL every time. But yeah, I guess it was just the idea there mainly.

It's a good point that we now should not really need to cache full entities. We will probably want to measure the performance difference here, with the additional queries needed 'n' all.

Ha

Also, re-titling/purposing. As making changes to the output cache will need alot more discussion, and is a much more complexed topic :)

The patch we have here is fine for the result cache.

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/cache/CachePluginBase.php
@@ -137,6 +137,25 @@ public function cacheSet($type) {
+      $clone = clone $row;
+      unset($clone->_entity, $clone->_relationship_entities);
+      $return[$key] = $clone;

Please add a comment explaining (1) what we are doing here and (2) why. It's not really clear from the phpDoc what 'preparing' means.

hmm ok, That bit of code you've pasted seems pretty self explanatory to me tbh. The code is self descriptive (clone x, unset(x,y), assign). Did you also see the param docs? I think they complete the picture too:

   * @param \Drupal\views\ResultRow[] $result
   *   The result containing loaded entities.
   *
   * @return \Drupal\views\ResultRow[] $result
   *   The result without loaded entities.

Here is a comment to that effect anyway.

Back to RTBC.

Hmm. Could we not load the entities after the cache_set()? Then it wouldn't be necessary to remove them from the cache entry and add them back. Thinking about other stuff than loaded entities which might get in here at some point.

I didn't look at how feasible that is, if the answer is 'not at the moment', then OK but could do with a follow-up.

I think that's a 'not at the moment' I'm afraid. cacheSet is called from ViewExecutable after query->execute() is called, it's in there that the rows are loaded, and loadEntities() is also called etc...

I kind of like the fact that the query plugin is not aware of caching at the moment. The question on the other hand is whether the view executable should instead call the load entities method, as even only the query plugin will know exactly how to load them, it should not need to know when to load them.

This though is indeed more a follow up.

Back to catch!

OK that's fair enough. Committed/pushed to 8.x, thanks!

Great. Thanks, and merry christmas, catch.

Reopening, looks like this never actually got committed :D

So here is a reroll. Good place to start.

I kinda have the feeling we should test that we don't serialize the _entity components.

The entities themself are aren't even that much of a problem, they are pretty well optimized, although caching separately could result in cache inconsistencies if they are updated in the meantime. The biggest problem is really the ->view property, that serialized the whole view.

Quick test

+++ b/core/modules/views/src/Plugin/views/cache/CachePluginBase.php
@@ -341,6 +343,29 @@ protected function getCacheTags() {
+      unset($clone->_entity, $clone->_relationship_entities);


+++ b/core/modules/views/src/Tests/Plugin/CacheTest.php
@@ -166,7 +166,37 @@ public function testSubqueryStringCache() {
+      $this->assertIdentical($row->_entity, NULL, 'Cached row "_entity" property is NULL');
+      $this->assertIdentical($row->_relationship_entities, [], 'Cached row "_relationship_entities" property is empty');

Evidently, this patch works, because it's green (unless $cache->data['result'] is empty, which you also may want to add a preventive assertion for, to prevent future regressions).

But how is it possible that the result of unset() is NULL in one case and the empty array in the other?

Added an assertion that results are in the cache, that's a fair point.

But how is it possible that the result of unset() is NULL in one case and the empty array in the other?

This is possible because...serialization.

Try it:

$row = new \Drupal\views\ResultRow();
unset($row->_relationship_entities);
var_dump($row);
// Property gone.
var_dump(unserialize(serialize($row)));
// $_relationship_entities = array() again

Oh, those are the defaults on ResultRow. Then that makes sense :)

Considering this was RTBC before, and that all that what was missing was more test coverage, I think this is now RTBC again.

Talking to berdir on IRC, maybe we should set the values to their original instead of unsettings, but then, maybe we just add a method to call that does this for us.

EDIT: looks like x-post with WIm there. Wim, what do you think about this latest change? re-RTBC? :)

Interdiff looks good to me.

Yep, trivial changes. RTBC+1

Thanks both.

Committed/pushed to 8.0.x, thanks!