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

More Themis benchmarks #821

Merged
merged 22 commits into from
May 17, 2021
Merged

Conversation

G1gg1L3s
Copy link
Collaborator

@G1gg1L3s G1gg1L3s commented May 14, 2021

This PR introduces new benchmarks for the Themis core and its Rust wrapper.

Before we had only benchmarks for the Secure Cell in seal mode both with Master Key and Passphrase. I've added:

  • Secure Cell in Token Protect with Master Key
  • Secure Cell in Context Imprint with Master Key
  • Secure Message encryption/decryption
  • Secure Message signing/verification

All these benchmarks use the libthemis_sys bindings. I also added a couple of Rust wrapper benchmarks:

  • Secure Cell in Seal mode both with Master Key and Passphrase
  • Secure Cell in Token Protect with Master Key
  • Secure Cell in Context Imprint with Master Key

They are located at benches/themis/benches/secure_cell_wrapped_*.

It looks like it's a lot of changes, but if you look closely, you will see that all benches are copypasta with minor changes.

Although, I'm not sure about some moments:

  • Maybe it's necessary to adjust input sizes of some benchmarks, for example in Secure Message. I'm not sure what values should be there, I just took them from the Seal bench that was present. If you run cargo bench -- --list, you will see 212 lines of benchmarks, which takes 2-3 lives to run, so maybe some adjustments are needed.
  • Secure Message benches use big preallocated buffer, to eliminate overhead from allocations, but it may affect cache lines. So, maybe it's better to replace it with large enough buffer, but I need information about message overhead.
  • Naming of rust wrapper benches is under the question. Now, I use convention to add Wrapped word before the name of bench, but maybe there are better ways(?).

Also, no more backdoors this time!

Checklist

  • Change is covered by automated tests
    - [ ] Benchmark results are attached (if applicable)
  • The [coding guidelines] are followed
  • Public API has proper documentation
  • Example projects and code samples are up-to-date (in case of API changes)
    - [ ] Changelog is updated (in case of notable or breaking changes)

@vixentael
Copy link
Contributor

@ilammy could you please take a look?

@vixentael vixentael added infrastructure Automated building and packaging tests Themis test suite labels May 14, 2021
Copy link
Collaborator

@ilammy ilammy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awww, thanks, @G1gg1L3s! ❤️ More benchmarks are always useful and nice to have.

Nicely done, I like it.

It would also be nice to integrate some of these into CI. I believe it's already building all of them, and new Secure Cell benchmarks are already exercised, but you could exercise some Secure Message benchmarks too.

# This can take a while on the first run, it's better with caches
- name: Build benchmarks
run: |
cd benches/themis
cargo bench --no-run
# TODO: if building a pull request, compare base with updates
- name: Benchmark Secure Cell (master key)
run: |
cd benches/themis
cargo bench -- 'Secure Cell .* master key/4 KB'

It looks like it's a lot of changes, but if you look closely, you will see that all benches are copypasta with minor changes.

It's not a problem, IMO. Benchmarks aren't changing that often so duplication is okay: makes it easier to tweak individual ones, and avoids wasting enormous amounts of developer time to find a general solution, extract that into a reusable abstraction, etc.

If you run cargo bench -- --list, you will see 212 lines of benchmarks, which takes 2-3 lives to run

Well, you're not supposed to run all of them at once. Just the ones you're interested in. Though, I guess it might be helpful to look if there is a way to avoid running all of them if you do cargo bench.

Secure Message benches use big preallocated buffer [...] but it may affect cache lines.

Not really. Caches keep around the memory that is actually used, not just allocated. Criterion also doesn't do anything special with caches, so given that we're running the same loop over the same buffer, that buffer is very likely to just stay in cache after the first warm-up cycles. Crypto ops are just reading/writing memory sequentially, so they are very cache-friendly.

Naming of rust wrapper benches is under the question.

I'd call the binaries with rust prefix, as in rust_secure_cell_seal_master_key, and put something like that to the benchmark group names too, like RustThemis - Secure Cell encryption - Seal, master key. “Wrapped” is just not descriptive enough, IMO, and can be easily confused for something else.

Also, no more backdoors this time!

Fry squinting at you

I'm still as vigilant as ever.

Comment on lines +43 to +44
// Allocate buffer large enough for maximum test
let mut encrypted = vec![0; 10 * MB];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: b.iter() it the hot loop in benchmarks. It's okay to allocate and reallocate outside of that loop. You can, for example, do something like

let message = vec![0; size];
let mut encrypted = vec![0; size + 1024];

The current approach is fine too. (But you do allocate message every time.)

@ilammy
Copy link
Collaborator

ilammy commented May 15, 2021

Also, a trick question: what have you learned by observing the benchmark results and how can you explain them?

`secure_cell_wrapped -> rust_secure_cell`
`Wrapped Secure Cell -> RustThemis - Secure Cell`
Before, CI used the next regexp to filter the benches:
"Secure Cell .* passphrase/4 KB". But it matches "Secure Cell..."
as well as "RustThemis - Secure Cell...".

Replace regexp with an explicit "^Secure Cell...".
.github/workflows/test-core.yaml Outdated Show resolved Hide resolved
.github/workflows/test-core.yaml Outdated Show resolved Hide resolved
@ilammy
Copy link
Collaborator

ilammy commented May 15, 2021

Come to think of it, don't we want to put RustThemis benchmarks into benches/rust, or something? Similar to how tests are organized:

tests
├── _integration
├── check_ios_test.sh
├── common
├── objcthemis
├── phpthemis
├── pythemis
├── rbthemis
├── rust
├── soter
├── start_ios_test.sh
├── test.mk
├── themis
├── themispp
├── themispp_simple
└── tools

@vixentael, what's your opinion?

The same as a prevoius one, plus fix some quotation marks formatting
issues.
@G1gg1L3s
Copy link
Collaborator Author

And what I have observed:

  • I didn't see any difference between Seal and Token Protect. But, to my surprise, Context Imprint is a little slower. The difference is only a couple of microseconds and not always appears. In theory, it should be faster, because no MAC is calculated, but I guess this is due to the KDF that produces IV.
  • Secure Message is more interesting. I expect ECDSA to be faster on average, but actually RSA encryption/verification is 2-5 times faster. For example, what I saw is that for small messages RSA took 50 microseconds and ECDSA took 250 microseconds. On the other hand, RSA is a lot slower during decryption/signing. When encryption takes 50 microseconds, decryption takes more than 1 ms. But ECDSA still takes roughly 250 microseconds. These facts are not surprising, but it was interesting to look at numbers.
  • Rust wrappers add overhead almost indistinguishable from noise: less than microsecond for small inputs, less than 1% for big inputs. It's not surprising, because the code in benchmarks and wrappers looks almost the same, except for the one allocation, which happens in the wrappers.

@vixentael
Copy link
Contributor

Come to think of it, don't we want to put RustThemis benchmarks into benches/rust, or something?

Agree, let's put RustThemis benchmarks into a separate folder benches/rust AND add Readme to it.

benches
├── themis (themis core benchmarks)
|       ├── benches
|       └── themis core benchmarks README
└── rust (rustthemis benchmarks)
        └── rustthemis benchmarks README


@vixentael
Copy link
Contributor

vixentael commented May 16, 2021

Disregard failing Bitrise test, it's due #823

@G1gg1L3s
Copy link
Collaborator Author

I copied benches/rust/readme from benches/themis/readme, and changed only names and coverage section. Hope, it's ok.

Copy link
Contributor

@vixentael vixentael left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for changes @G1gg1L3s!

I think this PR deserves a merge. @ilammy WDYT?


## 🦀 RustThemis

Benchmarks for the Rust wrapper.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

benches/rust/README.md Show resolved Hide resolved
@vixentael vixentael requested a review from ilammy May 17, 2021 12:56
Add the goal of the RustThemis benchmarks.

Co-authored-by: vixentael <[email protected]>
@vixentael
Copy link
Contributor

Sorry @G1gg1L3s, I see some clippy warnings from CI:

https://github.com/cossacklabs/themis/pull/821/checks?check_run_id=2600921096

Screenshot 2021-05-17 at 15 57 51

Can you please fix them?

Remove some redundant copies
@G1gg1L3s
Copy link
Collaborator Author

Done! For some reason, I started to see these lints only after upgrading rust toolchain.

@vixentael
Copy link
Contributor

test passed 👏

@ilammy
Copy link
Collaborator

ilammy commented May 17, 2021

@vixentael,

@ilammy WDYT?

TMAMMHH.

@G1gg1L3s,

For some reason, I started to see these lints only after upgrading rust toolchain.

Clippy is a part of toolchain, after all. CI tracks the current stable version.

@vixentael vixentael merged commit 2bf951c into cossacklabs:master May 17, 2021
@vixentael
Copy link
Contributor

Failing bitrise test is not but part of this PR, but a temp result of #823 (as bitrise has old carthage).

As admin of this repo, I use my sudo 🧙‍♀️ powers for good and merge this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
infrastructure Automated building and packaging tests Themis test suite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants