optimize Send Message to Groups by changing Client => encryptMessageForDevices #721
Conversation
…e sessions and keys related to the message encryption and storing them into a cache, then storing them all into the db at once to minimize I/O to the database
|
@tulir can this be merged please? |
There was a problem hiding this comment.
The solution likely will solve the issue, but the price we'll have to pay later on maintenance with that code is too much, IMO.
If we could hide all "caches" inside the Store (so the caller encryptMessageForDevices doesn't care about caches and always get the final result - it'd be great! And reducing number of "caches" would also help, right now you move entities from one to another in a bit complex way.
ℹ️ It's not an official maintainer opinion, I was just passing by the PR :)
| @@ -126,6 +126,13 @@ type PrivacyTokenStore interface { | |||
| GetPrivacyToken(user types.JID) (*PrivacyToken, error) | |||
| } | |||
|
|
|||
| type CacheStore interface { | |||
| GetSessions(addresses []string) map[string][]byte | |||
There was a problem hiding this comment.
Instead of using primitives (string/byte) we should try using already defined types, like types.JID (likely there's something for Session/IdentityKeys as well)
There was a problem hiding this comment.
IdentityKeys and Session types are both []byte in the code which is the reason I chose that
| Cache CacheStore | ||
| Container DeviceContainer | ||
|
|
||
| DatabaseErrorHandler func(device *Device, action string, attemptIndex int, err error) (retry bool) | ||
|
|
||
| //Cache to Temporary save sessions and identity keys for faster group send | ||
| SessionsCache map[string][]byte | ||
| IdentityKeysCache map[string][32]byte |
There was a problem hiding this comment.
Looks like too many Caches here, I lost the idea when we use SessionCache, when we use IdentityKeysCache and when just Cache.
There was a problem hiding this comment.
Hello
Sorry for the delay, I didn't check my github in a while
So the idea of CacheStore is to a have a way that implements the method to get all the required IdentityKeys and Sessions at once
SessionsCache and IdentityCache save the result of said query. While doing it on redis will certainly improve speed, I would be forcing the implementation of the library to be dependent on that DB
| } | ||
| //clear the cache once the encryption is done to release memory | ||
| clear(cli.Store.SessionsCache) |
There was a problem hiding this comment.
interesting that we clear cache after every call - why does it need to be available global then?
There was a problem hiding this comment.
Since the I/O to the db is being used inside LoadSession and IsTrustedIdentity in store/signal.go, the check had to be made whether we already have it or not. Since both of these functions are whatsmeows implementation of another library, this was the only suitable place I could find to place it in
|
I think that, like @devlikepro said, having multiple ambiguous caches isn't very sustainable. I'd suggest you implement your own SQLStore layer that hits your own custom cache first. You can also look into other options like Redis at that point. |
|
@tulir any chance you or any other maintainer are working on something that fixes this ? |
|
The proper solution is updating libsignal-protocol-go as well as the sql store to use contexts, after which the code can use a native db transaction. Not planned in the near future, but will probably be added at some point |
Hello |
If I'm not mistaken the approach you're suggesting is wrapping the entire process in a transaction, which wouldn't really solve the main issue which is too many I/Os to the database. Thank you for your response |
I meant implementing this on your own outside of the library, in my original approach. |
|
i need this |
|
I try this PR in my fork and o have this error
|
|
Individual mesage @http://s.whatsapp.net no working to with this PR, i send mutiples message and stop send |
You have to share the version you're using for me to understand what's wrong in your fork |
I use the current version of the main branch, I forked it and then applied this PR |
hello there
thank you for the library
we are using it on a project of ours with currently about 150 clients. Sending message in them had gotten extremely slow, especially in large groups, so I performed some changes on it to improve the speed. I decided to use some of those changes in the main library as well, in case someone was having the same issue. the changes are as following:
increased send message performance by collecting all neccessary device sessions and keys related to the message encryption and storing them into a cache, then storing them all into the db at once to minimize I/O to the database