Reduce number of queries required to read shares #7743
Conversation
Signed-off-by: Dariusz Olszewski <starypatyk@users.noreply.github.com>
starypatyk
added
2. developing
enhancement
feature: chat 💬
|
Can you give some rough numbers about your chats? Are they mostly files only? On our company instance there are roughly 7k files shared between 2m chat messages. So the cases where so many files are in a single chunk that duplicating the method is worth the improvements are rather limited |
|
Hi @nickvergessen, My "production" Nextcloud instance is a family server used primarily to share photos, so images are very frequently shared in chats. The server runs for over 8 years now (it started as Owncloud), but we use Talk for only about 1.5 year. The statistics are:
In two sample chats I have 33 and 40 images in the last 100 messages. On the other hand, in my day job (where we use Teams), I see that we also send a lot of images (primarily screenshots). As I understand the Handle pasting screenshots to the chat message form feature also uses files sharing. Same applies to Image keyboard support in chat that I had implemented at the beginning of this year in the Android client - to enliven the conversations. 😉 Maybe I am heavily biased, but for me images are an important part of chat conversations. |
Totally fine, we only have a Meme channel with a that high rate. |
|
Thanks @nickvergessen! Please note that the code is currently of "proof-of-concept" quality. I have not been able to find a way to gather all share IDs using the existing infrastructure (MessageParser::parseMessage -> Listener::parseSystemMessage -> SystemMessage::parseMessage -> SystemMessage::getFileFromShare). That's why there is a hack at the top level method (ChatController::receiveMessages) going directly to the RoomShareProvider. |
|
I deployed it on our testing copy for now
so yeah at the moment it is not helping too much. |
lib/Share/RoomShareProvider.php
Outdated
| @@ -631,6 +635,11 @@ public function getSharesBy($userId, $shareType, $node, $reshares, $limit, $offs | |||
| * @throws ShareNotFound | |||
| */ | |||
| public function getShareById($id, $recipientId = null): IShare { | |||
|
|
|||
| if (isset($this->sharesByIdCache[$id])) { | |||
There was a problem hiding this comment.
Cache needs to be invalidated on delete/update function calls
There was a problem hiding this comment.
I have added cache invalidation (cleaning actually 😉) in all update/delete calls that seemed related in 03f6476.
I would say that is even okay, could even make the function name reflect that: |
|
With those 2 PRs in addition our Meme channel is down to 69 queries: |
Co-authored-by: Joas Schilling <213943+nickvergessen@users.noreply.github.com> Signed-off-by: Dariusz Olszewski <8277636+starypatyk@users.noreply.github.com>
(Nuclear option - clean the cache entirely) Signed-off-by: Dariusz Olszewski <starypatyk@users.noreply.github.com>
Signed-off-by: Dariusz Olszewski <starypatyk@users.noreply.github.com>
|
@nickvergessen - thanks for your comments. I have already applied most of your suggestions. Regarding the remaining suggestion:
I would like to reduce code duplication between current
I am still working on this. |
Also fix handling of inaccessible shares Signed-off-by: Dariusz Olszewski <starypatyk@users.noreply.github.com>
|
@nickvergessen - I have refactored getShareById/getSharesByIds in e8bd174. I hope that I have fixed the issues with inaccessible shares and Could you check this please? The code became a bit convoluted and I am far from saying that I have checked all possible scenarios. |
|
Looks good. If you have troubles satisfying the CI jobs we can also take over at the end. |
Psalm uncovered a logic error regarding passing $shares array to resolveSharesForRecipient :-) Signed-off-by: Dariusz Olszewski <starypatyk@users.noreply.github.com>
Signed-off-by: Dariusz Olszewski <starypatyk@users.noreply.github.com>
|
I have managed to fix all failing tests. I am wondering, if it makes sense to write an additional test that would check that the |
I kind of dislike unit tests on that level. Where we have some tests were we run locally with total query counting enabled and that would then reveal an overall picture. |
Signed-off-by: Dariusz Olszewski <starypatyk@users.noreply.github.com>
Signed-off-by: Dariusz Olszewski <starypatyk@users.noreply.github.com>
|
@nickvergessen - I do not plan any more changes - unless you have additional comments. Please feel free to merge. |
|
Thanks a lot @starypatyk for the patch and the good analysis! |
When diagnosing slow loading of chat messages in the Talk Android app, I have noticed that information about files (usually images) shared in the chat is read using a separate query for each file.
This is an initial proof-of-concept that reduces it to a single query.
Instead of executing a separate query in RoomShareProvider::getShareById for each share, we gather all share ids and execute a single query in a new method RoomShareProvider::getSharesByIds.
While performance improvements in the development environment are difficult to notice, on my family "production" environment this gives around 15% improvement of the ChatController::receiveMessages execution time (280 ms down from 330 ms).
This code is certainly not production-ready and requires refactoring/clean-up. However I would like to get some feedback, if this looks like a reasonable way to go forward.
Trying to read info about all shared items together also seems to be a pre-requisite to eventually optimize SystemMessage::getFileFromShare. Currently additional 3-4 database queries are performed for each shared file (by the Files app if I understand correctly).
Thoughts?