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

Support message coalescing #306

Merged
merged 12 commits into from
Aug 25, 2022
Merged

Support message coalescing #306

merged 12 commits into from
Aug 25, 2022

Conversation

bdraco
Copy link
Member

@bdraco bdraco commented Aug 24, 2022

yarn link in this folder and then yarn link home-assistant-js-websocket

@bdraco bdraco changed the title Support message coalescing PoC: Support message coalescing Aug 24, 2022
@bdraco
Copy link
Member Author

bdraco commented Aug 24, 2022

That yarn add didn't work. Not sure how to actually test this

@balloob
Copy link
Member

balloob commented Aug 24, 2022

yarn link in this folder and then yarn link home-assistant-js-websocket

lib/connection.ts Outdated Show resolved Hide resolved
@balloob
Copy link
Member

balloob commented Aug 24, 2022

I think this change is great! I wonder how many race conditions we're going to discover.

lib/socket.ts Outdated Show resolved Hide resolved
@bdraco
Copy link
Member Author

bdraco commented Aug 24, 2022

I've got to be doing something dumb but I can't figure this one out

bdraco@Js-MacBook-Pro home-assistant-js-websocket % cd ~/frontend 
bdraco@Js-MacBook-Pro frontend % yarn link home-assistant-js-websocket
Usage Error: Invalid destination; Can't link the project to itself

$ yarn link [-A,--all] [-p,--private] [-r,--relative] <destination>
bdraco@Js-MacBook-Pro frontend % 

@balloob
Copy link
Member

balloob commented Aug 24, 2022

Hm, you can just do this I guess:

rm -rf node_moduels/home-assistant-js-websocket
ln -sf ../home-assistant-js-websocket node_moduels/home-assistant-js-websocket

And then it's basically an editable install. Restart webpack.

@bdraco
Copy link
Member Author

bdraco commented Aug 24, 2022

rm -rf node_modules/home-assistant-js-websocket
ln -sf ~/home-assistant-js-websocket node_modules/home-assistant-js-websocket

Seems to have worked.

Now I think I have a good test setup 👍

Thanks

@bdraco
Copy link
Member Author

bdraco commented Aug 24, 2022

Well spoke too soon

ERROR in ./src/state/disconnect-toast-mixin.ts 3:0-95
Module not found: Error: Can't resolve 'home-assistant-js-websocket' in '/Users/bdraco/frontend/src/state'

@bdraco
Copy link
Member Author

bdraco commented Aug 24, 2022

I'm going to do npm run build and copy in the files manually..

@bdraco
Copy link
Member Author

bdraco commented Aug 24, 2022

that works.

@bdraco
Copy link
Member Author

bdraco commented Aug 24, 2022

everything works and the latency problem from the connection overhead just disappeared with this change

@bdraco
Copy link
Member Author

bdraco commented Aug 24, 2022

Testing https://github.com/bdraco/scaletest and I get one message for the event storms it creates

Makes a big difference on performance

@bdraco
Copy link
Member Author

bdraco commented Aug 24, 2022

I changed scaletest to do 1000 sensors instead and it no longer overwhelms the system

@bdraco
Copy link
Member Author

bdraco commented Aug 24, 2022

And my browser is sitting at 1% cpu instead of maxing out

@bdraco
Copy link
Member Author

bdraco commented Aug 24, 2022

Still high cpu with firefox, but much better

@balloob
Copy link
Member

balloob commented Aug 24, 2022

We need to add an opt-in message somewhere. Maybe as part of the auth phase ?

@balloob
Copy link
Member

balloob commented Aug 24, 2022

Ah snap, we also need it to be backwards compatible. So can't do auth phase.

@bdraco
Copy link
Member Author

bdraco commented Aug 24, 2022

diff --git a/lib/messages.ts b/lib/messages.ts
index 57258c1..fd655e4 100644
--- a/lib/messages.ts
+++ b/lib/messages.ts
@@ -4,6 +4,7 @@ export function auth(accessToken: string) {
   return {
     type: "auth",
     access_token: accessToken,
+    features: ["coalesce_messages"],
   };
 }
 

I was thinking something like this but it blows up auth

@bdraco
Copy link
Member Author

bdraco commented Aug 24, 2022

Sending supported_features right after auth worked.

Kinda meh on the naming though

Screen Shot 2022-08-24 at 3 15 59 PM

Screen Shot 2022-08-24 at 3 16 05 PM

lib/messages.ts Outdated Show resolved Hide resolved
@bdraco
Copy link
Member Author

bdraco commented Aug 24, 2022

For testing I disabled the dev repo in frontend: to make sure it still works

before

And I get pages of

2022-08-24 15:28:25.198 ERROR (MainThread) [homeassistant.components.websocket_api.http.connection] [6429584608] Client exceeded max pending messages [2]: 2048
2022-08-24 15:28:25.199 ERROR (MainThread) [homeassistant.components.websocket_api.http.connection] [6429584608] Client exceeded max pending messages [2]: 2048
2022-08-24 15:28:25.199 ERROR (MainThread) [homeassistant.components.websocket_api.http.connection] [6429584608] Client exceeded max pending messages [2]: 2048
2022-08-24 15:28:25.199 ERROR (MainThread) [homeassistant.components.websocket_api.http.connection] [6429584608] Client exceeded max pending messages [2]: 2048
2022-08-24 15:28:25.199 ERROR (MainThread) [homeassistant.components.websocket_api.http.connection] [6429584608] Client exceeded max pending messages [2]: 2048
2022-08-24 15:28:25.199 ERROR (MainThread) [homeassistant.components.websocket_api.http.connection] [6429584608] Client exceeded max pending messages [2]: 2048
2022-08-24 15:28:25.199 ERROR (MainThread) [homeassistant.components.websocket_api.http.connection] [6429584608] Client exceeded max pending messages [2]: 2048
2022-08-24 15:28:25.199 ERROR (MainThread) [homeassistant.components.websocket_api.http.connection] [6429584608] Client exceeded max pending messages [2]: 2048
2022-08-24 15:28:25.199 ERROR (MainThread) [homeassistant.components.websocket_api.http.connection] [6429584608] Client exceeded max pending messages [2]: 2048
2022-08-24 15:28:25.199 ERROR (MainThread) [homeassistant.components.websocket_api.http.connection] [6429584608] Client exceeded max pending messages [2]: 2048
2022-08-24 15:28:25.201 ERROR (MainThread) [homeassistant.components.websocket_api.http.connection] [6429584608] Client exceeded max pending messages [2]: 2048
2022-08-24 15:28:25.201 ERROR (MainThread) [homeassistant.components.websocket_api.http.connection] [6429584608] Client exceeded max pending messages [2]: 2048

@bdraco
Copy link
Member Author

bdraco commented Aug 24, 2022

Here is the after
after

@balloob
Copy link
Member

balloob commented Aug 24, 2022

We should limit sending supported features based on the HA version that we get back. We can access the version string on the socket. https://github.com/home-assistant/home-assistant-js-websocket/blob/master/lib/socket.ts#L20

@balloob
Copy link
Member

balloob commented Aug 24, 2022

Otherwise people get warnings in their logs on older HA versions.

@bdraco
Copy link
Member Author

bdraco commented Aug 24, 2022

@balloob
Copy link
Member

balloob commented Aug 24, 2022

oops missed that.

lib/socket.ts Outdated Show resolved Hide resolved
lib/messages.ts Outdated Show resolved Hide resolved
@bdraco
Copy link
Member Author

bdraco commented Aug 24, 2022

Retesting looks good 👍

@bdraco bdraco marked this pull request as ready for review August 24, 2022 21:37
@bdraco bdraco changed the title PoC: Support message coalescing Support message coalescing Aug 24, 2022
@zachowj
Copy link
Contributor

zachowj commented Aug 25, 2022

Not sure if there is a standard enforcement, but you're using snake case where the existing code base is using camel case.

@bdraco
Copy link
Member Author

bdraco commented Aug 25, 2022

Not sure if there is a standard enforcement, but you're using snake case where the existing code base is using camel case.

That's what happens when you write more python 😄

Adjusted

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants