[PM-4154] Improve unlock time by making MultithreadEncryptServiceImplementation spawn multiple workers to use all system threads

[quexten]

Type of change

- [ ] Bug fix
- [x] New feature development
- [ ] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
- [ ] Build/deploy pipeline (DevOps)
- [ ] Other

Objective

MultithreadEncryptServiceImplementation currently uses one background worker to offload decryption to another thread to not block the UI. This PR makes it so that multiple (navigator.hardwareConcurrency) workers are spawned in order to make use of all available CPU resources.

This improves unlock time for very large vaults.

Code changes

• multithread-encrypt.service.implementation.ts: Spawn navigator.hardwareConcurrency workers and distribute the Decryptables onto the workers. Then await all promises and merge results.

Screenshots

Before you submit

• Please add unit tests where it makes sense to do so (encouraged but not required)
• If this change requires a documentation update - notify the documentation team
• If this change has particular deployment requirements - notify the DevOps team
• Ensure that all UI additions follow WCAG AA requirements

[bitwarden-bot]

Thank you for your contribution! We've added this to our internal Community PR board for review. ID: PM-4154

[quexten]

Partially fixes: https://github.com/bitwarden/clients/issues/1597

[quexten]

Hmm, so I can't inject the configservice to the encrypt service, because it depends on the authservice, which depends on the cryptoservice, which depends on the encrypt service, causing a dependency cycle. I don't see a good way to break this dependency cycle.

For the previous background thread "multithread-encrypt" service feature and for cipherkey encryption, it was done via this facility instead: https://github.com/bitwarden/clients/blob/6b7edced8e66b23855bea2d478536f83931a54cf/libs/common/src/platform/misc/flags.ts#L3-L7

Though this seems to be more for build-time configuration, not run-time server toggleable features. So this might not be optimal for rolling back in production?

Any advice on which path to take forward here?

[CLAassistant]

All committers have signed the CLA.

[quexten]

Uhh, sorry about the pings, messed up a rebase there and the bots picked it up and marked it for review.

[quexten]

at the moment, we take 1 thread with a TTL of 3 minutes. This was not a particularly disciplined approach (by me) because 1 inactive thread doesn't tie up many system resources. But if we're taking the max threads available to us, I assume we need to be more aggressive about freeing up system resources once we're done. I don't have any firm ideas here, can we just spin them up & terminate them every time the method is called? Or shoudl we have a shorter TTL which more accurately reflects inactivity? (at the moment the TTL starts counting when work is sent to the worker, not when the worker becomes inactive, it's not particularly robust)

Changed ttl to 1 minute, but changed the behaviour so that the ttl is disabled during decryption. Also added a limit of min(system threads, 16) to the number of workers, and a minimum of 500 items.

[bitwarden-bot]

Checkmarx One – Scan Summary & Details – e8bb45b1-7ea8-4243-a4b1-2311d2e1e4bd

No New Or Fixed Issues Found

[eliykat]

Hmm, so I can't inject the configservice to the encrypt service, because it depends on the authservice, which depends on the cryptoservice, which depends on the encrypt service, causing a dependency cycle. I don't see a good way to break this dependency cycle.

There is an upcoming refactor that might help here, I'll check in with the team responsible. Otherwise we could evaluate the flag in CipherService and pass the boolean as an argument when calling decryptItems, but I'd prefer not to if we can avoid it.

For the previous background thread "multithread-encrypt" service feature and for cipherkey encryption, it was done via this facility instead:

https://github.com/bitwarden/clients/blob/6b7edced8e66b23855bea2d478536f83931a54cf/libs/common/src/platform/misc/flags.ts#L3-L7

Though this seems to be more for build-time configuration, not run-time server toggleable features. So this might not be optimal for rolling back in production?

You've understood correctly, that is no longer used and should be cleaned up.

[quexten]

There is an upcoming refactor that might help here, I'll check in with the team responsible. Otherwise we could evaluate the flag in CipherService and pass the boolean as an argument when calling decryptItems, but I'd prefer not to if we can avoid it.

Is there a public PR for this change that I can keep an eye on at the moment or this internally tracked work for now?

If the refactor is a more long-term initiative, I'm happy to change it to pass this flag as an argument for now with a note to remove it as soon as is viable.

[torsina]

Shouldn't the number of Decryptable per worker be made an option in the client ? Different users have different vault sizes with different hardware capabilities and different needs.

[quexten]

To be honest, I'd like to avoid adding user configurable options unless absolutely needed.

I'd rather in the next step make a decision on good values based on empirical data. Next, there are other, further gains that can be had in decryption, that, combined with this change do not add user configuration complexity, but make the decryption time so fast that it's not noticeable.

[ccben87]

@eliykat Is there somewhere we can track the previously mentioned upcoming refactor? I'm really keen to see this get implemented as the performance impact we are seeing with over 11k ciphers is significantly impacting us.

[eliykat]

Thanks for your patience, @quexten our circular dependency issues should be resolved by #8279.

@ccben87 please note that this code is only used by the web vault and browser extension clients, it's not (yet?) used by the desktop client and is totally separate from mobile. Just FYI in case you're using one of those other clients.

[ccben87]

@eliykat Thanks for replying! We mostly use the web vault and browser extension. However, I think some may use desktop client. Where is the code for the desktop client or what do you mean by not yet used by desktop client? Will it be used by desktop client at some point?

[quexten]

@eliykat Thanks for replying! We mostly use the web vault and browser extension. However, I think some may use desktop client. Where is the code for the desktop client or what do you mean by not yet used by desktop client? Will it be used by desktop client at some point?

While I have not investigated the relevant code yet, the multi-thread (background thread) decryption service, which this PR extends, is disabled at build time in the desktop client: https://github.com/bitwarden/clients/blob/bcb2a976b094f57f1f7e1261e2692f12103d7b16/apps/desktop/config/base.json#L4

[eliykat]

@ccben87 The desktop client has a race condition when quickly switching between large accounts - its state can get out of sync between accounts because it doesn't force you to wait for the account switch to fully settle. e.g. if you change accounts rapidly, it might try to decrypt the new vault items with the old encryption key. It just so happens that this isn't a problem if you're not using multithreading, because decryption will lock up the UI 😅

Browser avoids this because its implementation is more robust, it forces you to wait until the selected account is "ready"/settled before it lets you interact with the UI again. So we need to implement a similar thing for desktop, but I'm not sure when that will be. (I have mentioned it to that team just now to make sure they're aware.)

[eliykat]

Update to my previous comment - I've followed up internally and the team in charge of account switching is going to schedule some work for the Desktop client to address this, which should then allow us to enable multithread decryption there as well. (No ETA yet but hopefully soon.)

[quexten]

Thanks for your patience, @quexten our circular dependency issues should be resolved by #8279.

@ccben87 please note that this code is only used by the web vault and browser extension clients, it's not (yet?) used by the desktop client and is totally separate from mobile. Just FYI in case you're using one of those other clients.

For any other community readers not keeping close eyes on related PRs, instead of #8279 , this is now waiting on #8325

For the desktop multi-threading, I don't think there is anything that this PR needs to do, so I don't think that's a blocker?

Regardless, will re-base and add the feature flag once #8325 is merged.

[MGibson1]

All cleared up! sorry for delaying you all 🙇

[ccben87]

@quexten Is this still waiting on you or is it ready to be merged? If there's still work to be done, I could take a look at it but I'm new to Typescript.

[quexten]

@ccben87 I plan to get back to this PR as soon as I find time, though that is probably in middle of May.