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?
Comment | File | Size | Author |
---|---|---|---|
#47 | create-access--1979094-47.patch | 34.92 KB | Berdir |
#47 | create-access--1979094-47-interdiff.txt | 1.58 KB | Berdir |
#45 | create-access--1979094-45.patch | 33.34 KB | Berdir |
#45 | create-access--1979094-45-interdiff.txt | 6.74 KB | Berdir |
#42 | create-access--1979094-41-interdiff.txt | 2.92 KB | Berdir |
Comments
Comment #1
BerdirAbout getFieldDefinitions(), see #1975486: Add a persistent cache in DatabaseStorageController::getFieldDefinitions(). That should help a lot with that :)
Comment #2
BerdirAlso, 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.
Comment #3
msonnabaum CreditAttribution: msonnabaum commentedYes, 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.
Comment #4
fagoYep, 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.
Comment #5
msonnabaum CreditAttribution: msonnabaum commentedLooking 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.
Comment #7
msonnabaum CreditAttribution: msonnabaum commentedComment #8
msonnabaum CreditAttribution: msonnabaum commented#5: node-create-access.patch queued for re-testing.
Comment #10
tim.plunkettThis seemed to work for me. Could probably use more improvements.
Comment #12
effulgentsia CreditAttribution: effulgentsia commentedI 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.
Comment #13
BerdirYes, 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.
Comment #14
msonnabaum CreditAttribution: msonnabaum commentedAdded 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.
Comment #16
fagohm, 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.
Comment #17
msonnabaum CreditAttribution: msonnabaum commentedPerhaps 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?
Comment #19
fagoYep, 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?
Comment #20
dawehnerJust 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.
Comment #21
Berdirnode type => entity_bundle, it should also be optional.
Comment #23
dawehnerRerolled.
Comment #24
dawehnerI tried to figure out whether the default values should be '' or NULL.
Comment #26
dawehnerI don't think we have to still load the full user.
Comment #28
BerdirThe function returns the account, this doesn't work :)
I am not sure I understand the comment here.
Missing @param for $access and @return.
Should probably be AccountInterface $account = NULL too. And docs ;)
This is a bit strange...
Should probably be changed to always pass in a "unique string identifier for the entity/operation" or so.
I think NULL is a better default value. the bundle isn't an empty string, it's not defined.
The identifying string should use the bundle if present.
Hook needs to be documented. Which is tricky as it's dynamic...
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.
This needs the bundle.
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.
@inheritdoc.
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.
Comment #29
dawehnerI would have oriented upon hook_$type_access, but there is none for it.
Comment #31
BerdirThis will hopefully install.
Comment #33
BerdirStrange, we currently eat recoverable fatal errors and don't display errors at all for them but I guess the testbot doesn't like them.
Comment #35
BerdirFun debugging with operator precedence.. to me a while to figure that out at 2am in the morning :)
Comment #37
BerdirThis 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.
Comment #39
catchComment #40
BerdirTagging, want to push this further.
Comment #41
BerdirThe createAccess() call in Entity/NG was completely wrong..
Also, the node access controller constructor was all wrong ;)
Also clarifying title...
Comment #42
BerdirAnd the interdiff.
Comment #43
dawehnerIs there a specific reason to not move this logic to the access controller directly?
Missing docs for "Constructs ..."
Opened a follow up: #2041333: Inject the module handler into the entity access controller
It seems to be that $account will always exist, so no need for this ternary
I guess we should pull the information from Drupal::request() now.
See above.
Potentially module_exists could be also replaced by Drupal::moduleHandler()....
Comment #45
BerdirThanks 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.
Comment #47
BerdirRemoving a bunch of code from the unit test ;)
Comment #48
dawehnerThank you for explaining the bits. I agree we should totally separate these two bits as much as possible.
Comment #49
dawehnerarg
Comment #50
Berdir#47: create-access--1979094-47.patch queued for re-testing.
Comment #51
tim.plunkettAs the one who mostly thought up the prepareEntityValues() being removed here, I'm +1. Thanks @Berdir, this is much better.
Comment #52
tim.plunkett.
Comment #53
catchLooks 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.
Comment #54
Mile23I'm tempted to say backport to D7, under Entity API (contrib). #1780646: entity_access() fails to check node type specific create access
Comment #55
BerdirUpdated https://drupal.org/node/1862420/revisions/view/2665992/2769961.
Comment #56.0
(not verified) CreditAttribution: commentedUpdated issue summary.