Skip to content

Test speedup#1678

Merged
phoerious merged 3 commits intokeepassxreboot:developfrom
luzat:chore-test-speedup
Mar 8, 2018
Merged

Test speedup#1678
phoerious merged 3 commits intokeepassxreboot:developfrom
luzat:chore-test-speedup

Conversation

@luzat
Copy link
Copy Markdown
Contributor

@luzat luzat commented Mar 8, 2018

Description

This speeds up the slowest tests, AutoType and Kdbx4, by adjusting some parameters which should not influence the tested semantics.

Motivation and context

Tests are slow. It's more fun to write code with faster test suites.

How has this been tested?

I ran tests on Debian GNU/Linux on an i7-6700K with ASAN on and off. The times went down from approximately 9.71s and 8.72s respectively to 4.38s and 3.08s for a make test. TestKdbx4 is still slow (still lots of cryptography), but TestAutoType is almost negligible.

Types of changes

  • Speed optimization

Checklist:

  • ✅ I have read the CONTRIBUTING document. [REQUIRED]
  • ✅ My code follows the code style of this project. [REQUIRED]
  • ✅ All new and existing tests passed. [REQUIRED]
  • ✅ I have compiled and verified my code with -DWITH_ASAN=ON. [REQUIRED]

luzat added 2 commits March 8, 2018 01:25
Decrease default autotype delay to 1 to improve test suite speed by
seconds. This shaves multiple seconds off the whole test suite. In some
cases, the largest part.

Also, initialize config just creating the test instance, just in case
that it ever depends on the configuration values at that point already.
This speeds up the Kdbx4 tests by using parameters optimized for speed
for the key derivation functions. On an i7-6700K the tests run close to
50% faster with this change (about 1.5s vs. 3s).
@droidmonkey
Copy link
Copy Markdown
Member

What is really slow is the ridiculous number of test executables we have to link. Good work on these!

kdf->setRounds(1);
kdf->processParameters({
{KeePass2::KDFPARAM_ARGON2_MEMORY, 1024},
{KeePass2::KDFPARAM_ARGON2_PARALLELISM, 1}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You cannot just use Argon2 unconditionally. The test must not make assumptions about which part of changing the KDF triggers the KDBX 4 upgrade. Imagine we introduce a bug where only Argon2 triggers the upgrade, but not AES-KDF. With these changes we would never notice.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This does not actually use Argon2, but iff Argon2 is used, then the parameters are applied. AesKdf ignores these, but the cipher is not changed.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Actually, thinking more about it... You are just setting the parameters and not changing the actual KDF. But I would still prefer you add a check here.

Copy link
Copy Markdown
Member

@phoerious phoerious left a comment

Choose a reason for hiding this comment

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

Thanks. Looks good to me.

@phoerious phoerious merged commit 46e8e3d into keepassxreboot:develop Mar 8, 2018
Piraty pushed a commit to Piraty/keepassxc that referenced this pull request Mar 14, 2018
* Tests: Speed up AutoType testing

Decrease default autotype delay to 1 to improve test suite speed by
seconds. This shaves multiple seconds off the whole test suite. In some
cases, the largest part.

Also, initialize config just creating the test instance, just in case
that it ever depends on the configuration values at that point already.

* Tests: Speed up Kdbx4 testing

This speeds up the Kdbx4 tests by using parameters optimized for speed
for the key derivation functions. On an i7-6700K the tests run close to
50% faster with this change (about 1.5s vs. 3s).
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