BUGFIX: Improve FusionView slow queries for current document and site #5597
Conversation
…cument The closest document check (which is often a slow query) can be skipped in this case.
Sebobo
changed the title
|
|
||
| $this->setFallbackRuleFromDimension($currentNode->dimensionSpacePoint); | ||
|
|
||
| $this->nodeTypeManager = $this->contentRepositoryRegistry->get($subgraph->getContentRepositoryId())->getNodeTypeManager(); |
There was a problem hiding this comment.
It seems a bit weird to me that we instantiate the NTM in the render() method.
That makes the behavior a bit fragile because it will throw when accessed before the render method was called once
There was a problem hiding this comment.
I simplified the code again and get the NTM when we need it.
| if (!$currentNode instanceof Node) { | ||
| throw new Exception('FusionView needs a variable \'site\' set with a Node object.', 1538996432); | ||
| $currentSiteNode = $this->variables['site'] ?? null; | ||
| if (!$currentSiteNode instanceof Node) { |
There was a problem hiding this comment.
So previously this case failed and now we try to fetch the site?
Wouldn't this disguise potential bugs? And is that related to the performance optimization?
There was a problem hiding this comment.
This is now the 8.3 behaviour again.
8.3 looked for the site in the context if it wasn't defined by the controller.
There was a problem hiding this comment.
The reason why it affects performance is, that the method is called at least twice.
We use the existing method to get the site node and fall back to the query if necessary.
Sebobo
force-pushed
the
bugfix/fusionview-slowqueries
branch
from
c5b1f01 to
a85b10a
Compare
There was a problem hiding this comment.
the fusion view and node controller relation ship is super dirty and this pr does not make it nicer :D ... but because its so dirty we seem to have lost sight where we query information as we do things needlessly again so thanks for clearing this up.
(For anyone whose eyes cry when skimming to the view i can only recommend looking at an aprroach where there is no view :D much nicer - but nothing for a bugfix https://codeberg.org/PackageFactory/neos-component-view/src/commit/059f0bfa0ab207553a22603fa126fba7762e7aa0/Classes/ComponentEngineNodeController.php#L112 CPX)
| $nodeTypeManager = $this->contentRepositoryRegistry->get($node->contentRepositoryId)->getNodeTypeManager(); | ||
|
|
||
| // Skip expensive subgraph lookup if the node is already a document node | ||
| if ($nodeTypeManager->getNodeType($node->nodeTypeName)?->isOfType(NodeTypeNameFactory::NAME_DOCUMENT)) { |
There was a problem hiding this comment.
ideally we should not need the node type manager here because if the view is used in the controller where we only route to documents we can trust the input and not be paranoid.
(asking things the node type manager loads and initialises the configuration which is also work which is ideally not part of the rendering?)
There was a problem hiding this comment.
Yes as with all those variables, the controller should supply them and the view should complain if not. Though I think in this case the Neos FusionView accepted any kind of node so far.
I would like to fix this for 9.x if we agree on the breakinesslevel.
3 participants
By reusing already existing information we can skip 2 unnecessary subgraph ancestry queries, which are marked as slow queries.
Both changes are reimplementations from Neos 8.3.
In my test this improved the loading time in the Neos.Demo by 20ms (70ms -> 50ms for a blank page).