In #1744302-11: [meta] Resolve known performance regressions in Drupal 8, I found a huge performance regression from 7.x to 8.x for the front page with no content. Per the following comment in that issue, a large portion of it is related to EntityNG. This surprised me, because at this time, only nodes and comments are converted to EntityNG, and a front page with no content has neither.

Turns out however, that _node_add_access() gets called twice: once for checking access to the node/add link that's part of the Tools menu block, and once by the Views ListingEmpty plugin. Since there are 2 node types ('article' and 'page'), this results in 4 calls to node_access('create', $node_type). In Drupal 7, this access was evaluated without creating a node entity object. But in Drupal 8, it results in 4 calls to entity_create('node'), so that access checkers can always operate on entities rather than sometimes on entities and sometimes on type strings.

At the same time, EntityNG instantiation in general, and nodes in particular, is very expensive, especially the first time in a request. Here's a breakdown:

  • Time to require_once the 54 classes listed in the attached txt file: 6ms. Note, that's with APC and without using any autoloader at all: just iterating that array and calling require_once on a full path on my hard drive. Maybe my MAMP computer has worse file I/O performance than a production server though.
  • Time to call typed_data()->getDefinitions(); exclusive of class loading: 1.5ms. Once called once, it's then statically cached, so this is isolated from the following items.
  • Time to call drupal_container()->get('plugin.manager.entity')->getStorageController('node')->getFieldDefinitions(array('EntityType' => 'node', 'Bundle' => 'page')); exclusive of the above items: 20ms. Again, once called once, it's then statically cached, so this is isolated from the following items.
  • Time to then call entity_create('node', array('type' => 'page')); the first time: 7ms.
  • Time for each additional entity_create('node', array('type' => 'page'));: 2.5ms. The difference between this and the first time is almost entirely due to the $this->prototypes cache in TypedDataManager::getPropertyInstance().

Some thoughts about this:

  • A total of 35ms to load/create the first entity in a request is pretty outrageous. I think we need to speed that up. We have #1762258: Benchmark: Bypass magic to speed up the new entity field API open though the patch there is focusing on some other areas than the above.
  • Maybe we should change node_access() to not create a $node for the sole purpose of checking creation access on a type? However, while that would speed up pages that don't otherwise instantiate any entities, how impactful in the real world would that be? In the real world, most requests need to instantiate an entity at some point anyway.
  • Even the 2.5ms for each additional node instance, once a lot of the typed data info is statically cached, is still a lot (~10x) larger than non-EntityNG entities (I tested a taxonomy_term, which is not yet NG, and found ~0.2ms for that). A lot of that is due to the magic getters and setters which are known to be about 30x slower than direct property access.

Given that we have other Entity Field API performance issues in the queue, I'm not yet sure what we should make the scope of this one. Thoughts?

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir’s picture

About getFieldDefinitions(), see #1975486: Add a persistent cache in DatabaseStorageController::getFieldDefinitions(). That should help a lot with that :)

Berdir’s picture

Also, the problem with access checks is that we now have an interface and that interface expects an EntityInterface.

The problem is that entity_create() does a lot of things that we are not really interested in. It's specifically for creating a new entity with proper field/language defaults, invokes hooks and so on.

What we are missing is a way to create a minimal entity object that just has the bundle information and nothing else. Not yet sure how to do that. But as a start, let's remove node_access() so that we have more control on what we call ->access() exactly: #1947880: Replace node_access() by $entity->access(). The performance problem is also discussed a bit there.

msonnabaum’s picture

Yes, the entity_create in node_access is the first thing to tackle here. I found it taking ~35ms on a default install for the front page, which as Alex points out, is kind of ridiculous.

It would be nice if node_access didn't have to call entity_create, but even so, most of this time was just spent in EntityNG's magic functions, so I feel like this could easily pop up again.

I find it very hard to figure out why most of the code that's executing is executing, so I'm totally relying on you guys here.

fago’s picture

The problem is that entity_create() does a lot of things that we are not really interested in. It's specifically for creating a new entity with proper field/language defaults, invokes hooks and so on.

Yep, I think we could save quite some time if we'd migrate to a factory here. As not applying default values also means not instantiating lots of objects.

msonnabaum’s picture

Status: Active » Needs review
FileSize
2.41 KB
94.51 KB

Looking at node_access, it strikes me that we created this problem by treating two different operations as if they were the same.

Checking create access is a very different operation than view/delete/update. The only context it needs is the type and the user.

How about an approach in the attached patch which makes this more explicit?

The savings is around 10ms when called only once, which isn't huge, but I think this is an overall code improvement so it's worth doing.

node-access-create-xhprof.png

Status: Needs review » Needs work

The last submitted patch, node-create-access.patch, failed testing.

msonnabaum’s picture

Status: Needs work » Needs review
msonnabaum’s picture

Issue tags: -Performance, -Entity Field API

#5: node-create-access.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Performance, +Entity Field API

The last submitted patch, node-create-access.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
948 bytes
1.98 KB

This seemed to work for me. Could probably use more improvements.

Status: Needs review » Needs work

The last submitted patch, node-1979094-10.patch, failed testing.

effulgentsia’s picture

I haven't reviewed this in detail to see if it fits in correctly with Drupal's entity access code in general, but just wanted to leave a quick comment saying it makes a lot of sense to me to have a separate method and signature for creation access, so +1 to the concept.

Berdir’s picture

Yes, we talked about this before, I was suggesting something like this too, but that was when we still had a method for every $op (which still always expected $entity).

The problem that we had is that we then need to limit what we support for create.

Needs to be the generic implementation, though and provide the same flexibility as the other operations, there are a lot of use cases to alter this.

msonnabaum’s picture

Status: Needs work » Needs review
FileSize
4.66 KB
3.04 KB

Added back the alter for create access, which seemed to be why the translation test was failing, but it's still failing for some reason. I'm not quite sure why.

Status: Needs review » Needs work

The last submitted patch, node-1979094-14.patch, failed testing.

fago’s picture

The only context it needs is the type and the user.

hm, while it sucks that the current approach is slow, it has the benefit of not hard-coding type and user. You can alter create access based on whatever entity field you want, e.g. some group membership.

So instead of using entity_create() we could go with our not-yet existing entity factory (new $class for now) and just pass along the minimal values. That saves us from applying default-values on all entity fields, i.e. instantiating all of them. Not sure whether this brings enough of a performance improvement, I guess we'd have to check.

msonnabaum’s picture

Status: Needs work » Needs review
FileSize
2.11 KB
5.79 KB

Perhaps the approach here solves the issues you have?

If there are use cases for passing additional context to the access create hook, that's reasonable, but I don't see how that context being fields on a temporary entity makes sense.

As evidenced by node_access's ambiguous function signature, the "create" op has different requirements. Why not make those requirements explicit rather than create the need for a temporary entity in support of a leaky abstraction?

Status: Needs review » Needs work

The last submitted patch, node-1979094-17.patch, failed testing.

fago’s picture

+++ b/core/modules/node/lib/Drupal/node/NodeAccessController.php
@@ -32,6 +32,41 @@ public function access(EntityInterface $entity, $operation, $langcode = Language
+  public function createAccess($type, AccountInterface $account = NULL, $context = array()) {

Yep, that looks reasonable. It makes explicit what we usually have + enables modules to provide/use additional context!

I'd assume that's how entity-create-access would work then generally?

dawehner’s picture

FileSize
12.41 KB

Just linking the issue for the access checker: #2028585: Replace entity_page_create_access by an access controller

It seems to be that there should be a really similar pattern to the way how checkAccess works, so we can cache the actual results.

This adds generic functionality to the base entity access controller.

Berdir’s picture

Status: Needs work » Needs review
Issue tags: +Entity Access
+++ b/core/lib/Drupal/Core/Entity/EntityAccessControllerInterface.phpundefined
@@ -36,6 +36,19 @@
+   * @param string $type
+   *   The node type to check access for.
...
+  public function createAccess($type, AccountInterface $account = NULL, $context = array());

node type => entity_bundle, it should also be optional.

Status: Needs review » Needs work

The last submitted patch, drupal-1979094-20.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
12.4 KB

Rerolled.

dawehner’s picture

FileSize
3.52 KB
12.51 KB

I tried to figure out whether the default values should be '' or NULL.

Status: Needs review » Needs work

The last submitted patch, drupal-1979094-24.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
2.8 KB
14.18 KB

I don't think we have to still load the full user.

Status: Needs review » Needs work

The last submitted patch, drupal-1979094-26.patch, failed testing.

Berdir’s picture

+++ b/core/lib/Drupal/Core/Entity/EntityAccessController.phpundefined
@@ -23,12 +23,25 @@ class EntityAccessController implements EntityAccessControllerInterface {
-    if (!$account) {
-      $account = $GLOBALS['user'];
-    }
+    $this->prepareUser($account);

The function returns the account, this doesn't work :)

+++ b/core/lib/Drupal/Core/Entity/EntityAccessController.phpundefined
@@ -45,17 +58,35 @@ public function access(EntityInterface $entity, $operation, $langcode = Language
+      // No result from hook, so entity checks are done.
+      $return = (bool) $this->checkAccess($entity, $operation, $langcode, $account);

I am not sure I understand the comment here.

+++ b/core/lib/Drupal/Core/Entity/EntityAccessController.phpundefined
@@ -45,17 +58,35 @@ public function access(EntityInterface $entity, $operation, $langcode = Language
+   * - At least one module says to grant access.
+   */
+  protected function processAccessHookResults($access) {

Missing @param for $access and @return.

+++ b/core/lib/Drupal/Core/Entity/EntityAccessController.phpundefined
@@ -45,17 +58,35 @@ public function access(EntityInterface $entity, $operation, $langcode = Language
+
+  protected function prepareUser($account) {

Should probably be AccountInterface $account = NULL too. And docs ;)

+++ b/core/lib/Drupal/Core/Entity/EntityAccessController.phpundefined
@@ -85,8 +116,8 @@ protected function checkAccess(EntityInterface $entity, $operation, $langcode, A
-   * @param \Drupal\Core\Entity\EntityInterface $entity
-   *   The entity for which to check 'create' access.
+   * @param \Drupal\Core\Entity\EntityInterface|string $entity
+   *   The entity for which to check access or a custom string.

This is a bit strange...

Should probably be changed to always pass in a "unique string identifier for the entity/operation" or so.

+++ b/core/lib/Drupal/Core/Entity/EntityAccessController.phpundefined
@@ -143,4 +191,55 @@ public function resetCache() {
+  public function createAccess($entity_bundle = '', AccountInterface $account = NULL, $context = array()) {

I think NULL is a better default value. the bundle isn't an empty string, it's not defined.

+++ b/core/lib/Drupal/Core/Entity/EntityAccessController.phpundefined
@@ -143,4 +191,55 @@ public function resetCache() {
+    if (($access = $this->getCache('create', 'create', $context['langcode'], $account)) !== NULL) {

The identifying string should use the bundle if present.

+++ b/core/lib/Drupal/Core/Entity/EntityAccessController.phpundefined
@@ -143,4 +191,55 @@ public function resetCache() {
+    $access = module_invoke_all($this->entity_type . '_create_access', $account, $context['langcode']);

Hook needs to be documented. Which is tricky as it's dynamic...

+++ b/core/lib/Drupal/Core/Entity/EntityAccessController.phpundefined
@@ -143,4 +191,55 @@ public function resetCache() {
+      // No result from hook, so entity checks are done.
+      $return = (bool) $this->checkCreateAccess($access, $context);

A I think I get the comment now. This should probably use "so entity checks are executed". done could also mean they already happened.. which is not the case.

+++ b/core/lib/Drupal/Core/Entity/EntityAccessController.phpundefined
@@ -143,4 +191,55 @@ public function resetCache() {
+  protected function checkCreateAccess(AccountInterface $account, array $context) {

This needs the bundle.

+++ b/core/lib/Drupal/Core/Entity/EntityAccessControllerInterface.phpundefined
@@ -36,6 +36,19 @@
+   * @param string $entity_bundle
+   *   (optional) The bundle to check access for. Defaults to an empty string.

This should probably use the same documentation as entity_page_create_access(), notably the "required if type uses bundles part" as it gets weird when you start to mix it for the same entity type.

+++ b/core/modules/node/lib/Drupal/node/NodeAccessController.phpundefined
@@ -32,6 +32,41 @@ public function access(EntityInterface $entity, $operation, $langcode = Language
+   * @param string $entity_bundle
+   *   The node type to check access for.
+   * @param \Drupal\Core\Session\AccountInterface $account
+   *   (optional) The user session for which to check access, or NULL to check
+   *   access for the current user. Defaults to NULL.
+   * @param array $context
+   *   An array of key-value pairs to pass additional context when needed.

@inheritdoc.

+++ b/core/modules/node/lib/Drupal/node/NodeAccessController.phpundefined
@@ -32,6 +32,41 @@ public function access(EntityInterface $entity, $operation, $langcode = Language
+  public function createAccess($entity_bundle, AccountInterface $account = NULL, $context = array()) {

I don't think you can make it required here when it's not in the interface.

Not sure about others, but at least the taxonomy term access controller might need some changes and we should update access() to state that you should not use this for op create as we don't want to support two path. Alternatively, forward the call using $this->createAccess($entity->bundle()) in access() if it's op create.

dawehner’s picture

Status: Needs work » Needs review
FileSize
11.52 KB
15.65 KB

Hook needs to be documented. Which is tricky as it's dynamic...

I would have oriented upon hook_$type_access, but there is none for it.

Status: Needs review » Needs work

The last submitted patch, drupal-1979094-29.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
1.22 KB
15.66 KB

This will hopefully install.

Status: Needs review » Needs work

The last submitted patch, drupal-1979094-31.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
727 bytes
15.67 KB

Strange, we currently eat recoverable fatal errors and don't display errors at all for them but I guess the testbot doesn't like them.

Status: Needs review » Needs work

The last submitted patch, create-access--1979094-33.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
5.86 KB
15.96 KB

Fun debugging with operator precedence.. to me a while to figure that out at 2am in the morning :)

Status: Needs review » Needs work

The last submitted patch, create-access--1979094-35.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
15.85 KB
30.66 KB

This patch should fix the taxonomy fails, also updating all other access controllers, this shows the API change that comes with this.

It is one, but it's easy to update and it's a considerable performance improvement, especially on pages that would otherwise not have to load any NG entities.

Status: Needs review » Needs work

The last submitted patch, create-access--1979094-37.patch, failed testing.

catch’s picture

Category: task » bug
Berdir’s picture

Issue tags: +sprint

Tagging, want to push this further.

Berdir’s picture

Title: EntityNG instantiation is slow and maybe called needlessly in some places » Separate create access operation entity access controllers to avoid costly EntityNG instantiation
Status: Needs work » Needs review
FileSize
31.65 KB

The createAccess() call in Entity/NG was completely wrong..

Also, the node access controller constructor was all wrong ;)

Also clarifying title...

Berdir’s picture

And the interdiff.

dawehner’s picture

+++ b/core/lib/Drupal/Core/Entity/Entity.phpundefined
@@ -273,6 +273,11 @@ public function getIterator() {
   public function access($operation = 'view', AccountInterface $account = NULL) {
+    if ($operation == 'create') {
+      return \Drupal::entityManager()
+        ->getAccessController($this->entityType)
+        ->createAccess($this->bundle(), $account);
+    }

Is there a specific reason to not move this logic to the access controller directly?

+++ b/core/lib/Drupal/Core/Entity/EntityAccessController.phpundefined
@@ -23,14 +23,27 @@ class EntityAccessController implements EntityAccessControllerInterface {
+  /**
+   * @param string $entity_type
+   *   The entity type of the access controller instance.
+   */

Missing docs for "Constructs ..."

+++ b/core/lib/Drupal/Core/Entity/EntityAccessController.phpundefined
@@ -45,17 +58,36 @@ public function access(EntityInterface $entity, $operation, $langcode = Language
     $access = module_invoke_all($entity->entityType() . '_access', $entity->getBCEntity(), $operation, $account, $langcode);

@@ -143,4 +175,75 @@ public function resetCache() {
+    $access = module_invoke_all($this->entity_type . '_create_access', $account, $context['langcode']);

Opened a follow up: #2041333: Inject the module handler into the entity access controller

+++ b/core/lib/Drupal/Core/Entity/EntityAccessController.phpundefined
@@ -100,13 +133,12 @@ protected function checkAccess(EntityInterface $entity, $operation, $langcode, A
+  protected function getCache($cid, $operation, $langcode, AccountInterface $account) {
     $uid = $account ? $account->id() : 0;

@@ -128,12 +161,11 @@ protected function getCache(EntityInterface $entity, $operation, $langcode, Acco
+  protected function setCache($access, $cid, $operation, $langcode, AccountInterface $account) {
     $uid = $account ? $account->id() : 0;

It seems to be that $account will always exist, so no need for this ternary

+++ b/core/lib/Drupal/Core/Entity/EntityAccessController.phpundefined
@@ -143,4 +175,75 @@ public function resetCache() {
+      $account = $GLOBALS['user'];

I guess we should pull the information from Drupal::request() now.

+++ b/core/lib/Drupal/Core/Entity/EntityNG.phpundefined
@@ -413,6 +413,11 @@ public function isEmpty() {
+    if ($operation == 'create') {
+      return \Drupal::entityManager()
+        ->getAccessController($this->entityType)
+        ->createAccess($this->bundle(), $account);

See above.

+++ b/core/modules/node/node.moduleundefined
@@ -2028,37 +2028,43 @@ function node_form_system_themes_admin_form_submit($form, &$form_state) {
+      if (module_exists('language')) {

Potentially module_exists could be also replaced by Drupal::moduleHandler()....

Status: Needs review » Needs work

The last submitted patch, create-access--1979094-41.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
6.74 KB
33.34 KB

Thanks for the review.

- I'd like to avoid having create as a valid operation for the access() method on access controllers. $entity->access('create') is also a pattern that's not (or should not be) used frequently, you should directly access createAccess() usually. Otherwise this issue is kinda pointless ;)
- We already replaced those module_invoke calls in #1953892: Move language node access logic into NodeAccessController
- Changed and inlined $account->id() in the cache methods
- "I guess we should pull the information from Drupal::request() now.". That's going to kill a ton of tests because there's no real request in tests at the moment. Let's not do that here.
- The module_invoke() in node_access() is just moved, the referenced issue above will remove it completely. Refactored this function a bit so that we don't have to put it into an else anymore and "move" it.

Updated the entity create access controllers to use createAccess(), that allows to simplify them quite a bit.

Status: Needs review » Needs work

The last submitted patch, create-access--1979094-45.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
1.58 KB
34.92 KB

Removing a bunch of code from the unit test ;)

dawehner’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Performance, -sprint, -Entity Field API

Thank you for explaining the bits. I agree we should totally separate these two bits as much as possible.

dawehner’s picture

arg

Berdir’s picture

#47: create-access--1979094-47.patch queued for re-testing.

tim.plunkett’s picture

As the one who mostly thought up the prepareEntityValues() being removed here, I'm +1. Thanks @Berdir, this is much better.

tim.plunkett’s picture

.

catch’s picture

Title: Separate create access operation entity access controllers to avoid costly EntityNG instantiation » Change notice: Separate create access operation entity access controllers to avoid costly EntityNG instantiation
Category: bug » task
Priority: Major » Critical
Status: Reviewed & tested by the community » Active

Looks much better. Couldn't find anything to complain about that wasn't already there.

Committed/pushed to 8.x, thanks!

Will need a small change notice/update.

Mile23’s picture

I'm tempted to say backport to D7, under Entity API (contrib). #1780646: entity_access() fails to check node type specific create access

Berdir’s picture

Title: Change notice: Separate create access operation entity access controllers to avoid costly EntityNG instantiation » Separate create access operation entity access controllers to avoid costly EntityNG instantiation
Priority: Critical » Major
Status: Active » Fixed
Issue tags: -sprint

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary: View changes

Updated issue summary.