Reduce Megolm blocking and add cancellation #3035

clarkf · 2023-01-08T23:09:26Z

This was originally intended to help with element-hq/element-web#21612, but ended up addressing #1255 as well.

The input delay was addressed by adding a setTimeout to the cpu-bound portion of the code. That should allow rendering code and event handlers to preempt key preparation periodically. Should help somewhat, but probably not a full solution. Ideally, setImmediate should be used or approximated, but it's basically not supported in any browser, so 0ms timeouts it is.

That allowed for cancellation to be addressed. There are multiple ways this could have been achieved, and I wasn't able to find a clear precedent. I opted for passing around stateful closures. It had the most minimal impact on the API that I could come up with.

I've tried my best to follow the contribution guidelines and code style documentation, but this is my first (substantial) contribution. Any and all feedback welcome!

Checklist

Tests written for new code (and old code if feasible)
Linter and other CI checks pass
Sign-off given on the changes (see CONTRIBUTING.md)

Notes: Introduces a backwards-compatible API change. MegolmEncrypter#prepareToEncrypt's return type has changed from void to () => void.

Here's what your changelog entry will look like:

✨ Features

Introduces a backwards-compatible API change. MegolmEncrypter#prepareToEncrypt's return type has changed from void to () => void. (#3035). Contributed by @clarkf.

robintown

The changes look pretty straightforward, thanks.

For the record, you're actually free to use setImmediate here, since any time the js-sdk gets used in a browser, it invariably gets run through Babel which has polyfills for that.

robintown · 2023-01-16T03:29:55Z

src/crypto/algorithms/megolm.ts

     */
-    public prepareToEncrypt(room: Room): void {
+    public prepareToEncrypt(room: Room): Stop {


I would prefer to just write out () => void instead of Stop, since the doc comments already describe well enough what the return value does, and when the types are this simple, aliases tend to only add confusion.

src/crypto/algorithms/megolm.ts

robintown · 2023-01-16T05:01:56Z

src/crypto/algorithms/megolm.ts

+                // Yield prior to checking each device so that we don't block
+                // updating/rendering for too long.
+                // See https://github.com/vector-im/element-web/issues/21612
+                await sleep(0);


While I see how this would unblock the event loop and fix the linked issue, I also want to make sure this won't create a performance regression in encryptMessage. It's quite possible that there will be a lot of devices processed here, and the HTML spec claims that after 5 nested calls, any further setTimeout calls are clamped to a minimum delay, which could add up.

Perhaps it would be more appropriate for the function to only yield on cancelable operations?

Suggested change

await sleep(0);

if (isCancelled !== undefined) await sleep(0);

Interesting, I'd never heard about the minimum delay!

Swapping to setImmediate, since it's polyfilled, which should obviate the problem. Seems core-js provides a MessageChannel-based implementation, only falling back to setTimeout on browsers that aren't targeted.

Also added the conditional!

Adds an async/promise-based version of `setImmediate`. Note that, despite being poorly adopted, `setImmediate` is polyfilled, and should be more performant than `sleep(0)`. Signed-off-by: Clark Fischer <[email protected]>

Currently, calling `Client#prepareToEncrypt` in a megolm room has the potential to block for multiple seconds while it crunches numbers. Sleeping for 0 seconds (approximating `setImmediate`) allows the engine to process other events, updates, or re-renders in between checks. See - element-hq/element-web#21612 - element-hq/element-web#11836 Signed-off-by: Clark Fischer <[email protected]>

NOTE: This commit introduces a backwards-compatible API change. Adds the ability to cancel `MegolmEncryption#prepareToEncrypt` by returning a cancellation function. The bulk of the processing happens in `getDevicesInRoom`, which now accepts a 'getter' that allows the caller to indicate cancellation. See matrix-org#1255 Closes matrix-org#1255 Signed-off-by: Clark Fischer <[email protected]>

robintown

Sorry for the wait, this looks good to land now 👍

@kerryarchibald

* Element-R: implement encryption of outgoing events ([\matrix-org#3122](matrix-org#3122)). * Poll model - page /relations results ([\matrix-org#3073](matrix-org#3073)). Contributed by @kerryarchibald. * Poll model - validate end events ([\matrix-org#3072](matrix-org#3072)). Contributed by @kerryarchibald. * Handle optional last_known_event_id property in m.predecessor ([\matrix-org#3119](matrix-org#3119)). Contributed by @andybalaam. * Add support for stable identifier for fixed MAC in SAS verification ([\matrix-org#3101](matrix-org#3101)). * Provide eventId as well as roomId from Room.findPredecessor ([\matrix-org#3095](matrix-org#3095)). Contributed by @andybalaam. * MSC3946 Dynamic room predecessors ([\matrix-org#3042](matrix-org#3042)). Contributed by @andybalaam. * Poll model ([\matrix-org#3036](matrix-org#3036)). Contributed by @kerryarchibald. * Remove video tracks on video mute without renegotiating ([\matrix-org#3091](matrix-org#3091)). * Introduces a backwards-compatible API change. `MegolmEncrypter#prepareToEncrypt`'s return type has changed from `void` to `() => void`. ([\matrix-org#3035](matrix-org#3035)). Contributed by @clarkf. * Stop the ICE disconnected timer on call terminate ([\matrix-org#3147](matrix-org#3147)). * Clear notifications when we can infer read status from receipts ([\matrix-org#3139](matrix-org#3139)). Fixes element-hq/element-web#23991. * Messages sent out of order after one message fails ([\matrix-org#3131](matrix-org#3131)). Fixes element-hq/element-web#22885 and element-hq/element-web#18942. Contributed by @justjanne. * Element-R: fix a bug which prevented encryption working after a reload ([\matrix-org#3126](matrix-org#3126)). * Element-R: Fix invite processing ([\matrix-org#3121](matrix-org#3121)). * Don't throw with no `opponentDeviceInfo` ([\matrix-org#3107](matrix-org#3107)). * Remove flaky megolm test ([\matrix-org#3098](matrix-org#3098)). Contributed by @clarkf. * Fix "verifyLinks" functionality of getRoomUpgradeHistory ([\matrix-org#3089](matrix-org#3089)). Contributed by @andybalaam.

clarkf requested a review from a team as a code owner January 8, 2023 23:09

clarkf requested review from florianduros and robintown January 8, 2023 23:09

github-actions bot added the Z-Community-PR Issue is solved by a community member's PR label Jan 8, 2023

github-actions bot deployed to PR Documentation Preview January 8, 2023 23:12 View deployment

robintown self-assigned this Jan 12, 2023

robintown removed the request for review from florianduros January 16, 2023 03:22

robintown removed their assignment Jan 16, 2023

robintown reviewed Jan 16, 2023

View reviewed changes

clarkf added 3 commits January 16, 2023 10:11

Add async setImmediate util

ddce1bc

Adds an async/promise-based version of `setImmediate`. Note that, despite being poorly adopted, `setImmediate` is polyfilled, and should be more performant than `sleep(0)`. Signed-off-by: Clark Fischer <[email protected]>

clarkf force-pushed the megolm-cancellation branch from 7fb2b39 to 1ee487a Compare January 16, 2023 18:25

github-actions bot deployed to PR Documentation Preview January 16, 2023 18:28 View deployment

robintown self-requested a review January 18, 2023 13:42

robintown approved these changes Jan 24, 2023

View reviewed changes

robintown added the T-Enhancement label Jan 24, 2023

Merge branch 'develop' into megolm-cancellation

40a4c8d

robintown enabled auto-merge January 24, 2023 17:20

github-actions bot deployed to PR Documentation Preview January 24, 2023 17:22 View deployment

robintown merged commit 70656e9 into matrix-org:develop Jan 24, 2023

clarkf deleted the megolm-cancellation branch January 24, 2023 18:58

clarkf mentioned this pull request Jan 25, 2023

Remove flaky megolm test #3098

Merged

3 tasks

uhoreg mentioned this pull request May 19, 2023

Messages get completely stuck at "Encrypting your message" element-hq/element-web#24612

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduce Megolm blocking and add cancellation #3035

Reduce Megolm blocking and add cancellation #3035

clarkf commented Jan 8, 2023 •

edited by github-actions bot

Loading

robintown left a comment

robintown Jan 16, 2023

robintown Jan 16, 2023

clarkf Jan 16, 2023 •

edited

Loading

robintown left a comment

	await sleep(0);
	if (isCancelled !== undefined) await sleep(0);

Reduce Megolm blocking and add cancellation #3035

Reduce Megolm blocking and add cancellation #3035

Conversation

clarkf commented Jan 8, 2023 • edited by github-actions bot Loading

Checklist

✨ Features

robintown left a comment

Choose a reason for hiding this comment

robintown Jan 16, 2023

Choose a reason for hiding this comment

robintown Jan 16, 2023

Choose a reason for hiding this comment

clarkf Jan 16, 2023 • edited Loading

Choose a reason for hiding this comment

robintown left a comment

Choose a reason for hiding this comment

clarkf commented Jan 8, 2023 •

edited by github-actions bot

Loading

clarkf Jan 16, 2023 •

edited

Loading