-
-
Notifications
You must be signed in to change notification settings - Fork 232
BUGFIX: Improve FusionView slow queries for current document and site #5597
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 9.0
Are you sure you want to change the base?
Conversation
…cument The closest document check (which is often a slow query) can be skipped in this case.
$fusionRuntime = $this->getFusionRuntime($currentSiteNode); | ||
|
||
$this->setFallbackRuleFromDimension($currentNode->dimensionSpacePoint); | ||
|
||
$this->nodeTypeManager = $this->contentRepositoryRegistry->get($subgraph->getContentRepositoryId())->getNodeTypeManager(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
c5b1f01
to
a85b10a
Compare
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).