Creating new user entities for anonymous users is very slow

Based on new relic reports, on cold caches, I'm seeing up to 50 calls to Drupal\Core\Entity\ContentEntityStorageBase::doCreate(), that takes 250ms.

Does that mean a User::load needs 5ms?

+++ b/core/modules/node/src/NodeViewBuilder.php
@@ -26,8 +27,15 @@ public function buildComponents(array &$build, array $entities, array $displays,
+       User::loadMultiple($uids);

3 instead of 2 spaces.

#1 doesn't yet remove the function.

I wonder why this is actually necessary at all? We're only doing it for Node entities. Why do other entity types don't need this?

+++ b/core/modules/node/src/NodeViewBuilder.php
@@ -26,8 +27,15 @@ public function buildComponents(array &$build, array $entities, array $displays,
-    // Attach user account.
-    user_attach_accounts($build, $entities);

Should we remove this dangerous helper then?

So user_attach_accounts() also also doing the preloading, why wan't we rewrite in a way that it doens't create the anonymous account unless needed?

Why don't we create the anonymous user account once and save it as before?

Yes you could delete uid 0 in previous versions, but we could just re-create it if its missing:

User::loadAnonymous() --> lazily creates the account and saves it in a locked state

Edit:

Or maybe not even saved to the database, but just created once per request, which takes 5 ms, which might be okay I think.

[…] just created once per request, which takes 5 ms, which might be okay I think.

"Just 5 ms per request" -> *gasp*! Are you sure you're @dawehner and not some impersonator? :P

I love the idea to simplify stuff and put the logic onto the user itself. Its a special factory of a user object, so putting that onto the user object is a good place.

100% agreed with #15!

This patch looks great to me, but this requires somebody with more Entity/Field/Typed Data API knowledge to RTBC.

+++ b/core/modules/comment/src/Entity/Comment.php
@@ -534,7 +535,11 @@ public static function preCreate(EntityStorageInterface $storage, array &$values
+      $entity = User::createAnonymous(array('uid' => 0, 'name' => $this->getAuthorName(), 'homepage' => $this->getHomepage()));

I think the only weird part is having to specify uid => 0 here. Why not put that in createAnonymous?

+1 to RTBC, but I feel yched would be good to review this.

Great to see clean-ups and performance improvements at the same time like this. Here a review:

1. +++ b/core/modules/comment/src/Entity/Comment.php
   @@ -534,7 +535,11 @@ public static function preCreate(EntityStorageInterface $storage, array &$values
   +    if (!$entity || $entity->isAnonymous()) {
   

   If the referenced entity is already anonymous, why would we create another one?

2. +++ b/core/modules/user/src/Entity/User.php
   @@ -430,6 +430,31 @@ public function getChangedTime() {
   +  public static function createAnonymous(array $values = array()) {
   

   Big +1 for having that method, but I don't like the name as it unfortunately collides with the create semantics of entities. Afair the factory patch would try to differentiate between create() and createInstance(), so maybe createAnonymousInstance(). Anyway, the previous getAnonymous() would be way better. Can we fix the twig problem somehow instead?

3. +++ b/core/modules/user/src/Entity/User.php
   @@ -430,6 +430,31 @@ public function getChangedTime() {
   +    // Values might not map to fields, so we can't reliable pass them into the
   +    // class. Anonymous users always have uid 0.
   

   Shouldn't we instead document $values to map to fields only - what's the point in passing in something else? This would be faster also as it saves us from instantiating field objects if they are not used later on.

I think this looks very good now :)

+++ b/core/modules/comment/comment.module
@@ -632,31 +632,6 @@ function comment_preprocess_block(&$variables) {
-function comment_prepare_author(CommentInterface $comment) {
...
-    $account->enforceIsNew(FALSE);

+++ b/core/modules/comment/src/Entity/Comment.php
@@ -534,7 +535,11 @@ public static function preCreate(EntityStorageInterface $storage, array &$values
   public function getOwner() {
...
+      $entity = User::createAnonymous(array('name' => $this->getAuthorName(), 'homepage' => $this->getHomepage()));

+++ b/core/modules/user/src/Entity/User.php
@@ -430,6 +430,31 @@ public function getChangedTime() {
+  public static function createAnonymous(array $values = array()) {
...
+    $values = array('uid' => 0) + $values;

+1 to removal, the only question about enforceIsNew(FALSE) - it was here for some reason, maybe just not allowing to save the author entity when comment saved

1. +++ b/core/modules/comment/src/Entity/Comment.php
   @@ -534,7 +535,11 @@ public static function preCreate(EntityStorageInterface $storage, array &$values
      public function getOwner() {
   -    return $this->get('uid')->entity;
   +    $entity = $this->get('uid')->entity;
   +    if (!$entity || $entity->isAnonymous()) {
   +      $entity = User::getAnonymousUser(array('name' => $this->getAuthorName(), 'homepage' => $this->getHomepage()));
   +    }
   +    return $entity;
      }
   

   Nitpick for clarity : s/$entity/$user ?

2. +++ b/core/modules/user/src/Plugin/Field/FieldFormatter/AuthorFormatter.php
   @@ -23,7 +23,7 @@
   +class AuthorFormatter extends EntityReferenceFormatterBase {
   

   Makes sense, but then the viewElements() method should use $this->getEntitiestoView(), like other ER formatters ?

3. +++ b/core/modules/user/src/Entity/User.php
   @@ -430,6 +430,32 @@ public static function getAnonymousUser() {
   +    // Values might not map to fields, so we can't reliable pass them into the
   +    // class. Anonymous users always have uid 0.
   +    $values = array('uid' => 0) + $values;
   +    $entity = new $class(array(), $entity_type->id());
   +    foreach ($values as $name => $value) {
   +      $entity->$name = $value;
   +    }
   

   Not sure I get why "values might not map to fields", what's special about the anon user regarding values and fields ?
   I'm not sure the generic User::getAnonymousUser($values) method should behave differently than User::__construct($values) regarding what happens with $values ?

   If that's because Comment.php uses that to assign an adhoc, non-field 'homepage' runtime property, then arguably that belongs to Comment.php ? Like so :

       if (!$entity || $entity->isAnonymous()) {
         $entity = User::getAnonymousUser(['name' => $this->getAuthorName()]));
       , $entity->homepage = $this->getHomepage();
       }
   

   Pushing it one step further : "make an anonymous User with tailor-made 'name' and 'homepage' properties that come from 3rd party data (here, data stored in the comment)" feels a bit weird, User shouldn't have to cater for special cases like this. As far as user.module is concerned, there is only *one* version of "the anon user", right ?

   Then shouldn't it be :
   - User::getAnonymousUser() : no param, the anon user is always the same, the method creates the object once and statically caches it for subsequent calls
   - Comment::getOwner() does :

   $user = clone User::getAnonymousUser();
   // Assign values stored in the comment.
   $user->name = $this->getAuthorName();
   $user->homepage = $this->getHomepage();

Back to NR for now ?

Looks like a great solution!

RTBC + 1

Prioritized changes are performance for beta evaluation.

@Berdir: yay, thanks !

This issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 70ea0f4 and pushed to 8.0.x. Thanks!

That comes a little late :-/, but I was thinking : maybe it's not too safe to trust the caller of getAnonymousUser() to clone the object in case it's going to modify it. If he fails to do that, he then pollutes the static object everyone else uses ?

Maybe getAnonymousUser() should always return a clone of its internal the static object ?

#42: Yes, indeed. We should definitely return a clone instead directly.

Follow-up?

Let's handle #42 in this issue. Reverted.

Each anonymous comment author could have different name

PS: profiling of the function (node with 273 comments) - with reverted patch 14.74%

@Berdir I wanna show that patch shaves like 7% on rendering of ~300 comments
And +1 to #42

RTBC, It is safer to return a clone for that common shared object.

I agree with @Fabianx #51 is safer. Thanks @Berdir.

This issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 3ab0ef9 and pushed to 8.0.x. Thanks!