comment_entity_storage_load() is too expensive on cold caches

Lets go for #2, statistics don't belong on the object / field itself, entities should really be immutable.

Could always add a static cache to comment_prepare_statistics for now ...

So originally we had a 'calculated' statistics field.
What if we did this:

• Add CommentStatisticsInterface::markForRead($entity_type, $entity_ids);
• When comment_entity_storage_load is called, instead of calling ->read, call markForRead.
• Collapse the properties in CommentItem back to a single statistics property, calculated - like we had at one point.
• In the getValue method of the calculated property call CommentStatisticsInterface::read, but instead of just reading the single value, make it also include anything flagged with markForRead and keep it in a static cache.

So essentially, lazy loading - we keep track of the items that might need to be read, but we don't read them until we actually use it?

Or is that basically going to end up with the same thing in terms of memory use?

Looks like duplicate of @catch's issue

Also there's #2318875: Redo CommentStatisticsInterface but not sure that fits into beta stage.

The primary problem with comment stats that we need comment_count in comment, forum, and rdf modules as entity property mostly always before build/render happens.

#13: Well, usually hopefully during render stage, not before.

Also a simple:

$comment->getStatistics() should be enough for that case, which is pre-loaded in entity_prepare_view().

Changing entity properties after loading that are not part of the schema is highly problematic and leads to different problems in regards to cacheability and future placeholdering, so if we can avoid it: Great!

@Fabianx not $comment->getStatistics() but $commented_entity! SO no way to add this method to any entity that have comment field attached

next on my radar

First pass

So instead of checking for a comment-field on the given entity-type, we just keep track of all content entities that pass through comment_entity_storage_load in a new service (CommentStatisticsLazyLoaderInterface).

We then convert the CommentItem->{various} statistics columns into their own data types.

When ->getValue is called on any of these, we load the statistics (in bulk) just-in-time.

This means it is harmless to mark an entity that is loaded as needing statistics (because the statistics will never be requested).

I think this gives us the best of both worlds - we retain the bulk loading, retain the 'no querying for entities that don't have comments' and lose the expensive field checks

Also, if the statistics aren't even used on the page, no loading happens at all (which isn't the case in HEAD).

Needs tests for some unit tests of the new service.

So in some instances node view builder is being invoked before the node is loaded? how does that work? render cache contexts?

Unfortunately when caches are cold - such as after submitting the modules page, the field check takes 125ms and 4mb of memory.

We need to redo that profiling.

That information is from before the field map was rewritten to be persisted, we do not calculated that anymore like we used to, we just load it from key value, should be much, much faster.

There might still be reasons to do this, but I'm fairly sure that we should change this to a normal issue or maybe even close it.

Re #23:

Unfortunately when caches are cold - such as after submitting the modules page, the field check takes 125ms and 4mb of memory.

We need to redo that profiling.

That information is from before the field map was rewritten to be persisted, we do not calculated that anymore like we used to, we just load it from key value, should be much, much faster.

There might still be reasons to do this, but I'm fairly sure that we should change this to a normal issue or maybe even close it.

This is not the only problem, therefore I would not close the issue.

I was chasing performance problems with our application and I've noticed some strange behavior. Consider an entity type with a comment field.

Execute the following command considering there are 20000 entities in the system.

drush ev 'gc_disable(); $n = 20000; $s = \Drupal::entityTypeManager()->getStorage($entity_type); while ($n > 0) {$s->loadUnchanged($n); $s->resetCache(); $n--;}'

Now because of the disabled garbage collection for cyclic references and the initialization of a field on entity load the memory consumption will be raising on each entity load, which consequently might/will exhaust the allowed memory size.

Even with the garbage collection for cyclic references enabled it is not a good idea to execute unneeded queries, especially when a large amount of entities are being loaded one after another and not at once.

---

We have a comment field, but we don't need the statistics at all and would actually turn them completely off. Loading the statistics lazy only when they are needed seems to be the best solution for me.

A plain reroll against 8.9.x. The only meaningful difference (except moving some lines up and down) was removing of condition

if (!\Drupal::hasService('comment.manager')) {
     return;
   }

in comment_entity_storage_load() that was fixed in #2527866: Investigate why comment.manager service doesn't exist when comment_entity_storage_load() is fired in ConfigImportAllTest

the number of failures is less then previous patch

Did a little fixing for tests. The only failing unit test, CommentStatisticsUnitTest, had to be updated because CommentStatistics::read() now returns associative array. To fix CommentItem kernel test, small change was made to CommentStatisticsLazyLoader, obviously, the possibility of missing statistics was not accounted for completely.

When we access a property we go through the magic methods.

$node->comment_field->comment_count should trigger

-> \Drupal\Core\Field\FieldItemList::__get()
-> \Drupal\Core\Field\FieldItemBase::__get()

However if we haven't accessed the property yet through \Drupal\Core\Field\FieldItemBase::get($property_name) which is implemented on the parent - \Drupal\Core\TypedData\Plugin\DataType\Map::get(), then the property will not be initialized yet and therefore \Drupal\Core\Field\FieldItemBase::__get() will return NULL.

The problem is that \Drupal\Core\Field\FieldItemList::__get() calls the magic method \Drupal\Core\Field\FieldItemBase::__get() and not the method that actually initializes the properties - \Drupal\Core\Field\FieldItemBase::get($property_name) and the reason for that is that it also returns values that are not field properties as documented in the method's body.

I think that the problem here lies within \Drupal\Core\Field\FieldItemBase::__get():

  public function __get($name) {
    // There is either a property object or a plain value - possibly for a
    // not-defined property. If we have a plain value, directly return it.
    if (isset($this->properties[$name])) {
      return $this->properties[$name]->getValue();
    }
    elseif (isset($this->values[$name])) {
      return $this->values[$name];
    }
  }

According to this code it will work only if the property has been initialized already, which will not be the case until the ::get() method is called.

Maybe we should try loading the property if it is not yet initialized:

  public function __get($name) {
    // Initialize the property first if it has not been initialized yet, as its ::getValue() method 
    // might contain some default value or lazy loading logic.
    $property = $this->properties[$name] ?? $this->get($name);

    // There is either a property object or a plain value - possibly for a
    // not-defined property. If we have a plain value, directly return it.
    if (isset($property)) {
      return $property->getValue();
    }
    elseif (isset($this->values[$name])) {
      return $this->values[$name];
    }
  }

If I am correct then Drupal\Tests\comment\Functional\CommentStatisticsTest should not fail anymore after that or at least the assertions which fail because of returned value NULL should pass.

Thank you for digging it, it reminds me about why we override list class for comment field, because default values sometimes missing

Approach in #34 seems promising, but it needs a bit of refinement. We step ahead of checking value logic with $this->get($name), which may have unexpected side effects. In fact, implementing this approach exactly as suggested didn't fix the test and broke my local installation immediately with the message

InvalidArgumentException: Property _original_delta is unknown. in Drupal\Core\TypedData\TypedDataManager->getPropertyInstance() (line 188 of core/lib/Drupal/Core/TypedData/TypedDataManager.php).
Drupal\Core\TypedData\Plugin\DataType\Map->get('_original_delta') (Line: 139)

I will try to move things around in FieldItemBase::_get() to achieve desired result.

Meanwhile, I tried to initiate properties in comment_entity_storage_load(), and it worked to the extent I've seen. Let's see what tests report. But as we are chasing the speed here, I'm not sure if this solution fits 100%.

Also new version of patch contains one kernel test fix.

Here's implementation of approach from #34. As my superficial testing showed, there was no bad in trying to call $this->get($name) in FieldItemBase::__get() in last place. We will get a meaningful exception if we try to address a non-existing property instead of NULL, which is even better. I'm giving this patch a number of 38b instead of 39 to show that it's not a consecutive development, but a different approach.

Seems there were too many exceptions with non-existing properties. So adding a check if property exists for the field. Also fixed code style.

There is a number of changes to the patch in previous comment.
First, we are introducing clearValues method for CommentStatisticsLazyLoader to flush the statically cached statistics.
Then, we are introducing a LazyLoadableInterface that should be implemented by lazy-loadable typed data and use it for stricter check on if we should try to load values for non-instantiated properties in FieldItemBase::__get().
Third, in CommentItem::getValue() we needed to instantiate properties to get behavior consistent with the current one.
And some tests were fixed (kernel test by adding filter module - though it is not needed with the introduction of p.2), functional test by using the above clearValues method, because resetting node storage cache wouldn't be enough to retrieve the actual data.

In this update,

• I changed the signature of clearValues method, introduced in previous patch, so that now it accepts one array parameter instead of 2,
• optimized calling this method in tests to clear only necessary loadedValues elements,
• also implemented hook_ENTITY_TYPE_insert and _delete for comment entity in comment module, to clear this static cache during one request,
• removed now unnecessary filter module from kernel tests, where it was enabled with the previous version of the patch,
• updated description for LazyLoadable interface

reroll with adaptation to typo fix