Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PM-6107] Use rayon to multithread encryption/decryption #215

Merged
merged 1 commit into from
Feb 8, 2024

Conversation

dani-garcia
Copy link
Member

Type of change

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

Objective

Use rayon to multithread the encryption and decryption of ciphers. Note that this will have no benefit for WASM, as in that case it falls back to a single thread model.

In my Mac I get 8-9x speedups when decrypting a big number of EncStrings.

@dani-garcia dani-garcia requested a review from Hinton September 1, 2023 16:28
@bitwarden-bot
Copy link

bitwarden-bot commented Sep 1, 2023

Logo
Checkmarx One – Scan Summary & Detailsf122f2cb-e65b-4607-8059-53aabf55ae61

New Issues

Severity Issue Source File / Package Checkmarx Insight
MEDIUM Unpinned Actions Full Length Commit SHA /build-cli-docker.yml: 151 Pinning an action to a full length commit SHA is currently the only way to use an action as an immutable release. Pinning to a particular SHA helps...
MEDIUM Unpinned Actions Full Length Commit SHA /build-cli-docker.yml: 75 Pinning an action to a full length commit SHA is currently the only way to use an action as an immutable release. Pinning to a particular SHA helps...
MEDIUM Unpinned Actions Full Length Commit SHA /build-cli-docker.yml: 68 Pinning an action to a full length commit SHA is currently the only way to use an action as an immutable release. Pinning to a particular SHA helps...

Fixed Issues

Severity Issue Source File / Package
MEDIUM Unpinned Actions Full Length Commit SHA /release-cli.yml: 213
MEDIUM Unpinned Actions Full Length Commit SHA /build-cli-docker.yml: 63
MEDIUM Unpinned Actions Full Length Commit SHA /build-cli-docker.yml: 70
MEDIUM Unpinned Actions Full Length Commit SHA /build-cli-docker.yml: 146

@dani-garcia
Copy link
Member Author

Done some more benchmarking, both running the native binary, WASM, and the iOS emulator, and with two different types of data:

  • One big array with 2 million EncStrings, each with 20 characters
  • One smaller array of objects that implement Encryptable and each contain 14 EncStrings, the total size of the array is 2 million / 14 to keep the total EncStrings the same between both. The goal of this test is to check that nesting parallel calls works as expected.
{
    name: EncString,
    password: EncString,
    inner: [
        {
            name: EncString,
            password: EncString,
            inner: [
                {
                    name: EncString,
                    password: EncString,
                    inner: []
                },
                {
                    name: EncString,
                    password: EncString,
                    inner: []
                }
            ]
        },
        ... another one, same as above
    ]
}

On all the benchmarks both data types performed identically, which tells us that there is no overhead when nesting parallel iterator calls.

The results for the different targets are as follows:

NATIVE:
Secuential: 4s
Parallel: 430-470ms
Improvement: 8-9x

WASM:
Secuential: 16-19s
Parallel: 16-19s
No improvement, because rayon doesn't support WASM threads. Though we can look into: https://github.com/GoogleChromeLabs/wasm-bindgen-rayon

iOS:
Secuential: 4.6s
Parallel: 500-600ms
Improvement: 8-9x

This is run on a M2Pro Mac with 8 performance cores and 4 efficiency cores.

@Hinton Hinton changed the title Use rayon to multithread encryption/decryption [PM-6107] Use rayon to multithread encryption/decryption Feb 6, 2024
@dani-garcia dani-garcia force-pushed the ps/multithreaded-encryption branch from 3e3be02 to e515bc5 Compare February 6, 2024 17:45
Copy link

codecov bot commented Feb 6, 2024

Codecov Report

Attention: 16 lines in your changes are missing coverage. Please review.

Comparison is base (714c2d4) 57.76% compared to head (e515bc5) 57.72%.
Report is 1 commits behind head on main.

Files Patch % Lines
crates/bitwarden-crypto/src/encryptable.rs 0.00% 8 Missing ⚠️
...rates/bitwarden-crypto/src/keys/key_encryptable.rs 0.00% 8 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #215      +/-   ##
==========================================
- Coverage   57.76%   57.72%   -0.05%     
==========================================
  Files         168      168              
  Lines        9912     9920       +8     
==========================================
  Hits         5726     5726              
- Misses       4186     4194       +8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@dani-garcia dani-garcia merged commit 9399346 into main Feb 8, 2024
58 of 59 checks passed
@dani-garcia dani-garcia deleted the ps/multithreaded-encryption branch February 8, 2024 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants