Improve ContentEntityBase::id() for better DX

This was done for performance, to avoid the additional calls every time it's called.

However, that was before Entity Field API happened with the objects and everything, now that's actually the overhead.

What we should do is keep a computed id property around, something like $this->computedId and then only fetch the id once and after that, return it from that.

We basically do that already for the bundle and the language, the problem there is that they have very unfortunate names right now (bundle and language), which means that you can't have a content entity where the bundle key is named bundle.

Maybe have entityKeys or computedKeys or so for this, that we can easily check? Then we can use more or less the same logic for id, revision_id, bundle, uuid and langcode/language?

Well the easy solution is that those simply can't use a default implementation.

Entities must have a ID property/field. Those compound keys are backed by special code like this in ConfigStorageController::save():

    // Build an ID if none is set.
    if (!isset($entity->{$this->idKey})) {
      $entity->{$this->idKey} = $entity->id();
    }

This is what I was talking about above.

This doesn't gain that much performance yet, but we could further optimize it to already preset the values in the constructor, so that we don't have the initialize the field objects as long as we don't change anything.

Oouch @ those unit test mock definitions... Seriously considering to limit this issue to content entities.

Crazy, no idea how there weren't given my bogus onChange() implementation.

The createDuplicate() tests probably all use EntityTest, where getKey('id') == 'id'.

The bundle must not be reset.

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -606,6 +606,13 @@ public function onChange($name) {
   +      if (isset($this->entityKeysCache[$key]) && $key != 'bundle') {
   +        unset($this->entityKeysCache[$key]);
   +      }
   

   it would be great to document why we don't want to unset the bundle.

2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -991,4 +998,28 @@ public function referencedEntities() {
   +   * @param $key
   

   Is the entity key always a string?

3. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -991,4 +998,28 @@ public function referencedEntities() {
   +        $property = $field_item->getMainPropertyName();
   +        $this->entityKeysCache[$key] = $field_item->$property;
   

   Is there an API to fetch the main property value?

In general I wonder whether we really need to call the id() method in the constructor, is there no way around that?

1. Because bad things happen :) The bundle is very very important for content entities, as it defines what fields an entity has, if it's not set, then it can start to recurse as it needs to know the bundle to acess the bundle field ;)

2. The value? string or int.

3. That's the API :) There are only a few use cases, so not sure if it would save much to have a method there. There are also use cases to know just the name, not the value (validation), that's why it was added, actually.

I still think it might be better to ignore config entities here and just deal with content entities, as the implementation is quite different and config entities might also need further optimization?

24: 2182239-id-key-24.patch queued for re-testing.

Ok, as suggested, going back to content entities, config entities are quite a different beast.

A bunch of changes:
- Introduced the pre-fill of the values in the constructor, this means that for entities loaded from the database, we never need to instantiate field objects
- Updated the getMainProperty() stuff for the new API.
- Avoid creating the EntityDataDefinition over and over again, this is somewhat unrelated but when profiling, I noticed that this is triggering *by far* the most calls to bundle()

Note that the removal of all the config stuff from the patch is not part of the interdiff, as that's just noise, the interdiff just shows the interesting parts.

Profiling is fairly hard as the method calls are moving to different places, but with the pre-filling and the persisted dataDefinition, this is definitely an improvement.

Ok, we still need that loop for the isDefaultRevision flag, although we could argue to either hardcode that specific thing or require to set it directly?

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -129,20 +122,43 @@
   +      // to set properties like isDefaultRevision.
   +      // @todo: Should this be converted somehow?
          if (property_exists($this, $key) && isset($value[Language::LANGCODE_DEFAULT])) {
   

   I'm not sure this is still required for content entities, there shouldn't be any defined properties being non-fields left?

2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -129,20 +122,43 @@
   +        if (is_array($this->values[$field_name]) && isset($this->values[$field_name][Language::LANGCODE_DEFAULT])) {
   +          $this->entityKeysCache[$key] = $this->values[$field_name][Language::LANGCODE_DEFAULT];
   +        }
   

   People could pass in the value without making use of the whole field structure. So we'd have to instantiate the field to get the value from there, what sucks as it potentially instantiates uneeded fields. Maybe the cache could be filled on-demand?

3. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -987,4 +1014,28 @@ public function referencedEntities() {
   +
   

   Unnecessary new line.

berdir clarified on IRC that the cache is already filled on demand, that's great. I guess 2 is fine as long as it keeps working with non-field-structured values as well, while the best performance of totally bypassing fields could be only achieved with the field structure.

As discussed on IRC we should benchmark to see what this actually brings.

Also, I must say the $entityCacheKeys property looks a bit ugly to me. Thinking about it, I'm wondering whether we shouldn't add a property as static cache per entity key as we already have $bundle ? Imo this would be less ugly, and with PHP >5.4 it should result in better memory usage also. (Declared properties use C structs then. I checked memory usage of defined properties vs 1 array property holding keys in the class definition issue and it was better with php>5.4.)

See #2168431: Warn developers not to name entity bundle "bundle" or to unset it if they do. Right now, you can not really use bundle as the bundle field, weird things happen.

Also, I need to be able to separate between not having a certain entity key and not yet having it loaded. Still thinking about how to optimize that.

Re-roll, the namespace was fixed as I had a conflict there anyway.

I tried a few things to get meaningful numbers and finally ended up with this:

use Drupal\Component\Utility\Timer;
use Drupal\node\Entity\Node;


$values = array(
  'nid' => array(Drupal\Core\Language\Language::LANGCODE_DEFAULT => 1),
  'vid' => array(Drupal\Core\Language\Language::LANGCODE_DEFAULT => 1),
  'uuid' => array(Drupal\Core\Language\Language::LANGCODE_DEFAULT => 'uid'),
);

for ($i = 0; $i < 100; $i++) {
  $node = new Node($values, 'node', 'article');
  
  Timer::start('id');
  $node->id();
  $node->uuid();
  $node->getRevisionId();
  
$end = Timer::stop('id');
}

print $end['time'] . "\n";

I'm mostly interested in avoiding to create field objects at all, so not doing repeated calls on the same object. And creating the object manually is the only way I can ensure that, load() will for example already call id() once.

This gives me 1-1.2ms with the patch, and 16-19ms on HEAD.

My initial script used load() and a loop on id() and the patch was still ~3 faster, even with the field object already created before.

As commented above, I don't think we should use properties directly for those key values, as we either have to use weird names or we make it impossible to use fields named like those keys, it is currently already impossible to have a bundle field, as written above. I want to fix that as part of this issue.

I'd also be happy to add unit test coverage when #2134857: PHPUnit test the entity base classes lands first, but I don't think we should try before that.

Also using the method for he label now.

Fixed that test.

Renamed entityKeysCache to entityKeys, with that rename, @fago confirmed in IRC that he is OK with this.

1. No it doesn't use more memory, adding a "print memory_get_peak_usage()" at the end of my example script above results in exactly 8784 bytes less peak memory usage ;) Not creating that object hundreds of times per request is saving much more than it is to keep one instance around.

2. As you said you said yourself, we should avoid going through the onChange() chain as much as possible anyway, this will be a bit slower, but we can live with that I think.

Re-roll of the re-roll after the entity base test patch was reverted.

Fixed the unset, that apparently wasn't renamed...

+++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
@@ -985,6 +1017,30 @@ public function referencedEntities() {
+  protected function getFromEntityKeyCache($key) {

Neither it is a static method nor it is using caching so why are we using name getFromEntityKeyCache. Yes entityKey is using a static cache but imo it is simple getter function.

Hm, the name is a bit tricky, yes. It is a cache, though, and #597236: Add entity caching to core follows a very similar pattern with getFromStaticCache() and getFromPersistentCache(), the former "just" accessing a property ($this->entities), exactly like this here. On the other side, it also returns it from the actual field if they cache isn't existent, so in this case, you could argue that it's an implementation detail.

Not sure what else to use, though. I'm not sure if just getEntityKey() or getKey() is clear enough that it returns the value for an entity key, unlike $entity_type->getKey(). It's a protected method, so it doesn't matter that much, it should only be used in rare cases outside of ContentEntityBase anyway.

Fair enough so I think it is RTBC.

Re-roll automerged by git rebase, so keeping at RTBC.

Back to RTBC.

Needs reroll.

Trivial conflicting in Term.php, patch diff only shows context changes, so back to RTBC.

Thanks for the rename, that's way better! Patch looks good to go to me as well.

+++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
@@ -599,6 +617,15 @@ public function onChange($name) {
+    // that check, as it ready only and must not change, unsetting it could

True, but that means we should throw an exception then I guess? We've got the 'read only' flag in typed data which does not throw any exceptions or so right now (as we did not had onChange() initially), so what about throwing exceptions for changes on read-only properties in general?
-> should be its own issue.

Opened #2221955: Prevent changes for read-only fields (like bundle).

Not sure about change record?

+++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
@@ -129,20 +122,43 @@
+  /**
+   * The instantiated entity data definition.
+   *
+   * @var \Drupal\Core\Entity\TypedData\EntityDataDefinition
+   */
+  protected $dataDefinition;

This should be NULLed in ContentEntityBase::__sleep()

Sure, can do that, just want to clarify that we're talking about this:

O:49:"Drupal\Core\Entity\TypedData\EntityDataDefinition":1:{s:13:"*definition";a:1:{s:11:"constraints";a:2:{s:10:"EntityType";s:4:"node";s:6:"Bundle";a:1:{i:0;s:7:"article";}}}}
That's exactly 179 characters. a few more for longer entity types/bundles ;)

Added that.

+++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
@@ -129,20 +122,43 @@
+  protected $entityKeys = array();

Sorry :( I should have realised that we should NULL this in __sleep() too. I think serialised entities should be as small as possible since if their storage is not on the same machine as the running php process then this will minimise network traffic.

Sure ;)

64: 2182239-id-key-64.patch queued for re-testing.

As discussed, we can't unset the entity keys because at least the bundle is required otherwise things get very sad.

70: 2182239-id-key-70.patch queued for re-testing.

Re-roll.

yeah, no entity without a bundle ;) So let's keep it then - back to RTBC.

2182239-id-key-73.patch no longer applies.

error: patch failed: core/lib/Drupal/Core/Entity/ContentEntityBase.php:700
error: core/lib/Drupal/Core/Entity/ContentEntityBase.php: patch does not apply
error: patch failed: core/modules/comment/lib/Drupal/comment/Entity/Comment.php:62
error: core/modules/comment/lib/Drupal/comment/Entity/Comment.php: patch does not apply
error: patch failed: core/modules/node/lib/Drupal/node/Entity/Node.php:62
error: core/modules/node/lib/Drupal/node/Entity/Node.php: patch does not apply
error: patch failed: core/modules/taxonomy/lib/Drupal/taxonomy/Entity/Term.php:57
error: core/modules/taxonomy/lib/Drupal/taxonomy/Entity/Term.php: patch does not apply

Re-roll, was fairly trivial, just context change, so keeping it at RTBC.

Those unit tests... ;)

Re-roll looks good. Looking at the patch again, I found the following though:

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -908,6 +939,8 @@ public function createDuplicate() {
   +    $duplicate->entityKeysCache = array();
   

   Should be $duplicate->EntityKeys!

2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -979,6 +1012,30 @@ public function referencedEntities() {
   +  protected function getFromEntityKeyCache($key) {
   

   Why not just call it getEntityKey() ? I guess that's a matter of preference though :)

Yeah, see #48 about the name, the problem IMHO is the difference between $entity_type->getKey() and $this/$entity->getEntityKey() but ok, let's try that.

getEntityKeyValue() else maybe? However, as this is protected I do not see that as a problem.
The update looks good, although I'm still wondering why it's necessary to set the field item class on the definition? Do you know? Maybe that would justify a comment for the next poor soul having to fix this test.

You mean the unit test change? That is needed because that's how the mainPropertyName lookup works, the definition class calls the static method mainPropertyName() on the field item class. And mocking the field definition class would possibly not be trivial either and this allows us to use more actual code instead of mocks.

sure, I see. Yeah, keep using the regular field definition class makes sense to me as well.

Back to RTBC then.

Btw, I closed #2227711: Consider whether to make ContentEntity properties used by uuid(), bundle(), language(), and getRevisionId() not fields as a duplicate of this because it mostly avoids the problem that is described there.

Committed 7ad0c57 and pushed to 8.x. Thanks!