clients
clients copied to clipboard
bitwarden
[PM-4154] Improve unlock time by making MultithreadEncryptServiceImplementation spawn multiple workers to use all system threads
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.hardwareConcurrencyworkers 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
Thank you for your contribution! We've added this to our internal Community PR board for review. ID: PM-4154
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?
Uhh, sorry about the pings, messed up a rebase there and the bots picked it up and marked it for review.
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.
Checkmarx One – Scan Summary & Details – 710225c1-f4cd-4e48-8840-528fb86b8efc
No New Or Fixed Issues Found
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.
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.
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.
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.
@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.
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.
@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?
@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
@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.)
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.)
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.
@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.
@ccben87 I plan to get back to this PR as soon as I find time, though that is probably in middle of May.
On initial check, there is a new/different dependency cycle now. ConfigApiService needs the TokenService, which needs the EncryptService which needs the ConfigApiService.
Changed to "Bulk Encrypt Service" now and gated the feature behind a feature flag. The plan is to remove decryptItems from EncryptService, multithread-encrypt.service.implementation.ts, BrowserFallbackEncryptServiceImplementation, BrowserMultithreadEncryptServiceImplementation entirely, once the feature has been rolled out and tested, leading to an overall less complex implementation. Right now there is a lot of duplication between the old code and new code, since I'd rather be very defensive when rolling out a feature on vault decryption.
@quexten have you done any perf analysis on this? I am curious what gains we might see at various vault sizes.
@MGibson1 originally asked me to review this as a community PR because I was familiar with the multithread stuff. However, the recurring blocker here has been circular dependencies between our services, which is impacted by Platform's work and vision for our services structure. Now that this PR is making further changes to encryption-related services, I'd like to kick it back to Platform (@justindbaur) so that they can review it directly and make sure they're happy with it. I fear my lack of code ownership is just holding this up.
PS. #9077 is merged :)
@willmartian While not extensive (just one run using performance.now, for a 20k cipher/80k cipherstring vault, in Firefox) the results are fairly clear. On an Apple M3 Pro: Single worker: 4.5s, bulk service: 1.3s. On an Intel i7-1185G7, on power-saver: ~21s to 8.5s, without powersaver 9s to 3.3s.
I can get more accurate measurements, but I think the benefit is pretty clear from these alone. This is just for the call to decryptItems. Whole unlock time ofcourse also includes KDF time, state parsing, and search indexing.
Thanks for the update @eliykat, since #9077 was merged, I have now removed the factory methods, bringing this PR to a more reasonable size.
I can get more accurate measurements
@quexten No need, just wanted a rough picture. This is great, thank you very much!