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

Improve performance for state update handling #3635

Merged
merged 4 commits into from Jul 2, 2023

Conversation

J-N-K
Copy link
Member

@J-N-K J-N-K commented May 27, 2023

Fixes #3620

There are two improvements in this PR:

  • The EventHandler now uses a "one-thread-per-subscriber` policy. This prevents single "slow" consumers from blocking others but still preserves event order.
  • The PageChangeListener and the SitemapResource have been refactored to use events instead implementing StateChangeListener.

J-N-K added 2 commits May 23, 2023 19:16
Signed-off-by: Jan N. Klug <github@klug.nrw>
Signed-off-by: Jan N. Klug <github@klug.nrw>
@J-N-K J-N-K added the enhancement An enhancement or new feature of the Core label May 27, 2023
@J-N-K J-N-K requested a review from a team as a code owner May 27, 2023 13:35
@J-N-K J-N-K changed the title Fix performance issues in state update handling Improve performance for state update handling May 27, 2023
Copy link

@TheTrueRandom TheTrueRandom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice improvements, tested on my raspberry pi with > 50 item updates/s which are handled with ease now. Before it was struggling with 30 updates/s.

Signed-off-by: Jan N. Klug <github@klug.nrw>
Signed-off-by: Jan N. Klug <github@klug.nrw>
@lolodomo
Copy link
Contributor

In my TODO list for the weekend to review this PR.

@lolodomo
Copy link
Contributor

lolodomo commented Jun 24, 2023

This code is probably correct even if I don't understand why it is better to use an event subscriber rather than implementing a StateChangeListener.
As this change updates the core behaviour of any sitemap UI, a very detailed review from @kaikreuzer is really expected. There were subtile code about groups and items state updates in previous code.

@J-N-K : I assume you checked that live update in BasicUI and Android app is still working as expected after your changes ?

@TheTrueRandom
Copy link

For me everything on BasicUI and Android app still works as before and I have quite some elements in it.

@lolodomo
Copy link
Contributor

lolodomo commented Jul 2, 2023

@kaikreuzer and all @openhab/core-maintainers : as it is said that performance are greatly improved (for users considering OH as a real-time system), I believe this PR should be considered. As it impacts a sensible part of sitemap UIs, I would suggest to not wait for the last OH4 minute to merge it but rather include it in coming milestone to have few weeks feedback form users.
I already reviewed the code and it looks OK to me. But I am honest, I do not understand why certain changes lead to improved performance. To be reviewed by someone who knows this part of code if possible.

Copy link
Member

@kaikreuzer kaikreuzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. My assumption is that the multi-threaded eventhandler brings the performance (well it actually shows that the event subscribers are not implemented well, since they are expected to return immediately...).
I do not see why moving to events for the page listeners is relevant here, but I am fine with that change as well. It is the riskier part wrt potential regressions, but I cannot think of a situation where this might be an issue. The only cases that would now differ in behavior are the ones where an item state changes without an event being created; this might only be the case at the system startup, but at that moment we do not care too much about the page update notifications either.

TL;DR: Lgtm, let's merge and have 2 more milestones for testing before GA.

@kaikreuzer kaikreuzer merged commit a656073 into openhab:main Jul 2, 2023
3 checks passed
@kaikreuzer kaikreuzer added this to the 4.0 milestone Jul 2, 2023
@J-N-K J-N-K deleted the executor-per-subscriber branch July 2, 2023 10:35
@lolodomo
Copy link
Contributor

lolodomo commented Jul 2, 2023

I believe the title of the PR could be enhanced to mention that it concerns sitemap UIs.

splatch pushed a commit to ConnectorIO/copybara-hab-core that referenced this pull request Jul 12, 2023
* Improve threading in EventHandler
* refactor pagechangelistener to event
* One executor per subscriber type, not per subscriber

Signed-off-by: Jan N. Klug <github@klug.nrw>
GitOrigin-RevId: a656073
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An enhancement or new feature of the Core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(Many) Issues with Sitemap Event Subscription
4 participants