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

Async Change Causing Random HA Slowdowns #15

Closed
khcnz opened this issue Sep 2, 2020 · 12 comments
Closed

Async Change Causing Random HA Slowdowns #15

khcnz opened this issue Sep 2, 2020 · 12 comments

Comments

@khcnz
Copy link
Collaborator

khcnz commented Sep 2, 2020

Hey Scott,

I have been getting really odd slow random slow loading across my HA instance for the past week or so where occasionally loading any HA page was very slow (a few seconds). This was happening on my local network and I discovered this seemed to be happening to ANY file/request served by HA (for example https://[LocalIP]/manifest.json which is a super basic manifest file)

I managed to isolate this to changes I made to add the sensor platform to get indoor/outdoor temperatures for my units (6 internal units connected x 2 calls for indoor and outdoor temp) which was on top of my existing 6 devices for climate - this was therefore potentially hitting the network 36 times for each update (every 1 minute I think).

That number of hits isn't too bad (but I think we can reduce it down to 6 by pulling back the outdoor temp on the update method and by sharing the api object between a climate and sensor object at initialization time - but that's a suggestion I'm happy to help with for a later date).

I've done a bit of digging and I think the async changes #7 you made aren't correct.. but I'm not an HA expert so before suggesting a PR thought I'd discuss that with you. From what I can tell, you've changed to use the async methods but the api calls inside update() are synchronous/blocking i/o calls - and I think HA just has the one main event thread between the web server and the calling of the aysnc update method. If you read the async docs I think they suggest you should either stick with the non async methods, or you need to wrap any synchronous methods inside update() with the following syntax:

result = await hass.async_add_executor_job(hub.update).

There is another forum question/answer along these lines here also

So in short, I think the HA instance was basically frozen for a few seconds every time the update call happened. You can see this by adding a few incorrect climate entries by ip address to your config then refresh https://[LocalIP]/manifest.json over and over again and you will see it slow down every minute for a couple of seconds.

HA seems to imply that the async methods are newer/better so without having to rewrite your econet library to be async as well I think this can be solved as simply as awaiting the api.update() call.

Let me know your thoughts and I'm happy to help with a PR if you agree!

@khcnz
Copy link
Collaborator Author

khcnz commented Sep 2, 2020

Oh I've also just realized the same will likely be true for:

  • async_turn_on/off
  • async_set_hvac_mode/async_set_fan_mode
  • async_set_temperature

@scottyphillips
Copy link
Owner

6 Devices! That is a lot of devices! :-) Its certainly not something I have encountered to be truthful since I only have the one device. If you want to propose a refresh of the component please raise a PR. To be honest I am not super familiar with the asyncio behaviour, so more then happy to take a PR on its merit. At this stage I would even be prepared to make you a code owner if you were happy to continue to improve the component.

@scottyphillips
Copy link
Owner

Ive pushed some updated code to 'experimental' which implements hass.async_add_executor_job for everything execpt on/off. For reasons I don't quite understand at present I cant seem to implement for async_turn_on or async_turn_off

@khcnz
Copy link
Collaborator Author

khcnz commented Sep 8, 2020

6 Devices! That is a lot of devices! :-)

It's not quite as many as it sounds like, it's really only 2 exterior devices, but i've got one exterior unit that drives 5 indoor units 👍

Its certainly not something I have encountered to be truthful since I only have the one device. If you want to propose a refresh of the component please raise a PR. To be honest I am not super familiar with the asyncio behaviour, so more then happy to take a PR on its merit.

Thanks I see you did it already, will help test this one ;)

At this stage I would even be prepared to make you a code owner if you were happy to continue to improve the component.

Sounds great i'm happy to contribute to help with the swing mode changes into hass as well. It will be a little easier to collaborate with PR's within the repo etc.

@khcnz
Copy link
Collaborator Author

khcnz commented Sep 16, 2020

I've been testing the change and it has fixed the behaviour I was having, will do a PR with a few things in the coming days

@scottyphillips
Copy link
Owner

Sweet thanks for the update.

@carlito1979
Copy link

Hi scotty - any chance this fix was pulled into the code? I'm about to hook up 3 heatpumps and want to use this awesome addon

@scottyphillips
Copy link
Owner

yes it was promoted into the main branch. im closing this off now

@khcnz
Copy link
Collaborator Author

khcnz commented May 22, 2021

I just updated to the latest version in master which removed the local changes I had and everything slowed down again! I think this wasn't merged to master?

The changes like this in this branch look they didn't make it to master branch - sorry i hadn't noticed sooner was running the old experimental branch locally
image

@scottyphillips
Copy link
Owner

I just merged the master and experimental branches but I’m away from my computer at the moment, I don’t suppose you can test the changes for me?

@scottyphillips
Copy link
Owner

I’m unsure why parts of the update slipped through the cracks, but I’ve just tested the master branch now and it seems to be working on my setup. The code is a little overdue for a clean up anyway I’ve been a bit busy doing dad stuff but when I get back to my computer I’ll fix up a few things.

@khcnz
Copy link
Collaborator Author

khcnz commented May 22, 2021

Running it now, so far everything working right

@khcnz khcnz closed this as completed May 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants