-
-
Notifications
You must be signed in to change notification settings - Fork 135
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
BUGFIX: Improve UI loading performance by removing UI nodedata script tag and using less data #3770
Conversation
I'm not sure how breaking this could be right now, therefore draft for now ;) @grebaldi I remember you started working on a replacement for the script tag, so your insights here would be very welcome. This doesn't need to be included in 8.3 but maybe we can :) |
Some test results with 82 nodes (haven't found a good way to measure time to interactive with the iframe): With Script Tag + with full PropertiesDocument size: 245KB / 14.6KB Without Script Tag + with MinimalProperties + ondemand node loadingDocument size: 198KB / 13.3KB (~80%) Without Script Tag + with MinimalProperties + Node batch loadingDocument size: 198KB / 13.3KB (~80%) |
* | ||
* @var Locale | ||
*/ | ||
protected $rememberedUserLocale; |
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.
Are those changes related?
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.
Not a 100%, just some optimisation as the locale is switched twice for every node triggered by the augmentation
I tested a variant of this change in my customers project and the document size was reduced from 1.8mb to 1.2mb. Also there are two child nodes queries for each node to get document and content child nodes. Maybe we also don't need those for the initial rendering with some adjustments. Right now it would break some parts of the ui. |
OK optimised a bit more and completely removed nodedata from the guest frame: Without NodeData in augmenterDocument size: 108KB / 7.5KB (~44%) When clicking through the demo site each page is about 50% quicker to load which actually feels much nicer. |
Great initiative, thanks for taking care. |
Yes of course. At the end we need the same amount of nodedata. But we kill the overhead of the function calls and document parsing. |
@bwaidelich I tested the loading performance a while yesterday with "Fast 3G" connection + Caching in my local Dev Context. Result: The page loads much faster, but if you try to immediately edit text in a CKEditor, the first click only selects the element instead of showing the caret to start editing. The second click then works. I want to test this on an actual remote server in Production Context to get a feeling for it. I also checked whether I can adjust the LoadingBar to wait for the GuestFrame to be initialised, but this is not a simple change right now. I would make sense to have a loading indicator of some kind for the guest initialisation too. This is already currently problem, when We currently also load some nodes twice as the structure tree needs them too. I want to check whether I can then skip some of them. |
Thanks for taking care of this. As a side effect, this will also help with some CSS selectors, as with the current approach selectors like |
Realised today this will also fix the issue neos/neos-development-collection#2988 with the Fusion Augmenter when augmenting a single content element. Previously it added another tag as it counted the element itself and the script tag instead of modifying the outside tag of the content element. |
The two main performance hogs which makes the async request for the nodedata slow is in It seems this was less of a problem in the previous version as during rendering the same first-level-node-cache was automatically used where possible. Right now I'm getting great results by optimising \Neos\Neos\Ui\ContentRepository\Service\NodeService::getNodeFromContextPath in skipping the creation of a completely new context with site and domain for each requested node. |
5242e1d
to
c439582
Compare
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.
@grebaldi I remember you started working on a replacement for the script tag, so your insights here would be very welcome.
My idea was to replace the content element wrapping technique we're using right now (HTML Augmenter) with an alternative approach using HTML comments instead. This would prevent collisions with userland markup and I also expect it to perform better, since HTML augmentation is quite slow.
That idea does not at all interfere with your change here. I actually thought we had rid ourselves of those script tags already 🙈
Thanks a lot for taking care of it! 👍
I've tested it locally (unthrottled), and I did notice some lag with the guest frame initialization (especially noticeable with lots of greyed-out not-inline-editable elements). But I think the trade-off is well worth it and we can optimize the initialization later still.
I've also re-run the CircleCI e2e tests, which were red due to connectivity issues. Everything's green now :)
c439582
to
701a047
Compare
Hi @grebaldi thx for the review! This morning while walking the 🦮 I thought about batch loading the nodes in the UI as a future optimisation. Meaning instead of loading all of them at once, We could load them in batches of 20(?) nodes or so and this way would make the nodes "above the fold" quicker accessible than the rest. |
@bwaidelich what do you think, should we merge this change and open a follow up to rework the ui loading state for 8.4 f.e.? |
I can't really give a qualified opinion (just wanted to chime in regarding potential pitfalls with n+1 requests) but my tendency would be to go the safer route (merge it into 8.4) |
Fyi for up merging it has to be adjusted in Neos 9 in Neos.Neos https://github.com/neos/neos-development-collection/blob/bf4df1ea7445e482df9c2e6a9c647915f6a33a53/Neos.Neos/Classes/Service/ContentElementWrappingService.php#L108 |
As we dont profit anymore from the fusion cache the ajax request might actually be slower. neos-ui/Resources/Private/Fusion/Prototypes/Page.fusion Lines 11 to 47 in cb5bccf
|
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.
Had one more look at the code
@mhsdesign are we finished here now? I merge the current 8.3 and the node updates on click disappeared again thx to Wilhelms fix. |
2d46ef2
to
2401318
Compare
@grebaldi @mhsdesign one thing this PR changes is that rendering the page content will not update the stored nodes anymore with the state from the rendered nodedata. It relies on the store which is mostly prefilled by the content tree before the content is rendered. There it could be that changes of another user will be rendered in the content, but will not update the stored nodes. Option 1: We could let the content tree always load nodes from the server when switching pages. Wdyt? |
I think Option 1 sounds quite fair :) Option 2 may save some HTTP requests, but I don't think it's worth the effort. To track content changes per document (via hash or timestamp), we would have to traverse each document's content tree anyway. In the best case we save an HTTP request, in the worst case, we do the traversal twice. |
…nodedata With this change the minimal required nodedata for each node in the rendered content is inserted as data attribute and not as inline script anymore. This improves performance as no extra function call is executed for each node. Additonally the reduction in rendered node attributes reduces the output filesize and again improves loading time. To prevent just-in-time loading of nodes all incomplete nodedata is requested after the guest frame has finished loading.
Co-authored-by: Marc Henry Schultz <[email protected]>
…r guest frame The content tree is loaded early and can load fully loaded nodes without being slower as the response time is the same. But this way we can skip lots of nodes from being loaded by the guest frame if they are already present in the store.
…use the store previously it was cumbersome to collect all the `nodes` that are really on the page. But it should make no difference to do the lookup on a bigger set.
…ry endpoint that means `neosUiDefaultNodes` only needs one param. Technically we use a `Set` most times when we invoke the `q` endpoint but seb convinced me that this filtering doesnt hurt and is probably more sane :D
This prevents showing outdated content, as the guest frame will not load new content by itself anymore.
2fd3193
to
f4520af
Compare
@grebaldi @mhsdesign I removed some code in the content tree document change saga and this way the content tree always reloads the nodes and this way we have up-to-date content. IMO we can merge then. |
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.
Thanks for all the effort
The `Classes/Aspects/AugmentationAspect.php` was adjusted via neos/neos-ui#3770 but in Neos 9.0 it resides here in Neos.Neos Thus we have to redo the adjustments. Fixes partially neos#4951 by removing illegal php dependencies
Remove deprecated interpretation of inline scripts (`data-neos-nodedata`) #3770
What I did
With this change the minimal required nodedata for each node in the rendered
content is inserted as data attribute and not as inline script anymore.
This improves performance as no extra function call is executed for each node.
Additonally the reduction in rendered node attributes reduces the output filesize and again improves loading time.
To prevent just-in-time loading of nodes all incomplete nodedata is requested after the guest frame has finished loading.
One big benefit of that is that the scripts tags won't interfere with JS plugins anymore and using the Fusion Augmenter will not wrap a single object with a ContentWrapping with another tag due to the script tag.
Fixes neos/neos-development-collection#2988
How to verify it
To test this change one can use the
bugfix/remove-ui-script-tag-compiled-artifacts
branch in composer:preview
document should be smaller (10-30%) than before and load quicker (10-40%)