Remove the need to call `/initialSync` in `getMessagesByUserIn`. #297

Gnuxie · 2022-05-17T11:57:51Z

At the moment we call /initialSync to give a from token to /messages.
In this PR we instead do not provide a from token when calling /messages,
which has recently been permitted in the spec
Technically this is still unstable in the spec
https://spec.matrix.org/unstable/client-server-api/#get_matrixclientv3roomsroomidmessages
matrix-org/matrix-spec#1002

Synapse has supported this for over 2 years and Element web depends on it for threads.
matrix-org/matrix-js-sdk#2065

Given that redactions are super heavy in Mjolnir already and have been reported
as barely functional on matrix.org I believe we should also adopt this approach as
if for some reason the spec did change before the next release (1.3) (extremely unlikely) we can revert this commit.

At the moment we call `/initialSync` to give a `from` token to `/messages`. In this PR we instead do not provide a `from` token when calling `/messages`, which has recently been permitted in the spec Technically this is still unstable in the spec https://spec.matrix.org/unstable/client-server-api/#get_matrixclientv3roomsroomidmessages matrix-org/matrix-spec#1002 Synapse has supported this for over 2 years and Element web depends on it for threads. matrix-org/matrix-js-sdk#2065 Given that redactions are super heavy in Mjolnir already and have been reported as barely functional on matrix.org I believe we should also adopt this approach as if for some reason the spec did change before the next release (1.3) (extremely unlikely) we can revert this commit.

Yoric · 2022-05-17T15:12:42Z

That looks pretty good!
Out of curiosity, is there a way to compare performance between two runs?

Do you need to update the README to specify Synapse version requirement?

Gnuxie · 2022-05-17T17:44:19Z

@Yoric Technically supported in Synapse since v1.48.0 (when v3 endpoints were added) so no change to the readme is required (it asks for 1.53).

As for performance, there's not an easy way to compare other than just comparing differences in response to the redact command. We also don't really know how well it is performing in the wild other than our own and the matrix.org deployment. I don't think a comparison is warranted from a technical perspective given that we used to discard the entire response from /initialSync to obtain a token to call /messages from what would almost always be the same position in the stream as now. It should be quicker and if it isn't then something terrible and absurd is going on. Quicker by how much is another question though as there are other factors to consider.

Yoric · 2022-05-18T10:26:38Z

Quicker by how much is another question though as there are other factors to consider.

We should probably consider instrumenting performance, eventually. But that would be a different PR!

Yoric

Oops, forgot to click "submit review".

Yoric · 2022-05-17T15:09:38Z

src/utils.ts

+    let token: string|null = null;
+    do {
+        const bfMessages: { chunk: any[], end?: string } = await backfill(token);
+        const lastToken: string|null = token;


Isn't that the previous token, rather than the last token?

Yoric · 2022-05-17T15:10:48Z

src/utils.ts

-            }
+    // We check that we have the token because rooms/messages is not required to provide one
+    // and will not provide one when there is no more history to paginate.
+    let token: string|null = null;


Should we store the token for next time? Or don't we ever need to do this more than once per room?

We paginate backwards, so we never want to use this token again because it is far in the past, next time we will want to be in the future. It might be an optimization to say "We should store the token to see if we get to this point next time and can finish early", but that is problematic to implement and may not work at all since there's no guarantee for you to end up in the same spot (align with really, you might be one event ahead or behind, but the next chunk is going to be 10 messages) in the stream next time.

Actually I'm just wrong

We can provide a to token i think

It still seems difficult to optimize for though

If it's difficult to optimize for, don't worry about it. At best, if you think that a comment would be useful for the next time someone ventures in this code, leave one.

It doesn't seem right putting the comment here because you can't make this optimization to getMessagesByUserIn alone. This optimization would be specific to redactions because it is valid to want to paginate into history you have "seen before" in other situations.

Yoric · 2022-05-17T15:11:16Z

src/utils.ts

-    }
-
-    function backfill(from: string) {
+    function backfill(from: string|null) {


Nit: Could you take the opportunity to document?

Yoric

Looks good to me!
Let's keep on making Mjölnir faster :)

Yoric · 2022-05-23T16:02:19Z

src/utils.ts

-            }
+    // We check that we have the token because rooms/messages is not required to provide one
+    // and will not provide one when there is no more history to paginate.
+    let token: string|null = null;


If it's difficult to optimize for, don't worry about it. At best, if you think that a comment would be useful for the next time someone ventures in this code, leave one.

Yoric · 2022-05-23T16:03:00Z

src/utils.ts

-    } else {
-        throw new Error(`Internal Error: rooms/initialSync did not return a pagination chunk for ${roomId}, this is not normal and if it is we need to stop using it. See roomInitialSync() for why we are using it.`);
-    }
+        // This check exists only becuase of a Synapse compliance bug https://github.com/matrix-org/synapse/issues/12102.


Nit: "because".

Gnuxie force-pushed the gnuxie/from-messages branch from 3732579 to 048e679 Compare May 17, 2022 12:04

Gnuxie changed the title ~~Remove need to call /initialSync in getMessagesByUserIn.~~ Remove the need to call /initialSync in getMessagesByUserIn. May 17, 2022

Gnuxie requested a review from Yoric May 17, 2022 13:46

Yoric reviewed May 19, 2022

View reviewed changes

feedback

8610971

Gnuxie requested a review from Yoric May 20, 2022 16:04

Yoric approved these changes May 23, 2022

View reviewed changes

typo

76b5689

Gnuxie merged commit 558cbb3 into main May 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove the need to call `/initialSync` in `getMessagesByUserIn`. #297

Remove the need to call `/initialSync` in `getMessagesByUserIn`. #297

Gnuxie commented May 17, 2022 •

edited

Loading

Yoric commented May 17, 2022

Gnuxie commented May 17, 2022

Yoric commented May 18, 2022

Yoric left a comment

Yoric May 17, 2022

Yoric May 17, 2022

Gnuxie May 20, 2022 •

edited

Loading

Yoric May 23, 2022

Gnuxie May 24, 2022

Yoric May 17, 2022

Yoric left a comment

Yoric May 23, 2022

Yoric May 23, 2022

Remove the need to call /initialSync in getMessagesByUserIn. #297

Remove the need to call /initialSync in getMessagesByUserIn. #297

Conversation

Gnuxie commented May 17, 2022 • edited Loading

Yoric commented May 17, 2022

Gnuxie commented May 17, 2022

Yoric commented May 18, 2022

Yoric left a comment

Choose a reason for hiding this comment

Yoric May 17, 2022

Choose a reason for hiding this comment

Yoric May 17, 2022

Choose a reason for hiding this comment

Gnuxie May 20, 2022 • edited Loading

Choose a reason for hiding this comment

Yoric May 23, 2022

Choose a reason for hiding this comment

Gnuxie May 24, 2022

Choose a reason for hiding this comment

Yoric May 17, 2022

Choose a reason for hiding this comment

Yoric left a comment

Choose a reason for hiding this comment

Yoric May 23, 2022

Choose a reason for hiding this comment

Yoric May 23, 2022

Choose a reason for hiding this comment

Remove the need to call `/initialSync` in `getMessagesByUserIn`. #297

Remove the need to call `/initialSync` in `getMessagesByUserIn`. #297

Gnuxie commented May 17, 2022 •

edited

Loading

Gnuxie May 20, 2022 •

edited

Loading