Skip to content

Improve base-32 hash decoding performance with reverse map#13680

Merged
Ericson2314 merged 1 commit intoNixOS:masterfrom
avnik:avnik/fast-base32
Aug 4, 2025
Merged

Improve base-32 hash decoding performance with reverse map#13680
Ericson2314 merged 1 commit intoNixOS:masterfrom
avnik:avnik/fast-base32

Conversation

@avnik
Copy link
Contributor

@avnik avnik commented Aug 3, 2025

Motivation

This PR replaces the linear search over nix32Chars with a constexpr-initialized reverse lookup table for decoding base-32 hashes. The changes include:

  • Defining nix32Chars as a constexpr char[].
  • Adding a constexpr std::array<unsigned char, 256> (reverseNix32Map) to map characters to their base-32 digit values at compile time.
  • Replacing the slow character search loop with a direct lookup using reverseNix32Map.
  • Removing std::once_flag/isBase32 logic in references.cc in favor of reverseNix32Map

This significantly improves performance and clarity of base-32 decoding logic, while preserving correctness.

@avnik avnik requested a review from edolstra as a code owner August 3, 2025 15:09
Copy link
Member

@Ericson2314 Ericson2314 left a comment

Choose a reason for hiding this comment

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

Thanks for this! This is indeed much better: faster and clearer.

Just one small thing, then let's land it!

The changes include:

* Defining nix32Chars as a constexpr char[].
* Adding a constexpr std::array<unsigned char, 256> (reverseNix32Map) to map characters to their base-32 digit values at compile time.
* Replacing the slow character search loop with a direct lookup using reverseNix32Map.
* Removing std::once_flag/isBase32 logic in references.cc in favor of reverseNix32Map

Signed-off-by: Alexander V. Nikolaev <avn@avnik.info>
@avnik avnik force-pushed the avnik/fast-base32 branch from cad1d68 to 4bfc007 Compare August 3, 2025 16:20
@avnik avnik requested a review from Ericson2314 August 3, 2025 16:20
@avnik
Copy link
Contributor Author

avnik commented Aug 3, 2025

@Ericson2314 your suggestion applied

@xokdvium
Copy link
Contributor

xokdvium commented Aug 3, 2025

I wouldn't mind if we could add a benchmark for this, but this could be done in a follow-up.

@avnik
Copy link
Contributor Author

avnik commented Aug 3, 2025

@xokdvium also wouldn't be bad to add dedicated tests for both nix32 encoding/decoding

@xokdvium
Copy link
Contributor

xokdvium commented Aug 3, 2025

wouldn't be bad to add dedicated tests for both nix32 encoding/decoding

Absolutely. If you'd like you could handle it in a separate PR. Any help with the test coverage would be greatly appreciated.

@Ericson2314
Copy link
Member

Note re #13682 that this PR is not supposed to affect how reference scanning works. It just affects how the parsing of hashes themselves (a Hash constructor) works, and then deduplicates the two.

Copy link
Member

@Ericson2314 Ericson2314 left a comment

Choose a reason for hiding this comment

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

I talked to @xokdvium, and we can just do the rest in a follow-up PR.

@Ericson2314 Ericson2314 merged commit 6ab8cbe into NixOS:master Aug 4, 2025
14 checks passed
@Ericson2314
Copy link
Member

We are working on this now, so don't sweat @avnik.

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