[PaperUI] Performance improvements - control page #3731
Conversation
This commit seems to be in here by accident. |
htreu
force-pushed
the
3722-performance-issue
branch
from
3e4d740
to
ee9cd61
Compare
|
ups, fixed :-) |
|
Build is faling for me: |
|
And here is my file npm-debug.log: |
|
It seems you are hit by this issue: ariya/phantomjs#14140. |
|
Ok, with gulpfile.js file updated, I succeeded building Paper UI. If I stop bundle 216 and start new bundle 218, I got this error: Any advice how I should process ? |
|
Suppressing the feature openhab-ui-paper helped. With the version packaged in OH snapshot (same in your version), I also notice that displaying the extensions is now slower than ever ... but working if I am very patient and click several times on "Continue" in my Firefox popup asking me if I want to continue or stop the script. |
|
@lolodomo You should not installing a new PaperUI bundle but replace / updating your currently installed one (for Karaf see |
htreu
force-pushed
the
3722-performance-issue
branch
from
ee9cd61
to
2916387
Compare
|
@lolodomo I did another commit which for me improves rendering speed a lot. Using the chrome performance tools it shows only half the time rendering the control page. Please give it a try and let me know if you still have issues. Thanks. |
|
travis-ci can be ignored for now, its complaining about failing HTTP requests... |
|
@lolodomo you dont necessarily have to update your openHAB instance for a quick test: |
|
Interesting, thank you, I will test this evening. |
|
@htreu : can you please give me an example of the change to do in the gupfile ? That is not obvious. |
|
change the line to |
|
I tried but I got an error when I run the command npm start: and I can find that inthe file npm-debug.log: |
|
I created a gulpfile.js w/o tests and placeholders for ip address and port here: https://gist.github.com/htreu/4084452d0b54b03e4b9dc39659efc287 Please replace your local copy with the content from the gist, replace ip address and port in line 184 as shown above and run |
|
Ok, thank you, I was able to start "your" Paper UI. Unfortunately, I see no change to my problem. I have probably to identify which one of my things is putting Paper UI in such unresponsive case. |
htreu
force-pushed
the
3722-performance-issue
branch
from
2916387
to
42c6d1a
Compare
|
@lolodomo I did another bunch of improvements. For me the change between tabs in the control page is much more smooth now. What really kills performance for me is the local:sun thing, even now. I have to work on this once more. Please give it another try. tnx. |
|
Still unusable, I even had to kill my Firefox process. There are apparently problems with Image channels. Was there any change in this part ? |
|
|
|
I was running only one npm start. |
|
My feeling is that all tabs are loaded. Can you confirm ? Why not just loading the current tab corresponding to the selected location ? |
|
Yes, currently all tabs with all things/channels/items are loaded into the DOM. I will check if this can be done asynchronously. |
|
But even with only one tab it lags when astro:sun with all channels linked is displayed. |
|
I have one astro sun thing too but only with 2 date channels linked. |
|
Is it normal to have REST requests for state for my 2 Sonos things while they are not in the current tab ? |
|
Angular should observe only the specific DOM elements which are bind to the item states. Then it will only change values in these DOM elements. But this is also the case for off screen items (in tabs currently not visible). With the current setup we are not able to change that. But the performance impact should not be too dramatic. |
|
@maggu2810 I'll review it today. |
|
https://community.openhab.org/t/sonos-currentalbumart-error/29682/10 Finally, it looks like images were causing problems. |
@aounhaider1 Thanks for your willingness to review this PR. Is your review already finished? |
|
Lgtm. I have just one comment on thing-type requests. We recently improved the number of requests to same thing-types by caching them. In the new code this seems to be lost and now a new request is sent even if the same thing-type was already requested. Maybe this can be improved. |
|
Hi @aounhaider1, thanks for the review. The effect you see is actually the main drawback this refactoring introduces. In fact the thing-types do get cached. Unfortunately when rendering for the first time all things are rendered simultaneously and thing-types are requested multiple times. The cache is then filled during promise resolving but after (read: later) all thing-types have been requested. |
|
@htreu: could you please check yourself the impact of image items ? It was reported by 2 users on the forum that image item makes Paper UI unresponsive. |
|
#3838 seems to confirm a problem with images. |
|
I will check image items with this PR and report back here. |
Signed-off-by: Henning Treu <henning.treu@telekom.de>
|
From what I can see in my setup the image items have no impact on performance. I did although make one small enhancement assuring the order of items is stable between reloads. Also: rebased to current master. |
Signed-off-by: Henning Treu <henning.treu@telekom.de>
htreu
force-pushed
the
3722-performance-issue
branch
from
015f761
to
fcc50f1
Compare
|
Maybe your next challenge could be to solve performance with loading of extensions ;) |
|
challenge accepted :-) Although I would like do do it separate from this one. |
Is this still true if e.g. the Z-Wave binding is used. IIRC the size of the thing-types has been around 1.5 MB. |
|
See: #3010 |
|
Thanks @maggu2810. I will check again if this is an issue. |
|
@htreu So is this PR in a state where it improves stuff and is mergeable? If so, I would go for it and address further things in separate PRs. |
|
Although thing-types get requested several times on initial rendering I would like to have this merged. The gain from this PR in using the AngularJS API outperforms the loss from multiple requests. @kaikreuzer or @maggu2810 wdyt? edit: I missed @kaikreuzer´s last comment, so please proceed and merge :-) |
|
As soon as you have created a new issue, I would be fine to merge this PR :-) |
|
Further improvement in caching thing-types: #3954. |
Addresses #3722.