Node::isPublished() and Node::getOwnerId() are expensive

Perhaps a stupid question but as entity keys aren't translatable, how could we mark a specific translation as published/unpublished if the status is an entity key?

Or is @Berdir working on that in the other issue?

Perhaps a stupid question but as entity keys aren't translatable, how could we mark a specific translation as published/unpublished if the status is an entity key?

I see we load those in the beginning of core/lib/Drupal/Core/Entity/ContentEntityBase.php:166, so what about not loading them on pages with multilingual content?

Good point. I don't think not loading them is an option, I doubt that's actually possible and it prevents it from working for non-translatable keys as well.

Maybe we need to key $this->entityKeys with the langcode.

So an ContentEntity object is never constructed with more than one langcode, right? Are we sure that we can't load it, as you expect it, in the right language?

Yes it is. A contentEntity always has all the langcodes and translation values and one of them is active.

So could we do $this->entityKeys[$langcode][$key_name] instead of $this->entityKeys[$key_name] and fill up all the langcodes that have a value? And then in getEntityKey we use the active langcode?

Yeah, something like that is what I was thinking. A bit of overhead for id/type/uuid that aren't actually translatable, but that's tricky to identify without causing additional overhead

OK, just posting a rudimentary/untested patch for feedback and so we can see what this would look like.

+++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
@@ -149,7 +149,7 @@
+    $this->entityKeys['bundle'][LanguageInterface::LANGCODE_DEFAULT] = $bundle ? $bundle : $this->entityTypeId;

@@ -508,12 +510,12 @@ protected function setDefaultLangcode() {
-      $this->entityKeys['langcode'] = $this->defaultLangcode;
+      $this->entityKeys['langcode'][LanguageInterface::LANGCODE_DEFAULT] = $this->defaultLangcode;

@@ -985,23 +987,25 @@ public function referencedEntities() {
+  protected function getEntityKey($key, $langcode = LanguageInterface::LANGCODE_DEFAULT) {
+    if (!isset($this->entityKeys[$key][$langcode]) || !array_key_exists($key, $this->entityKeys)) {

Should we not set those special keys to all languages, so we don't trigger ->get() for them?

Yes, this currently does some unnecessary gets as we only set the language-specific values that are defined in $this->values. But maybe we can think of a cleaner way than just copying the value from the default language to all other possible languages?

There's no way for us to distinguish between translatable/untranslatable keys without too much overhead either?

@Berdir what do you think?

Was thinking about exactly that. We could maybe have two properties, $this->entityKeys and $this->entityKeysPerLanguage. We could check the first and the second and if neither has it, add it to the right one based on the field definition isTranslatable() information?

I had thought of something similar yes. $this->entityKeys would contain the non-translatable keys only and $this->entityKeysPerLanguage the translatable keys only?

Yes, exactly.

Just posting another unfinished (broken) patch for feedback. Maybe one of you can easily see what's wrong here (it's ignoring me when I try to unpublish a node).

On a node this gives:

entityKeys: bundle, id, revision, uuid
entityKeysTranslatable: label, langcode, status, uid, default_langcode

Actually this shouldn't be green as the following didn't work with this patch:

- enable content translation
- add a language
- add a translatable content type
- translate a node to another language
- unpublish the translation through the node edit form

---> translation is still published

So we probably need to add a test for this.

Just posting another patch here that should to illustrate the problem.

Under certain circumstances, when we unset the $node->entityKeysTranslated property in the onChange method triggered during publication of the node (i.e. when we do $this->parent->onChange($this->name) in ItemList::setValue() during the ::publish handler), this change is not being propagated to the entity object in the node form, so isPublished will still get the "outdated" value in the "status" entity key when the field has already been updated, resulting in the translation not being saved correctly. I could only reproduce this on a translatable content type without custom fields, with a body and a title only, when adding/editing a translation.

This may be pointing to an underlying bug...

I can't tell where, but it feels like there would be a great place somewhere to add some documentation, why we are doing that: performance optimization for some cases ...

+++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
@@ -166,13 +173,26 @@ public function __construct(array $values, $entity_type, $bundle = FALSE, $trans
+          $definition = $this->getFieldDefinition($field_name);
+          if ($definition->isTranslatable()) {
+            foreach ($this->values[$field_name] as $langcode => $field_value) {
+              if (is_array($this->values[$field_name][$langcode])) {
+                if (isset($this->values[$field_name][$langcode][0]['value'])) {
+                  $this->entityKeysTranslatable[$key][$langcode] = $this->values[$field_name][$langcode][0]['value'];
+                }
+              }
+              else {
+                $this->entityKeysTranslatable[$key][$langcode] = $this->values[$field_name][$langcode];
+              }
             }
           }
           else {
-            $this->entityKeys[$key] = $this->values[$field_name][LanguageInterface::LANGCODE_DEFAULT];
+            if (isset($this->values[$field_name][LanguageInterface::LANGCODE_DEFAULT][0]['value'])) {
+              $this->entityKeys[$key] = $this->values[$field_name][LanguageInterface::LANGCODE_DEFAULT][0]['value'];
+            }
+            else {
+              $this->entityKeys[$key] = $this->values[$field_name][LanguageInterface::LANGCODE_DEFAULT];
+            }
           }
         }
       }
@@ -409,6 +429,9 @@ public function set($name, $value, $notify = TRUE) {

@@ -409,6 +429,9 @@ public function set($name, $value, $notify = TRUE) {
     // notified to handle changes afterwards. We can ignore notify as there is
     // no parent to notify anyway.
     $this->get($name)->setValue($value, TRUE);
+    if (isset($this->entityKeysTranslatable[$name][$this->activeLangcode])) {
+      unset($this->entityKeysTranslatable[$name][$this->activeLangcode]);
+    }

Like somewhere here maybe, see previous commet ...

I asked @plach to have a look at the onChange issue.

Also:

<catch> stefan_r: I wonder a bit if we really need to check if it's translatable.
<catch> stefan_r: rather than just loop over the field values and figure out either way.

updating summary

I recently saw the following in ContentEntityBase::initializeTranslation():

    // Reset language-dependent properties.
    unset($translation->entityKeys['label']);

So either that code does not actually work or we are thinking too complicated here :-)

This fixes the issue reported in #22.

+++ b/core/modules/node/src/Tests/NodeTranslationUITest.php
@@ -58,6 +59,41 @@ function testTranslationUI() {
+  function testTranslationStatusChange() {

We already have NodeTranslationUITest::doTestPublishedStatus() that's supposed to test this, we should update that one instead.

Thanks, that fix makes sense. I think we can remove the snippet in this interdiff as well then.

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -682,7 +705,7 @@ protected function initializeTranslation($langcode) {
        // Reset language-dependent properties.
   -    unset($translation->entityKeys['label']);
   +    unset($translation->entityKeysTranslatable['label']);
   

   Should we really be hardcoding the label property here, what if we have more language-dependent properties than just the label?

2. +++ b/core/modules/node/src/NodeForm.php
   @@ -232,6 +254,7 @@ protected function actions(array $form, FormStateInterface $form_state) {
   +      $element['publish']['#published'] = TRUE;
   
   @@ -240,10 +263,10 @@ protected function actions(array $form, FormStateInterface $form_state) {
   +      $element['unpublish']['#published'] = FALSE;
   

   Maybe we can rename this to something like #set_published_status to prevent confusion?

Actually $this->activeLanguage shouldn't change in the lifetime of an object, meaning there should be no need to put the translatable values on a separate $entityKeysTranslatable variable, we can just store whatever the value for the active language is.

$this->activeLanguage has nothing to do with the current language of the site or the container. It definitely does change when you call getTranslation() (what happens is that the object is cloned and $this->activeLanguage is changed.

But @tsteockler has a good point.. we need to either unset them all in __clone() or key by active langcode, not both. Not quite sure yet what's better, keying by language might be better, because it's quit likely that sites will load the entity and then switch the language, this improvement won't work well on multilingual sites otherwise.

OK let's go back to the previous approach then, I must've misunderstood on IRC :)

That was an attempt at doing away with the isTranslatable check for performance reasons but I think it'd be cleaner if we had this information somehow, this patch feels a bit hacky.

5:00 PM <catch> stefan_r: one thing we discussed was making that also an entity key, but would need input from plach.
5:01 PM <catch> stefan_r: or not an entity key but moving it into the entity definition itself - would have to override in code to change it.

Not sure what I was supposed to validate, I reviewed the whole patch, just in case. It looks good to me, the only things that I found are:

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -991,18 +1000,28 @@ public function referencedEntities() {
   +        if (!$definition->isTranslatable() && isset($this->entityKeys[$key][LanguageInterface::LANGCODE_DEFAULT])) {
   

   We can swap these two tests if we want improve performance a bit.

2. +++ b/core/modules/node/src/NodeForm.php
   @@ -204,10 +203,33 @@ public function form(array $form, FormStateInterface $form_state) {
   +    if (isset($element['#set_published_status'])) {
   +      $node->setPublished($element['#set_published_status']);
   +    }
   +  }
   
   @@ -232,6 +254,7 @@ protected function actions(array $form, FormStateInterface $form_state) {
   +      $element['publish']['#set_published_status'] = TRUE;
   
   @@ -240,10 +263,10 @@ protected function actions(array $form, FormStateInterface $form_state) {
   +      $element['unpublish']['#set_published_status'] = FALSE;
   

   Not sure why this was changed from #published to #set_published_status: it doesn't look like an a DX improvement to me. What about just #published_status?

Btw, it would be nice if someone could add me to the contributors list, since I wrote parts of the patch :)

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -166,13 +166,19 @@ public function __construct(array $values, $entity_type, $bundle = FALSE, $trans
   +            if (is_array($this->values[$field_name][$langcode])) {
   +              if (isset($this->values[$field_name][$langcode][0]['value'])) {
   +                $this->entityKeys[$key][$langcode] = $this->values[$field_name][$langcode][0]['value'];
   

   The owner is an entity reference field, so this won't work for them.

   We need a second case here for target_id.

2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -555,8 +561,13 @@ public function onChange($name) {
   +        if (isset($this->entityKeys[$key][LanguageInterface::LANGCODE_DEFAULT])) {
   +          unset($this->entityKeys[$key][LanguageInterface::LANGCODE_DEFAULT]);
   +        }
   +        if (isset($this->entityKeys[$key][$this->activeLangcode])) {
   +          unset($this->entityKeys[$key][$this->activeLangcode]);
   +        }
   

   We can use elseif here, for a tiny performance improvement, since we know that we don't have both.. I think?

3. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -991,18 +1000,28 @@ public function referencedEntities() {
   +    if (!isset($this->entityKeys[$key][$this->activeLangcode])) {
   +      if ($key == 'bundle') {
   +        $this->entityKeys[$key][$this->activeLangcode] = $this->entityKeys[$key][LanguageInterface::LANGCODE_DEFAULT];
   +      }
   +      elseif ($this->getEntityType()->hasKey($key)) {
   

   A bit confused by the logic here.. did the plan with two different properties not work? Then we wouldn't have to hardcode bundle here.

   I have a feeling that the current code isn't really working for id/revision ID and so on.

Working a bit on this.

1. actually works for owner because it's a base field and not inside 'value'.

Confirmed that on multilingual sites, we still have to get the field definitions to access the value, even for things like the id. That's not so nice and a bit a regression.

So let's try something like this, only did manual tests.

With this patch, I can *almost* render a node (without comments) without getting the field definitions, the only thing that breaks it is NodeViewBuilder::getBuildDefaults(), the check for in_preview. Wondering if we can solve that in a better way.

#2507455: TypedData::setValue() with $notify set to TRUE on a get request causes unnecessary onChange notification is also relevant, because currently, setting _attributes on e.g. the title unsets the entity key and it needs to be refetched. Doesn't make a big difference really because it only happens on a render cache miss which means that we had to instantiate the title field anyway to render it.

i don't like that there's so much code in the constructor, I'm thinking about a helper method or something there to avoid duplicating it.

No idea how that test works right now, but the current default language in the language manager is en, not de. Rebuilding solves that. Also fixed a bug in setDefaultLanguage() that cased one of the two test fails there.

That might not be 100% correct yet for untranslatable entity types with a langcode field.

By documenting is_preview as a public property, which is not nice but just reflects reality, we can avoid the __isset() and, at least without comment module, view node/N without having to load the field definitions at all.

Also did some profiling with a normal authenticated user (user 1 is not useful as that one bypasses the access checks), with standard but without comment fields to get the best possible improvement.

Total wall time varies too much for me (I'm seeing anything between 66-120ms), but we're saving 800 function calls (2.4%) then and 1.2MB memory (7%).

Node::access() is 2.2ms (57%) faster, no calls to typed data manager or getFieldDefinitions() left. That also means that we load quite a bunch of classes less.

@Berdir I had gotten rid of the two different properties in an attempt to have less calls to isTranslatable() and not having to load field definitions as suggested earlier by catch. But it doesn't seem like something we can easily get around, so #62 makes more sense than #55

@plach as to #published vs #set_published_status, I got confused myself when reading the code, I assumed "published" referred to the current state. If it's not an improvement let's put it back to whatever it was and just put a comment to clarify. Or maybe instead of a boolean it could be a called 'publish' or 'unpublish'.

+++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
@@ -138,13 +138,20 @@
+   * Holds untranslatable entity keys like the ID, bundle and revision ID, keyed by langcode.

+   * Holds translatable entity keys like the ID, bundle and revision ID .

That doesn't seem right :)

+++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
@@ -555,8 +583,13 @@ public function onChange($name) {
+      if ($key != 'bundle') {

Do we still need to hardcode this?

Just out of interest where does the 14 ms increase in Wall time come from in that first run?

Pretty sure that's just the normal variation, in my case, I've seen anything between 66ms and 120ms with both patches, I just picked two that were close together.

If you want to get better results, you could try to set up and run https://github.com/LionsAd/xhprof-kit, that's going to do lots of requests and then it takes the average to compare it.

Yes, when I do 100 runs the Wall time decreases rather than increases. On a regular node page:

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=5589b6f75b296&...

This already has test coverage, right?

Looks good.

Proposed commit message:

Issue #2498919 by stefan.r, Berdir, catch, plach: Node::isPublished() and Node:getOwnerId() are expensive

Does this need a change record, btw.?

re-roll

Re-roll looks great!

Well the translatableEntityKeys property just an added internal property and it doesn't affect the actual API, so I would assume not.

Don't know if we need one for the added entity keys defined on the node either (uid, status) as those only ought to be accessed through isPublished() / getOwnerId().

A little change crept into the re-roll, sorry.

Doh, yes :)

I'm posting here so that those involved in this issue might be able to lend a hand in resolving what appears to be a regression in the node preview feature as a result of this fix.

#2535792: Preview feature broken if requested twice

If any of you who contributed here could take a quick glance at the problem and it might be apparent to one of you what the solution might be.

This should probably have had an upgrade path since it changed the schema on uid and status fields. Discovered this over in #2507201: Upgrade path for MySQL switch to multibyte UTF8 which adds node and term data to the starting db for update tests.