Skip to content
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

[PaperUI] Performance improvements - control page #3731

Merged
merged 2 commits into from Aug 4, 2017

Conversation

Projects
None yet
6 participants
@htreu
Copy link
Contributor

commented Jun 23, 2017

Addresses #3722.

@kaikreuzer

This comment has been minimized.

Copy link
Member

commented Jun 23, 2017

PaperUI: implement new location widget

This commit seems to be in here by accident.

@htreu htreu force-pushed the htreu:3722-performance-issue branch from 3e4d740 to ee9cd61 Jun 23, 2017

@htreu

This comment has been minimized.

Copy link
Contributor Author

commented Jun 23, 2017

ups, fixed :-)

@lolodomo

This comment has been minimized.

Copy link
Contributor

commented Jun 24, 2017

Build is faling for me:

[INFO] [10:43:05] Starting 'test'...
[INFO] adding F:\Dev\openhab2-master\git\smarthome\extensions\ui\org.eclipse.smarthome.ui.paper\node_modules\angular-mocks\angular-mocks.js
[INFO] 24 06 2017 10:43:06.047:ERROR [karma]: { [Error: spawn UNKNOWN] code: 'UNKNOWN', errno: 'UNKNOWN', syscall: 'spawn' }
[INFO] Error: spawn UNKNOWN
[INFO]     at exports._errnoException (util.js:856:11)
[INFO]     at ChildProcess.spawn (internal/child_process.js:298:11)
[INFO]     at exports.spawn (child_process.js:362:9)
[INFO]     at spawnWithoutOutput (F:\Dev\openhab2-master\git\smarthome\extensions\ui\org.eclipse.smarthome.ui.paper\node_modules\karma\lib\launchers\process.js:141:24)
[INFO]     at Object._execCommand (F:\Dev\openhab2-master\git\smarthome\extensions\ui\org.eclipse.smarthome.ui.paper\node_modules\karma\lib\launchers\process.js:63:21)
[INFO]     at Object._start (F:\Dev\openhab2-master\git\smarthome\extensions\ui\org.eclipse.smarthome.ui.paper\node_modules\karma-phantomjs-launcher\index.js:77:10)
[INFO]     at Object.<anonymous> (F:\Dev\openhab2-master\git\smarthome\extensions\ui\org.eclipse.smarthome.ui.paper\node_modules\karma\lib\launchers\process.js:14:10)
[INFO]     at emitOne (events.js:82:20)
[INFO]     at Object.emit (events.js:169:7)
[INFO]     at Object.start (F:\Dev\openhab2-master\git\smarthome\extensions\ui\org.eclipse.smarthome.ui.paper\node_modules\karma\lib\launchers\base.js:42:10)
[INFO]     at Object.j (F:\Dev\openhab2-master\git\smarthome\extensions\ui\org.eclipse.smarthome.ui.paper\node_modules\karma\lib\launcher.js:114:17)
[INFO]     at Object.setTimeout.bind.j (F:\Dev\openhab2-master\git\smarthome\extensions\ui\org.eclipse.smarthome.ui.paper\node_modules\qjobs\qjobs.js:143:18)
[INFO]     at Timer.listOnTimeout (timers.js:92:15)
[INFO] [10:43:06] 'test' errored after 560 ms
[INFO] [10:43:06] Error: 1
[INFO]     at formatError (F:\Dev\openhab2-master\git\smarthome\extensions\ui\org.eclipse.smarthome.ui.paper\node_modules\gulp\bin\gulp.js:169:10)
[INFO]     at Gulp.<anonymous> (F:\Dev\openhab2-master\git\smarthome\extensions\ui\org.eclipse.smarthome.ui.paper\node_modules\gulp\bin\gulp.js:195:15)
[INFO]     at emitOne (events.js:77:13)
[INFO]     at Gulp.emit (events.js:169:7)
[INFO]     at Gulp.Orchestrator._emitTaskDone (F:\Dev\openhab2-master\git\smarthome\extensions\ui\org.eclipse.smarthome.ui.paper\node_modules\orchestrator\index.js:264:8)
[INFO]     at F:\Dev\openhab2-master\git\smarthome\extensions\ui\org.eclipse.smarthome.ui.paper\node_modules\orchestrator\index.js:275:23
[INFO]     at finish (F:\Dev\openhab2-master\git\smarthome\extensions\ui\org.eclipse.smarthome.ui.paper\node_modules\orchestrator\lib\runTask.js:21:8)
[INFO]     at cb (F:\Dev\openhab2-master\git\smarthome\extensions\ui\org.eclipse.smarthome.ui.paper\node_modules\orchestrator\lib\runTask.js:29:3)
[INFO]     at removeAllListeners (F:\Dev\openhab2-master\git\smarthome\extensions\ui\org.eclipse.smarthome.ui.paper\node_modules\karma\lib\server.js:379:7)
[INFO]     at Server.<anonymous> (F:\Dev\openhab2-master\git\smarthome\extensions\ui\org.eclipse.smarthome.ui.paper\node_modules\karma\lib\server.js:390:9)
[INFO]     at Server.g (events.js:260:16)
[INFO]     at emitNone (events.js:72:20)
[INFO]     at Server.emit (events.js:166:7)
[INFO]     at emitCloseNT (net.js:1524:8)
[INFO]     at nextTickCallbackWith1Arg (node.js:467:9)
[INFO]     at process._tickCallback (node.js:389:17)
[ERROR]
[ERROR] npm ERR! Windows_NT 5.1.2600
[ERROR] npm ERR! argv "F:\\Dev\\openhab2-master\\git\\smarthome\\extensions\\ui\\org.eclipse.smarthome.ui.paper\\node\\node.exe" "F:\\Dev\\openhab2-master\\git\\smarthome\\extensions\\ui\\org.eclipse.smarthome.ui.paper\\node\\node_modules\\npm\\bin\\npm-cli.js" "run" "build"
[ERROR] npm ERR! node v5.4.0
[ERROR] npm ERR! npm  v3.3.12
[ERROR] npm ERR! code ELIFECYCLE
[ERROR] npm ERR! org.eclipse.smarthome.ui.paperui@0.9.0 build: `gulp default`
[ERROR] npm ERR! Exit status 1
[ERROR] npm ERR!
[ERROR] npm ERR! Failed at the org.eclipse.smarthome.ui.paperui@0.9.0 build script 'gulp default'.
[ERROR] npm ERR! Make sure you have the latest version of node.js and npm installed.
[ERROR] npm ERR! If you do, this is most likely a problem with the org.eclipse.smarthome.ui.paperui package,
[ERROR] npm ERR! not with npm itself.
[ERROR] npm ERR! Tell the author that this fails on your system:
[ERROR] npm ERR!     gulp default
[ERROR] npm ERR! You can get their info via:
[ERROR] npm ERR!     npm owner ls org.eclipse.smarthome.ui.paperui
[ERROR] npm ERR! There is likely additional logging output above.
[ERROR]
[ERROR] npm ERR! Please include the following file with any support request:
[ERROR] npm ERR!     F:\Dev\openhab2-master\git\smarthome\extensions\ui\org.eclipse.smarthome.ui.paper\npm-debug.log
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
@lolodomo

This comment has been minimized.

Copy link
Contributor

commented Jun 24, 2017

And here is my file npm-debug.log:

0 info it worked if it ends with ok
1 verbose cli [ 'F:\\Dev\\openhab2-master\\git\\smarthome\\extensions\\ui\\org.eclipse.smarthome.ui.paper\\node\\node.exe',
1 verbose cli   'F:\\Dev\\openhab2-master\\git\\smarthome\\extensions\\ui\\org.eclipse.smarthome.ui.paper\\node\\node_modules\\npm\\bin\\npm-cli.js',
1 verbose cli   'run',
1 verbose cli   'build' ]
2 info using npm@3.3.12
3 info using node@v5.4.0
4 verbose run-script [ 'prebuild', 'build', 'postbuild' ]
5 info lifecycle org.eclipse.smarthome.ui.paperui@0.9.0~prebuild: org.eclipse.smarthome.ui.paperui@0.9.0
6 silly lifecycle org.eclipse.smarthome.ui.paperui@0.9.0~prebuild: no script for prebuild, continuing
7 info lifecycle org.eclipse.smarthome.ui.paperui@0.9.0~build: org.eclipse.smarthome.ui.paperui@0.9.0
8 verbose lifecycle org.eclipse.smarthome.ui.paperui@0.9.0~build: unsafe-perm in lifecycle true
9 verbose lifecycle org.eclipse.smarthome.ui.paperui@0.9.0~build: PATH: F:\Dev\openhab2-master\git\smarthome\extensions\ui\org.eclipse.smarthome.ui.paper\node\node_modules\npm\bin\node-gyp-bin;F:\Dev\openhab2-master\git\smarthome\extensions\ui\org.eclipse.smarthome.ui.paper\node_modules\.bin;F:\Dev\openhab2-master\git\smarthome\extensions\ui\org.eclipse.smarthome.ui.paper\node;C:\Documents and Settings\Administrateur\bin;C:\Program Files\Git\mingw32\bin;C:\Program Files\Git\usr\local\bin;C:\Program Files\Git\usr\bin;C:\Program Files\Git\usr\bin;C:\Program Files\Git\mingw32\bin;C:\Program Files\Git\usr\bin;C:\Documents and Settings\Administrateur\bin;C:\Documents and Settings\All Users\Application Data\Oracle\Java\javapath;C:\WINDOWS\system32;C:\WINDOWS;C:\WINDOWS\system32\wbem;C:\Program Files\Intel\DMIX;C:\Program Files\MKVtoolnix;C:\Program Files\Git\cmd;C:\apache-maven-3.3.9\bin;C:\Program Files\nodejs;C:\Documents and Settings\Administrateur\Application Data\npm;C:\Program Files\Git\usr\bin\vendor_perl;C:\Program Files\Git\usr\bin\core_perl;
10 verbose lifecycle org.eclipse.smarthome.ui.paperui@0.9.0~build: CWD: F:\Dev\openhab2-master\git\smarthome\extensions\ui\org.eclipse.smarthome.ui.paper
11 silly lifecycle org.eclipse.smarthome.ui.paperui@0.9.0~build: Args: [ '/d /s /c', 'gulp default' ]
12 silly lifecycle org.eclipse.smarthome.ui.paperui@0.9.0~build: Returned: code: 1  signal: null
13 info lifecycle org.eclipse.smarthome.ui.paperui@0.9.0~build: Failed to exec build script
14 verbose stack Error: org.eclipse.smarthome.ui.paperui@0.9.0 build: `gulp default`
14 verbose stack Exit status 1
14 verbose stack     at EventEmitter.<anonymous> (F:\Dev\openhab2-master\git\smarthome\extensions\ui\org.eclipse.smarthome.ui.paper\node\node_modules\npm\lib\utils\lifecycle.js:232:16)
14 verbose stack     at emitTwo (events.js:87:13)
14 verbose stack     at EventEmitter.emit (events.js:172:7)
14 verbose stack     at ChildProcess.<anonymous> (F:\Dev\openhab2-master\git\smarthome\extensions\ui\org.eclipse.smarthome.ui.paper\node\node_modules\npm\lib\utils\spawn.js:24:14)
14 verbose stack     at emitTwo (events.js:87:13)
14 verbose stack     at ChildProcess.emit (events.js:172:7)
14 verbose stack     at maybeClose (internal/child_process.js:821:16)
14 verbose stack     at Process.ChildProcess._handle.onexit (internal/child_process.js:211:5)
15 verbose pkgid org.eclipse.smarthome.ui.paperui@0.9.0
16 verbose cwd F:\Dev\openhab2-master\git\smarthome\extensions\ui\org.eclipse.smarthome.ui.paper
17 error Windows_NT 5.1.2600
18 error argv "F:\\Dev\\openhab2-master\\git\\smarthome\\extensions\\ui\\org.eclipse.smarthome.ui.paper\\node\\node.exe" "F:\\Dev\\openhab2-master\\git\\smarthome\\extensions\\ui\\org.eclipse.smarthome.ui.paper\\node\\node_modules\\npm\\bin\\npm-cli.js" "run" "build"
19 error node v5.4.0
20 error npm  v3.3.12
21 error code ELIFECYCLE
22 error org.eclipse.smarthome.ui.paperui@0.9.0 build: `gulp default`
22 error Exit status 1
23 error Failed at the org.eclipse.smarthome.ui.paperui@0.9.0 build script 'gulp default'.
23 error Make sure you have the latest version of node.js and npm installed.
23 error If you do, this is most likely a problem with the org.eclipse.smarthome.ui.paperui package,
23 error not with npm itself.
23 error Tell the author that this fails on your system:
23 error     gulp default
23 error You can get their info via:
23 error     npm owner ls org.eclipse.smarthome.ui.paperui
23 error There is likely additional logging output above.
24 verbose exit [ 1, true ]
@htreu

This comment has been minimized.

Copy link
Contributor Author

commented Jun 24, 2017

It seems you are hit by this issue: ariya/phantomjs#14140.
Please try to rebuild w/o tests: change org.eclipse.smarthome.ui.paper/gulpfile.js line 93 from gulp.task('default', ['test']); to gulp.task('default', ['inject']); and try again.

@lolodomo

This comment has been minimized.

Copy link
Contributor

commented Jun 24, 2017

Ok, with gulpfile.js file updated, I succeeded building Paper UI.
Now I am facing a new problem, I failed replacing Paper UI in my production environment.
I put the new compiled jar file in my addons folder. Then I got these bundles:

openhab> bundle:list | grep UI
137 | Active    |  80 | 0.9.0.201706170545     | Eclipse SmartHome UI
138 | Active    |  80 | 0.9.0.201706170545     | Eclipse SmartHome UI Icons
171 | Active    |  80 | 2.1.0.201706161409     | openHAB Dashboard UI
186 | Active    |  80 | 0.9.0.201706170545     | Eclipse SmartHome Basic UI, Fragments: 197
187 | Active    |  80 | 0.9.0.201706170545     | Eclipse SmartHome WebApp UI
197 | Resolved  |  75 | 2.1.0.201706161409     | openHAB Basic UI Fragment, Hosts: 186
198 | Active    |  75 | 2.1.0.201706161409     | openHAB Classic UI Fragment
216 | Active    |  80 | 0.9.0.201706170545     | Eclipse SmartHome Paper UI, Fragments: 217
217 | Resolved  |  75 | 2.1.0.201706161409     | openHAB Paper UI Theme Fragment, Hosts: 216
218 | Installed |  80 | 0.9.0.201706241405     | Eclipse SmartHome Paper UI

If I stop bundle 216 and start new bundle 218, I got this error:

openhab> bundle:stop 216
openhab> bundle:start 218
Error executing command: Error executing command on bundles:
        Error starting bundle 218: Could not resolve module: org.eclipse.smarthome.ui.paper [218]
  Another singleton bundle selected: osgi.identity; osgi.identity="org.eclipse.smarthome.ui.paper"; type="osgi.bundle"; version:Version="0.9.0.201706170545"; singleton:="true"

Any advice how I should process ?

@lolodomo

This comment has been minimized.

Copy link
Contributor

commented Jun 24, 2017

Suppressing the feature openhab-ui-paper helped.
I can now access Paper UI, not from the dashnoard but with the direct link http://192.168.xx.xx:8080/paperui/index.html#/inbox/search
Strangely, I don't get the usual colors...
If I go to the Control page, there is apparently no lock but the page remains empty.

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.

@maggu2810

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2017

@lolodomo You should not installing a new PaperUI bundle but replace / updating your currently installed one (for Karaf see bundle:update --help)

@htreu htreu force-pushed the htreu:3722-performance-issue branch from ee9cd61 to 2916387 Jun 27, 2017

@htreu

This comment has been minimized.

Copy link
Contributor Author

commented Jun 27, 2017

@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.

@htreu

This comment has been minimized.

Copy link
Contributor Author

commented Jun 27, 2017

travis-ci can be ignored for now, its complaining about failing HTTP requests...

@htreu

This comment has been minimized.

Copy link
Contributor Author

commented Jun 27, 2017

@lolodomo you dont necessarily have to update your openHAB instance for a quick test:
Edit org.eclipse.smarthome.ui.paper/gulpfile.js again on line 185 and change the target property to the address of your local openHAB instance. Then run npm start from the terminal which will open PaperUI from the current checkout in your browser.

@lolodomo

This comment has been minimized.

Copy link
Contributor

commented Jun 27, 2017

Interesting, thank you, I will test this evening.

@lolodomo

This comment has been minimized.

Copy link
Contributor

commented Jun 28, 2017

@htreu : can you please give me an example of the change to do in the gupfile ? That is not obvious.

@htreu

This comment has been minimized.

Copy link
Contributor Author

commented Jun 29, 2017

change the line

server.middleware = proxyMiddleware(['/rest','/icon','/audio'], {target: 'http://localhost:8080'});

to

server.middleware = proxyMiddleware(['/rest','/icon','/audio'], {target: '<yourServerAddressHere>'});
@lolodomo

This comment has been minimized.

Copy link
Contributor

commented Jul 1, 2017

I tried but I got an error when I run the command npm start:

...
[11:43:28] gulp-inject 21 files into index.html.
[11:43:28] Finished 'inject' after 97 ms
[11:43:28] Starting 'test'...
adding F:\Dev\openhab2-master\git\smarthome\extensions\ui\org.eclipse.smarthome.ui.paper\node_modules\angular-mocks\angular-mocks.js
01 07 2017 11:46:41.893:ERROR [launcher]: PhantomJS failed 2 times (timeout). Giving up.



[11:46:41] 'test' errored after 3.2 min
[11:46:41] Error: 1
    at formatError (F:\Dev\openhab2-master\git\smarthome\extensions\ui\org.eclipse.smarthome.ui.paper\node_modules\gulp\bin\gulp.js:169:10)
    at Gulp.<anonymous> (F:\Dev\openhab2-master\git\smarthome\extensions\ui\org.eclipse.smarthome.ui.paper\node_modules\gulp\bin\gulp.js:195:15)
    at Gulp.emit (events.js:95:17)
    at Gulp.Orchestrator._emitTaskDone (F:\Dev\openhab2-master\git\smarthome\extensions\ui\org.eclipse.smarthome.ui.paper\node_modules\orchestrator\index.js:264:8)
    at F:\Dev\openhab2-master\git\smarthome\extensions\ui\org.eclipse.smarthome.ui.paper\node_modules\orchestrator\index.js:275:23
    at finish (F:\Dev\openhab2-master\git\smarthome\extensions\ui\org.eclipse.smarthome.ui.paper\node_modules\orchestrator\lib\runTask.js:21:8)
    at cb (F:\Dev\openhab2-master\git\smarthome\extensions\ui\org.eclipse.smarthome.ui.paper\node_modules\orchestrator\lib\runTask.js:29:3)
    at removeAllListeners (F:\Dev\openhab2-master\git\smarthome\extensions\ui\org.eclipse.smarthome.ui.paper\node_modules\karma\lib\server.js:379:7)
    at Server.<anonymous> (F:\Dev\openhab2-master\git\smarthome\extensions\ui\org.eclipse.smarthome.ui.paper\node_modules\karma\lib\server.js:390:9)
    at Server.g (events.js:180:16)
    at Server.emit (events.js:117:20)
    at net.js:1277:10
    at process._tickCallback (node.js:458:13)

npm ERR! Windows_NT 5.1.2600
npm ERR! argv "C:\\Program Files\\nodejs\\node.exe" "C:\\Program Files\\nodejs\\node_modules\\npm\\bin\\npm-cli.js" "start"
npm ERR! node v0.10.48
npm ERR! npm  v2.15.1
npm ERR! code ELIFECYCLE
npm ERR! org.eclipse.smarthome.ui.paperui@0.9.0 start: `gulp serve --development`
npm ERR! Exit status 1
npm ERR!
npm ERR! Failed at the org.eclipse.smarthome.ui.paperui@0.9.0 start script 'gulp serve --development'.
npm ERR! This is most likely a problem with the org.eclipse.smarthome.ui.paperui package,
npm ERR! not with npm itself.

and I can find that inthe file npm-debug.log:

9 verbose stack Error: org.eclipse.smarthome.ui.paperui@0.9.0 start: `gulp serve --development`
9 verbose stack Exit status 1
9 verbose stack     at EventEmitter.<anonymous> (C:\Program Files\nodejs\node_modules\npm\lib\utils\lifecycle.js:217:16)
9 verbose stack     at EventEmitter.emit (events.js:98:17)
9 verbose stack     at ChildProcess.<anonymous> (C:\Program Files\nodejs\node_modules\npm\lib\utils\spawn.js:24:14)
9 verbose stack     at ChildProcess.emit (events.js:98:17)
9 verbose stack     at maybeClose (child_process.js:766:16)
9 verbose stack     at Process.ChildProcess._handle.onexit (child_process.js:833:5)
@htreu

This comment has been minimized.

Copy link
Contributor Author

commented Jul 3, 2017

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 npm start again.

@lolodomo

This comment has been minimized.

Copy link
Contributor

commented Jul 3, 2017

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 htreu force-pushed the htreu:3722-performance-issue branch from 2916387 to 42c6d1a Jul 5, 2017

@htreu

This comment has been minimized.

Copy link
Contributor Author

commented Jul 5, 2017

@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.

@lolodomo

This comment has been minimized.

Copy link
Contributor

commented Jul 5, 2017

Still unusable, I even had to kill my Firefox process.
Something that could be interesting:

[HPM] Proxy error: EADDRINUSE. localhost:3000 -> "192.168.1.xxx:8080/rest/items/SonosSalonAlbumArt/state"
[HPM] Proxy error: EADDRINUSE. localhost:3000 -> "192.168.1.xxx:8080/rest/items/SonosChambreAlbumArt/state"
[HPM] Proxy error: EADDRINUSE. localhost:3000 -> "192.168.1.xxx:8080/rest/items/WUForecastIcon/state"

There are apparently problems with Image channels. Was there any change in this part ?
I will try to unlink all these channels and see if it is better.

@htreu

This comment has been minimized.

Copy link
Contributor Author

commented Jul 5, 2017

[HPM] Proxy error: EADDRINUSE. localhost:3000 is an error from the node web proxy. I´m not sure what port/address the message is referring to. Do you have multiple npm start processes running? I will setup an example with image channels too and see if I can reproduce this and the performance impact.

@lolodomo

This comment has been minimized.

Copy link
Contributor

commented Jul 5, 2017

I was running only one npm start.

@lolodomo

This comment has been minimized.

Copy link
Contributor

commented Jul 5, 2017

My feeling is that all tabs are loaded. Can you confirm ? Why not just loading the current tab corresponding to the selected location ?

@htreu

This comment has been minimized.

Copy link
Contributor Author

commented Jul 5, 2017

Yes, currently all tabs with all things/channels/items are loaded into the DOM. I will check if this can be done asynchronously.

@htreu

This comment has been minimized.

Copy link
Contributor Author

commented Jul 5, 2017

But even with only one tab it lags when astro:sun with all channels linked is displayed.

@lolodomo

This comment has been minimized.

Copy link
Contributor

commented Jul 5, 2017

I have one astro sun thing too but only with 2 date channels linked.

@lolodomo

This comment has been minimized.

Copy link
Contributor

commented Jul 5, 2017

Is it normal to have REST requests for state for my 2 Sonos things while they are not in the current tab ?
If a thing state is updated, does it trigger a full reload in control tabs, even if the thing is not displayed in the current control tab ?

@htreu

This comment has been minimized.

Copy link
Contributor Author

commented Jul 5, 2017

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.

@lolodomo

This comment has been minimized.

Copy link
Contributor

commented Jul 23, 2017

https://community.openhab.org/t/sonos-currentalbumart-error/29682/10

Finally, it looks like images were causing problems.

@maggu2810

This comment has been minimized.

Copy link
Contributor

commented Jul 24, 2017

@maggu2810 I'll review it today.

@aounhaider1 Thanks for your willingness to review this PR. Is your review already finished?

@aounhaider1

This comment has been minimized.

Copy link
Contributor

commented Jul 24, 2017

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.
screen shot 2017-07-24 at 15 06 40

@htreu

This comment has been minimized.

Copy link
Contributor Author

commented Jul 25, 2017

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.
In contrast to the angularjs digest cycle (which is responsible for nearly half the rendering time) the duplicate requests are really fast. Thats the reason I decided to live with this drawback (at least for now).

@lolodomo

This comment has been minimized.

Copy link
Contributor

commented Jul 25, 2017

@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.
Maybe you fixed this problem too ?

@lolodomo

This comment has been minimized.

Copy link
Contributor

commented Jul 28, 2017

#3838 seems to confirm a problem with images.

@htreu

This comment has been minimized.

Copy link
Contributor Author

commented Jul 28, 2017

I will check image items with this PR and report back here.

Henning Treu
PaperUI: Control page performance improvements & refactoring
Signed-off-by: Henning Treu <henning.treu@telekom.de>
@htreu

This comment has been minimized.

Copy link
Contributor Author

commented Jul 31, 2017

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.

Henning Treu
order items on label
Signed-off-by: Henning Treu <henning.treu@telekom.de>

@htreu htreu force-pushed the htreu:3722-performance-issue branch from 015f761 to fcc50f1 Jul 31, 2017

@lolodomo

This comment has been minimized.

Copy link
Contributor

commented Jul 31, 2017

Maybe your next challenge could be to solve performance with loading of extensions ;)

@htreu

This comment has been minimized.

Copy link
Contributor Author

commented Aug 1, 2017

challenge accepted :-) Although I would like do do it separate from this one.

@maggu2810

This comment has been minimized.

Copy link
Contributor

commented Aug 1, 2017

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.
In contrast to the angularjs digest cycle (which is responsible for nearly half the rendering time) the duplicate requests are really fast. Thats the reason I decided to live with this drawback (at least for now).

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.
I will try to find the respective issue.

@maggu2810

This comment has been minimized.

Copy link
Contributor

commented Aug 1, 2017

See: #3010

@htreu

This comment has been minimized.

Copy link
Contributor Author

commented Aug 1, 2017

Thanks @maggu2810. I will check again if this is an issue.

@kaikreuzer

This comment has been minimized.

Copy link
Member

commented Aug 2, 2017

@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.

@htreu

This comment has been minimized.

Copy link
Contributor Author

commented Aug 4, 2017

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.
In the long run I don´t want to live with this drawback either but I request to do it on a separate issue after this is merged.

@kaikreuzer or @maggu2810 wdyt?

edit: I missed @kaikreuzer´s last comment, so please proceed and merge :-)

@kaikreuzer

This comment has been minimized.

Copy link
Member

commented Aug 4, 2017

As soon as you have created a new issue, I would be fine to merge this PR :-)

@htreu

This comment has been minimized.

Copy link
Contributor Author

commented Aug 4, 2017

Further improvement in caching thing-types: #3954.

@kaikreuzer kaikreuzer merged commit 06fbe2b into eclipse:master Aug 4, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
ip-validation
Details

@htreu htreu deleted the htreu:3722-performance-issue branch Aug 4, 2017

@kaikreuzer kaikreuzer added this to the 0.9.0 milestone Nov 30, 2017

@kaikreuzer kaikreuzer added the bug label Dec 15, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.