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

Improve crypto benchmarks #6164

Merged
merged 4 commits into from Aug 26, 2020
Merged

Improve crypto benchmarks #6164

merged 4 commits into from Aug 26, 2020

Conversation

jedisct1
Copy link
Contributor

  • 1MiB objects on the stack doesn't play well with wasmtime. Reduce these to 512KiB so that the webassembly benchmarks can run.
  • Pass expected results to a blackBox() function. Without this, in release-fast mode, the compiler could detected unused return values, and would produce results that didn't make sense for siphash.
  • Add AEAD constructions to the benchmarks.
  • Inline chacha20Core() makes it 4 times faster.
  • benchmarkSignatures() -> benchmarkSignature() for consistency.

@kubkon kubkon added the standard library This issue involves writing Zig code for the standard library. label Aug 25, 2020
Copy link
Collaborator

@daurnimator daurnimator left a comment

Choose a reason for hiding this comment

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

Should the benchmarks pre-warn the cpu cache/branch predictor? I recall seeing significant differences when I was benchmarking some of this code originally.

lib/std/crypto/gimli.zig Show resolved Hide resolved
@jedisct1
Copy link
Contributor Author

Should the benchmarks pre-warn the cpu cache/branch predictor? I recall seeing significant differences when I was benchmarking some of this code originally.

We perform quite a lot of iterations for each measured primitive, so it shouldn't make much of a difference.

But even if it does, I'd rather leave the benchmarks as-is, so that they are more representative of the expected performance in actual applications.

@jedisct1
Copy link
Contributor Author

Any idea of a better name for blackBox()? How about forceEval?

@data-man
Copy link
Contributor

How about forceEval?

This is already exists in std.math.

// TODO: Hide the following in an internal module.

Time is for to do it. :)

@jedisct1
Copy link
Contributor Author

black_box is the name used by criterion (in Rust) and sightglass, but this is indeed not very descriptive.

doNotOptimizeAway() is quite descriptive, but as pointed out by data-man, we already have such a function called forceEval() is std.math!

So maybe stick to that name? Otherwise, we can rename std.math's to doNotOptimizeAway() for consistency.

@jedisct1
Copy link
Contributor Author

blackBox is now to std.mem.forceEval(), that std.math.forceEval() remains an alias for.

Put a thumb up if we should keep forceEval(), and a thumb down to rename to doNotOptimizeAway (or something else) :)

@jedisct1
Copy link
Contributor Author

jedisct1 commented Aug 26, 2020

Put a thumb up if we should keep forceEval(), and a thumb down to rename to doNotOptimizeAway (or something else) :)

Alright, 100% of the votes are for the latter. 😄

Renamed!

@andrewrk
Copy link
Member

andrewrk commented Aug 26, 2020

CI failure is my fault, should be fixed in b498eeb (the good news is this was a latent bug exposed by the added safety feature introduced in 6fb105f). If you rebase and force push, it should refresh the CI run. I do want to let it run for this change to make sure the asm works on the various architectures. LLVM has bespoke inline assembly parsing for each arch.

- 1MiB objects on the stack doesn't play well with wasmtime.
Reduce these to 512KiB so that the webassembly benchmarks can run.
- Pass expected results to a blackBox() function. Without this, in
release-fast mode, the compiler could detected unused return values,
and would produce results that didn't make sense for siphash.
- Add AEAD constructions to the benchmarks.
- Inline chacha20Core() makes it 4 times faster.
- benchmarkSignatures() -> benchmarkSignature() for consistency.
@data-man
Copy link
Contributor

@jedisct1

Put a thumb up

#3597
#3600

:)

@jedisct1
Copy link
Contributor Author

@andrewrk all green!

@jedisct1
Copy link
Contributor Author

@data-man thumbupped!

@andrewrk andrewrk merged commit 091d693 into ziglang:master Aug 26, 2020
@andrewrk
Copy link
Member

andrewrk commented Aug 26, 2020

Nice work! BTW just in case you haven't seen it, we have this repo for tracking performance of various benchmarks over time:

https://github.com/ziglang/gotta-go-fast/

We don't have any crypto benchmarks yet (ziglang/gotta-go-fast#5) but that would be a nice addition.

We also don't have a fully featured way of exposing the data yet. @wozeparrot made some google charts that are nice in the meantime, but eventually I would like to have a whole page of ziglang.org that lets you browse all the benchmarks over time and reveal which commits happened and inspect other details. If you look at records.csv you can see we have a lot of unexplored data.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
standard library This issue involves writing Zig code for the standard library.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants