Optimize getCommentedEntity()

This is the easy part and getCommentedEntity() should pretty much vanish from your flamegraph.

The problem is that what's really expensive is the *first* call to a content entity field because we then need to load all the caches. So a fair amount of the time will move to buildLinks() and the calls in there now, unless we can optimize those as well.

Some should be possible, but at least $status = $commented_entity->get($entity->getFieldName())->status; is hard because that's a configurable field, we can't put that in a static list of entity keys. We'd need to make that apply to all fields, which does have costs too.

Completely untested, I probably made some stupid typos :)

But once it works, it would be interesting to do another flamegraph and look what this changes exactly. If we can get to the point of at least not having to load comment field definitions then that might help quite a bit already.

Yeah, as promised. that had a stupid typo (missing return) :)

This should be better. Also added it for field_name and status, which means that at least for comment, we don't need to load any field definitions to render those links. Still need for the configurable field on the commented entity.

I have some ideas on making getEntityKey() more generic and available for all fields, if you know what field/property you want. Maybe we can even read it out of $this->values if we don't have a field object. But the problem is that this doesn't work with either get() or __get() since that always needs to return the field item list, we do need a new API for that.

Urks. That's the nasty side-effect of using entity_keys, it alters the schema. Will look into being able to use something like this without having to specify entity keys.

First implementation of a function that doesn't go through entity keys to access the field values directly.

node/1 with a single comment with comment form on a separate page has no getTranslatedField() calls left for comment or node (only two for Shortcut).

This should fix those fails. Going to wait for performance tests before doing more work here.

+++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
@@ -505,6 +505,41 @@ protected function getTranslatedField($name, $langcode) {
+
+  public function getFieldValue($field_name, $property) {

A couple of more meta questions. a) Is there a reason we didn't went with this helper method in the first place. Wss it considered as premature optimization? Another question: In \Drupal\Core\Entity\ContentEntityStorageBase::setPersistentCache we cache the entity after loading. Could we introduce some "additional" caching layer for an entity with active field item lists?

a) I don't know, it's pretty hacky (try 7 places and hope for a value) and bypasses the usual API's.
b) We do the opposite actually, and remove those objects because they are huge and have recursive references on serialize.

On a) It doesn't work for everything, for example, which is why we have to re-implement ->entity ourself and can't just do getFieldValue('entity_id', 'entity').

One of the really problematic parts about field item objects are computed properties. They currently have to be created as property objects, which we can avoid for non-computed properties and to do so, we first need to figure out which are actually computed, which means we have to call propertyDefinitions().

See FieldItemBase::__construct(). If we can somehow avoid that loop there over the property definitions and create them instead on demand when requested, then that might be quite a big win for normal field access. The current code is optimized for being able to clone them later, but if we never actually access them then not creating them in the first place should even be better. The code was written and profiled with a very different Drupal, when accessing those kind of properties was way more common than it is now I think.

Note that in this case, it would actually *not* help, since ->entity is a computed property.

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -505,6 +505,41 @@ protected function getTranslatedField($name, $langcode) {
   +  public function getFieldValue($field_name, $property) {
   

   Doc block missing.

2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -505,6 +505,41 @@ protected function getTranslatedField($name, $langcode) {
   +    elseif ($this->values[$field_name][LanguageInterface::LANGCODE_DEFAULT]) {
   

   Can we explain this a bit?

#19: Nice, within CommentLazyBuilders::buildLinks(), we now spend ~90% in ContentEntityBase::access(), compared to ~50% before. Which means building comment links has gotten roughly twice as fast.

As mentioned in IRC, it looks like @moshe currently has assertions enabled, which isn't a good idea for profiling. In general, we seem to be spending a lot more time on access and loading the comment than we did in the flamegraph in the issue summary. And as @dawehner mentioned, we still have apc_store() calls in there. Which is definitely strange, that should only happen if the fast storage is getting entries evicted e.g. due to being full. But why would that happen when always requesting the same page? Also seeing that in multiple other places, e.g within the routing "tower" and loading block plugin definitions.

Also, the flamegraph in the issue summary AFAIK had comment form disabled, so rendering the links was 20%, in the before link above, it's "just" 10 and 30% on the comment form.

I also guess that @moshe was not using uid 1 this time, because I can see a call to getOwnerId() in the access check, which is probably an important reason why access is now more expensive. We can also optimize that, will do so in the next update. Also seeing 10% on the login block in there, so probably anonymous, without page cache?

So I've been doing some flamegraphs myself and my results look quite different different.

Setup: node/1 with 5 comments, normal user with edit own but not toolbar permission, smartcache working. comment form enabled.

renderLinks() is 5% before, 4% after. So it clearly is an improvement, but not comparable in any with @moshe.

Before:
https://dl.dropboxusercontent.com/u/2059345/node-with-5-comments-auth-Fl...

After:
https://dl.dropboxusercontent.com/u/2059345/node-with-5-comments-auth-pa...

That said, what's weird in my calls IMHO is that I'm seeing too much MainContent rendering, that should be cached? I do get a X-Drupal-Dynamic-Cache:HIT header.

We're currently discussing a likely bug in fast chained/apc where we are setting an incorrect ttl. That would explain the apc_store() calls and should change his numbers a lot as well.

The best reason I can think of is that you're not actually getting Dynamic Page Cache hits. But, one can clearly see the recursive cache get, which means a redirect is being followed, which means… that is quite unlikely.

The only other reason I can think of why MainContentViewSubscriber is showing up so significantly is that the first time it is rendered, it is so slow that it causes this many samples to be collected, and once you're getting Dynamic Page Cache HITS, it is so fast that everything else is showing up in such a relatively small way.

But that seems fairly implausible.

However, to rule that out, you would ideally do a test with Dynamic Page Cache disabled, so we can compare.

These flamegraphs are definitely very confusing.

From @Berdir #17

See FieldItemBase::__construct(). If we can somehow avoid that loop there over the property definitions and create them instead on demand when requested, then that might be quite a big win for normal field access

Indeed, not sure why that couldn't move to __get(). I guess that's a question for @fago ?

Coz other than that, yeah, introducing an alternate $entity->getFieldValue($field_name, $property_name) syntax, that works faster but doesn't work in all cases, is not really rejoicing :-)

Especially since Map::get() seems to have some logic to take care of lazy creating the typedDataManager()->getPropertyInstance() anyway ?

I have to confess that after a few months since the last time I had dig in there, the logic between FieldItemBase::__get() and the ::get() inherited from Map confuses me a bit now...

get() does, but __get() and various other places don't, it will likely not be a trivial thing to get working reliable. Anyway, here's a first patch: #2591447: Only initialize computed properties when needed.

However, that's a completely different thing. It does not make this unecessary in any way, we'd still need to load the field definitions from cache and create a field item and field item list class and so on.

This will always be way faster, for the cases where it can be used.

I think this can be a useful thing to add, with the proper documentation and name (@plach suggested getFieldItemValue() and some point in IRC).

Shortcut btw is another example where we access field objects on every request (for the users that have access only, of course).

@Berdir : I do get the performance argument, of course... I guess this is us finally coming to terms with the fact that the short syntax we've been advertising is a perf drag :-/

But offering fast access based on the raw ContentEntity::$values seems problematic ?
- wouldn't this bypass default values on a fresh entity ?
- would bypass any custom massaging of the values that the field type Item class might be doing ? (OK, I guess that's rare)
- more importantly, AFAICT ContentEntity::$values are currently only ever updated back to match the Item values in updateOriginalValues(), which is only called by the storage on preSave(). We could use onChange(), but I can't really help finding it a bit brittle / easy to sidestep.

So, "faster, but doesn't work on all properties, behaves differently on fresh entities, and overlooks the changes that might have occurred since the entity was loaded" seems potentially fairly puzzling & dangerous ?

I guess this is us finally coming to terms with the fact that the short syntax we've been advertising is a perf drag :-/

I don't know if we can blame short syntax for that, that is likely still faster than the long version because it's going through an additional typed data object to get the value. But yes, we should have a non-magic method to do the same as __get(), which we unfortunately don't.

- wouldn't this bypass default values on a fresh entity ?

No because:

+    // If a field object is already instantiated for this field, use that.
+    if (isset($this->fields[$field_name])) {
+      return $this->get($field_name)->$property;
+    }

- would bypass any custom massaging of the values that the field type Item class might be doing ? (OK, I guess that's rare)

Yes, I guess that is possible. But like for computed properties, then we need to document that. We need to make it very clear that this is about accessing known, specific fields/field types. Any kind of generic access should still use the "official" API.

- more importantly, AFAICT ContentEntity::$values are currently only ever updated back to match the Item values in updateOriginalValues(), which is only called by the storage on preSave(). We could use onChange(), but I can't really help finding it a bit brittle / easy to sidestep

Again no, see above.

As soon as we have field object, it uses that. That covers defaults, changed values, ... There's no way this would pass if that wouldn't work :)

I perfectly understand being wary about this, I am too, very. But it is also the only way out (that I can see) of having to load two big arrays from cache, unserialize dozens of objects, creating multiple objects and what not just to access a plain value.

+    // If a field object is already instantiated for this field, use that.
+    if (isset($this->fields[$field_name])) {
+      return $this->get($field_name)->$property;
+    }

Oh doh, how did I miss that ?
Yeah, so, no major objection on my side then ;-)

Code-wise :
- any way we can factor the mostly similar logic between the main "if" and "elseif" branches ? It seems about picking between $this->values[$field_name][$this->activeLangcode] and $this->values[$field_name][LanguageInterface::LANGCODE_DEFAULT], and then applying the exact same logic ?
- checks like "if (isset($foo['bar']) && is_array($foo))" are a bit intriguing ? Why is the is_array) check needed at all ?

Sounds like this issue is on track to make a fairly big performance improvement. Thanks @yched and @Berdir :)

I've added a bit to the summary on why this is an RC target. It'd be good to add an explanation of whether there is any disruption, though.

What remains to be done here exactly? @Berdir, I know you're on vacation. If you can give instructions, I'm happy to execute them for you.

The patch needs basic docblocks and some understandable documentation on when and how to use this method. Also try to address #30.

I got pulled into other things. I think we still want to do this?

AFAICT this wouldn't break any APIs, so it can still be done.

Rerolled, refactored and inverted the logic in getFieldValue().

Fixed that test.

+++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
@@ -503,6 +503,51 @@ protected function getTranslatedField($name, $langcode) {
+  public function getFieldValue($field_name, $property) {

I think this might cause some confusion for those using IDE completion, should we mark this method @internal and describe when/where it should be used?

Other than that, looks RTBC to me.

I don't think it has to be @internal, that means it can't be used and might be changed. What I don't know yet is if it should be public and on the interface or protected and just a helper method. I'm going towards public, as it's useful for callers as well. I'm considering to add more methods like that, like hasFieldValue(), for the common use case of checking if e.g. an entity reference is already present on a field.

Suppose the method should be named getFieldPropertyValue and part of FieldableEntityInterface

Looks fine to me, one small grammar thing:

+++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
@@ -503,6 +503,51 @@ protected function getTranslatedField($name, $langcode) {
+        // value are a scalar, just return that.

'values are' or 'value is' ?

reroll and fixes #45

Re-rolling patch for 8.2.x branch.
Patch is successfully applied.

Fixing couple of failures, it was not getting applied for 8.4.x
Re-rolling patch for 8.4.x.

Re-rolling for 8.6.x branch.

Currently it is failing for few cases where "entity_type" field is empty.
Currently there is one issue which is in progress for this https://www.drupal.org/project/drupal/issues/2820364
So this issue will add restriction which won't allow to have empty base field values.

May be we should attempt this once it is merged.

Some things I noticed in the patch.

+++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
@@ -611,6 +611,51 @@ protected function getTranslatedField($name, $langcode) {
+      elseif ($this->values[$field_name][LanguageInterface::LANGCODE_DEFAULT]) {

This will fail if the value is FALSE or 0 (valid values). We should probably use isset just like we do 3 lines up.

+++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
@@ -611,6 +611,51 @@ protected function getTranslatedField($name, $langcode) {
+        if (isset($field_values[0][$property]) && is_array($field_values[0])) {
...
+        elseif (isset($field_values[$property]) && is_array($field_values)) {

The order of isset and is_array should probably be turned around.

is_array is only executed when the isset returns true at which point we already know we're dealing with an array.

Did changes as per #60, please review:)

Providing interdiff of patch #56 and #64.

This should work fine:)

Re #60:

While it possibly can be optimized, the reason for that is_array() is that isset($field_values[$property]) does *not* guarantee that $field_values is an array: https://3v4l.org/ZP77f.

You're of course right Berdir. I did have in mind that $some_str = 'text'; $b = $some_str['letter']; throws a warning. I forgot however that isset is not a function but a language construct so it circumvents that.

Reroll, removed the isset/is_array order changes.

We improved the handling of those fields, at least one of these tests wasn't failing for me anymore.

Still not clear objections to introduce another public api to access properties of fields? Initial idea was to skip loading commented entity for no reason but then entity static cache came

@andypost: I'm not sure yet about where the method should, just wanted to get it back to green first.

I don't really get your second paragraph, the idea was always (at least starting with comment #1) to avoid having to load the field definitions and having to initialize that stuff. The entity as well as the commented entity has to be loaded anyway, but especially the commented entity is almost certainly already in the static cache, as the 95% performance-relevant-use-case is viewing the comments on the canonical route of that entity.

Added another check to avoid failing if a bogus entity type is defined, I guess we can't prevent migrate from filling in stub values.

+++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
@@ -618,6 +618,51 @@ protected function getTranslatedField($name, $langcode) {
+   * Gets the value of a specific property of a field.
...
+   * Only the first delta can be accessed with this method.
...
+  public function getFieldValue($field_name, $property) {

I simply love this! Absolutely great idea!

@Berdir++++

The entity API creates a lot of objects, especially on saving an entity. We have a lot of nested entity references with multiple translations (~10). When we save the whole structure at once, then our memory usage increases a lot. I've been measuring memory usages of up to 700 MB. Having so much objects triggers at some point the PHP garbage collector, which starts sweating, takes a lot of time and still cannot free a lot of memory, because of the circular references - something like the composer case. Therefore we just have to turn it off to speedup the system and yes this gives us a pretty good speedup. Yes, the memory usages increases but for us it is better to require more memory but to reduce the processing time. Therefore I was dreaming about an approach, where we could bypass the creation of so many objects when interacting with the entity API. I think that we could do this for all the simple fields.

Some suggestions:
1. I think we could make $property optional. For that we could add a class property to the entity class, where we could store the mapping field_name => main_property_name. I think this would not introduce that much of an overhead, because when we are loading an entity, we already have retrieved the field definitions. In \Drupal\Core\Entity\Sql\SqlContentEntityStorage::loadFromSharedTables() we retrieve the table mapping and the responsible method \Drupal\Core\Entity\Sql\SqlContentEntityStorage::getTableMapping() is retrieving the field definitions.
2. I think it would be nice to provide the ability to either load only the value of a specific field delta or all of them.
3. We could add a setFieldValue($field, $value) method, which could be used to directly alter the values in $entity->values if there is no field object initialized. However we can do this only if the field does not have any logic in the onChange method, which I think is the case for fields having only a single property.
4. Further we could prevent ContentEntityBase::hasTranslationChanges() from initalizing fields and instead only compare the raw values.
5. Much further we could also prevent \Drupal\Core\Entity\Sql\SqlContentEntityStorage::mapToStorageRecord() from initializing the field objects.
6. Much much further - we could also prevent loading entity translation objects by adding a $langcode parameter to the new API methods for setting and getting a field value.

Thinking further about this I realize that we could do this also for multiple property fields - for this we should make the field methods like setValue and onChange static.

I realize, that it would be a tremendous work to do all this, but I think that it will be worth it, as at the end the entity API will get a performance boost and also decrease memory usage.

Thanks for the feedback.

1) Hm, yes, that might work, but it also requires more logic, will think about it.
2) That would result in either varying result types (scalar or array) or multiple methods. I don't like the first and the second could be a follow-up?
3) Seems like a separate issue to me, as you wrote, that would get tricky with tracking changes and so on...
4) Not sure since fields can have their own comparison logic now. We could possibly only compare initialized fields, but that's a separation that's unrelated to this.
5) i have an issue somewhere where I tried to optimize that, specifically tried to write it so that it only updates base tables if they changed, similar to dedicated tables, but profiling showed that currently, calculating the difference is slower than just doing the queries. We could try to look into optimizing that somehow, yes.
6) I don't really see how that would since getTranslationFromContext() works on the entity and needs that to decide if the entity is available in a given language.

Lets try to get something simple done before get into the advanced use cases. That does mean that we can't change the API anymore, so we need to decide on that.

+1 on having this method.

I discovered this through similar work I was doing for a performance issue we were encountering in Organic Groups (ref. PR #555). We noticed that cached pages were unreasonably slow for users that have a lot of groups, and the slowdown was happening in a cache context. I was surprised to find that most time was spent in reading field values.

I did some profiling in that issue with my own variant of the solution that is proposed here. In my testing I was traversing over 1140 entities, and doing a small number of field value lookups per entity. I have assertions turned of and the result is an average of 3 test runs.

• Before patch: 99.05ms
• After patch: 0.26ms
• Performance gain: 38096%, or almost 400x faster

I will leave the "Needs profiling" tag since I have tested with my own variant and not the actual patch posted here, but it looks like the speed gain will be substantial.

Rerolled for 9.3.x.

Tests fail because the comment_entity_statistics table doesn't exist.

Fixed issue in the patch.
Trying to get this rolling...

Believe issue summary still needs attention.

New functions and parameters should be typehinted.

Is there profiling results to review as well?

Recommend using MRs as well.