Rendering of large widget structures / AlfList views can take a very long time about aikau HOT 16 CLOSED

OK. So this is all great stuff... in all honesty, performance has never been my major priority - it was always something I hoped we'd be able to get back to. However, my initial observation on reading this is .... wow, this is going to be a lot to absorb!

Naturally I'm also concerned about backwards compatibility - where you have entirely distinct widgets then we can easily absorb them as new modules without any impact, but where you're proposing changes to key mixin modules then we need to tread more carefully. Have you run the existing unit test suite against any of these changes? Or are these just focused on the use case that you've provided in your Gist? Can we look at breaking these down into smaller pull requests or do they need to be taken altogether?

The other obvious difficulty is in not actually being able to see the code changes that you've made. It might make sense for you to create a pull request of everything just so that we can review and experiment with it. Although we might not merge that request, we can use it as a means to establish how to move this forwards.

Hopefully you can understand the need to take our time over major changes to the internal codebase - it is also imperative that the Alfresco team understand the changes as we need to be able to support the changed code.

Having said all that - we definitely want to move forwards on this, assuming everything is as good as you say it is and doesn't cause any backwards compatibility issues. If you can raise the pull request so that we can start reviewing proposed changes then that would be a great place to start.

We're committed to making Aikau a proper open-source project and accepting contributions from the community though - and we're grateful that you've taken the time to make this effort.

from aikau.

The changes I made the last week and a half were completely distinct from the core widgets - I basically re-created the widgets and adapted the mixins via sub-classing (I didn't dare change anything in core that drastically). So the standard unit tests would not cover those. Our widgets also contain a few other feature additions / changes to the default widgets that they aren't really comparable anymore (i.e. we removed the Property tooltip handling in favor of standard DOM title attribute to reduce layout calculation overhead, extracted Property/InlineEditProperty/PropertyLink/Boolean features into three separate mixins ("RenderSupport", "LinkSupport", "EditSupport") and recombined those into a single, universal Property widget).

What I can definitely do (might get to it next weekend) is "liberate" our changes from the custom widgets / mixins and generalise them into the core widgets / mixins as a branch/PR to compare against. Then I can also run the existing unit tests...
My primary concern with this ticket was to raise the performance issues / concerns with you as early as possible.

from aikau.

OK, cool... I'm just very happy that you're looking to contribute back into Aikau. As I said, I always knew we'd be able to improve performance - but we needed to get other things in place first (breadth of widgets, testing, etc). Looking forward to seeing some code!

from aikau.

The branch Aikau-958 in my fork contains the "liberated" and generalised changes from our internal module that are sufficient to run the test case I used in this ticket (additional changes would be required to make some derived widgets work, i.e. InlineEditProperty).
As part of this I have created a "BaseWidget" module as a pendant to the BaseService, and introduced a _ConstructedWidgetMixin module for any widgets that do not use HTML templates. The CoreWidgetProcessing mixin has been enhanced with support of detached DOM initialisation of children which relies on the lifecycle functions provided by BaseWidget. This behaviour is automatically active when the widget that includes the mixin is a BaseWidget and attached to the live DOM, the widget is not a BaseWidget, or it is explicitly forced.
The WidgetInfo widget can now be suppressed for a specific widget and I have also included the relevant flag in the auto-inheritance of configuration properties if not set explicitly.

Somewhere during this "liberation", the improvements lost some of their effectiveness. Current profiling runs indicate that these changes only end up in a gain of around / below 25%, where in our module we achieved significantly better results (although we used a significantly more complex JSON page model with more complex widgets). One thing I have not "liberated" yet is our feature to transform inline-style attributes from JSON configuration into runtime CSS rules.

I'll do some more analysing of where I lost some of the effectiveness of the changes, but at least you now have a reference of what we (essentially) did.

from aikau.

OK... thanks - I'll clone your forked repo and take a look at the changes you've made

from aikau.

I've raised https://issues.alfresco.com/jira/browse/AKU-930 for us to cover investigating and merging the proposed changes into the Aikau code. It's currently planned for our next sprint.

from aikau.

OK... I've started to take a look at the proposed changes. My plan was to check out the branch provided, build it and then run the unit tests.

Unfortunately I hit a few conflicts in ListRenderer which I believe I've resolved correctly, however it seems like there are a lot of problems that are present on the branch (https://github.com/Alfresco/Aikau/tree/AKU-931_Performance) the unit test index page is not rendering correctly (there are no links displayed - just properties). I hit a few unit test pages at random and all seemed to have issues.

It might be worth you inspecting my branch to see that you're happy that I've resolved the conflicts as you've expected. Also, if you have time it might be useful to see if you can debug any of the issues - just getting the unit test index page running properly would be a good step.

I'll kick off a full regression test and see how many additional errors it throws up.

from aikau.

I've added test failures to https://issues.alfresco.com/jira/browse/AKU-931

from aikau.

I thought I mentioned it, but the changes on the branch were meant to be sufficient to run the test case of this ticket (the one in my gist) and showcase the performance differences. By no means were they meant to be complete, i.e. result in (all) existing unit tests to be run successfully.I explcitly mentioned that derived widgets (such as InlineEditProperty) would likely need to be adapted, but could have been more liberal and used a warning in bold text.
Since the poor quality of Node / Node-related libraries broke my use of maven-frontend-plugin, I won't be able to run any of those until I have had time to setup a VM for that...

from aikau.

That's an interesting list of failures. For some I can't think of how those could even have been affected by the change...

from aikau.

OK ... sorry, I must have missed that... I do obviously want to get the performance improvements in (if for no other reason than to demonstrate that we are willing to accept contributions!). I'll come back to this at the end of the sprint and look to see what code we can safely take from the changes you've proposed.

from aikau.

Another update... I've been looking through the code changes and your performance review in more detail (which is excellent, thank you for that). I'm in complete agreement that we need to tackle these things, unfortunately as you'd originally indicated (but I'd not fully absorbed) the changes can't be merged as they currently are because they break too many other things. The one thing I think we can definitely take without change is the date caching improvements.

The ideas your code pushes are really good, I just need to find a way to incorporate them without breaking too many other things. I'll continue to work on this for the rest of the current sprint and try and plan away to move forward by tackling specific areas without causing too much disruption.

from aikau.

I've made some progress... the cleanup at the end of _registerProcessedWidget was breaking promise resolution, removing that got a few things working again (but presumably introduces potential memory issues). Switching back to the original model based AlfDetailedView code worked as well. There's still quite a lot of things broken though - I'll dig into those next week.

from aikau.

I've merged the future version of generating child widgets in a new development package called aikau which is available in the 1.0.96 release. I'd appreciate you reviewing this code (from a performance point of view) as I start to create more widgets that use it. FYI... I haven't addressed the templating overhead concerns yet.

from aikau.

FYI... I'm starting to investigate this issue under https://issues.alfresco.com/jira/browse/AKU-878. I've set up a sample performance test page (that renders a list that is 100 rows by 50 cells with a single property rendered in each). I'm profiling the page and trying to squash the problems in a way that will be backwards compatible.

I'm going to be pushing to a private branch and am happy to take contributions to this branch - but preferably in small chunks. I am going to be working through your suggested changes though from your previous PR.

from aikau.

I'm going to close this issue now following the updates made in the 1.0.101 release as described in this post: https://community.alfresco.com/community/ecm/blog/2016/12/12/improving-aikau-performance

from aikau.