Skip to content

avoid compiling regular expressions for algorithms at init - #122

Merged
sudo-bmitch merged 1 commit into
opencontainers:masterfrom
mvdan:init-cost
Oct 15, 2026
Merged

sudo-bmitch merged 1 commit into
opencontainers:masterfrom
mvdan:init-cost

Conversation

@mvdan

@mvdan mvdan commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor

avoid compiling regular expressions for algorithms at init
RegisterAlgorithm compiled a regular expression for each algorithm,
such as ^[a-f0-9]{64}$ for SHA256, and the large repetition counts made
these costly to compile. Since Validate already checks the length,
checking for lowercase hex digits is enough.

algorithmRegexp was also compiled at init time for RegisterAlgorithm;
replace it with an equivalent hand-written check.

As measured by https://github.com/mvdan/benchinit:

                                │     old      │                 new                 │
                                │    sec/op    │   sec/op     vs base                │
GithubComOpencontainersGoDigest   117.57µ ± 1%   55.07µ ± 3%  -53.16% (p=0.000 n=10)

                                │      old      │                 new                  │
                                │     B/op      │     B/op      vs base                │
GithubComOpencontainersGoDigest   73.258Ki ± 0%   9.586Ki ± 0%  -86.91% (p=0.000 n=10)

                                │    old     │                new                 │
                                │ allocs/op  │ allocs/op   vs base                │
GithubComOpencontainersGoDigest   687.0 ± 0%   126.0 ± 0%  -81.66% (p=0.000 n=10)

Validate, which used the per-algorithm regular expressions,
gets faster as well, as shown by a new benchmark:

            │      old      │                new                 │
            │    sec/op     │   sec/op     vs base               │
Validate-24   435.25n ± 21%   65.10n ± 4%  -85.04% (p=0.000 n=8)

@mvdan

mvdan commented Oct 12, 2026

Copy link
Copy Markdown
Contributor Author

cc @thaJeztah @AkihiroSuda this cuts init time cost by half :)

@AkihiroSuda
AkihiroSuda requested a balanced review from Copilot October 13, 2026 00:10
@AkihiroSuda

Copy link
Copy Markdown
Member

Needs DCO

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

Digest validation now repeatedly computes hash size, causing a hot-path regression for BLAKE3.

1 open finding
What changed in this PR

Replaces initialization-time regular expressions with direct validation logic.

Changes:

  • Adds manual algorithm-name and lowercase-hex validation.
  • Simplifies registered algorithm state.
  • Adds registration cases and validation benchmark coverage.
File Description
algorithm.go Replaces regex validation with character checks.
algorithm_test.go Expands algorithm-name test cases.
digest_test.go Adds a digest validation benchmark.

🧠 Review effort: Balanced


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread algorithm.go Resolved
@mvdan

mvdan commented Oct 13, 2026

Copy link
Copy Markdown
Contributor Author

Needs DCO

Oops, fixed.

@AkihiroSuda

Copy link
Copy Markdown
Member

RegisterAlgorithm compiled a regular expression for each algorithm,
such as ^[a-f0-9]{64}$ for SHA256, and the large repetition counts made
these costly to compile. Since Validate already checks the length,
checking for lowercase hex digits is enough.

algorithmRegexp was also compiled at init time for RegisterAlgorithm;
replace it with an equivalent hand-written check.

As measured by https://github.com/mvdan/benchinit:

                                    │     old      │                 new                 │
                                    │    sec/op    │   sec/op     vs base                │
    GithubComOpencontainersGoDigest   117.57µ ± 1%   55.07µ ± 3%  -53.16% (p=0.000 n=10)

                                    │      old      │                 new                  │
                                    │     B/op      │     B/op      vs base                │
    GithubComOpencontainersGoDigest   73.258Ki ± 0%   9.586Ki ± 0%  -86.91% (p=0.000 n=10)

                                    │    old     │                new                 │
                                    │ allocs/op  │ allocs/op   vs base                │
    GithubComOpencontainersGoDigest   687.0 ± 0%   126.0 ± 0%  -81.66% (p=0.000 n=10)

Validate, which used the per-algorithm regular expressions,
gets faster as well, as shown by a new benchmark:

                │      old      │                new                 │
                │    sec/op     │   sec/op     vs base               │
    Validate-24   435.25n ± 21%   65.10n ± 4%  -85.04% (p=0.000 n=8)

Signed-off-by: Daniel Martí <mvdan@mvdan.cc>
@mvdan

mvdan commented Oct 13, 2026

Copy link
Copy Markdown
Contributor Author

Yep, should be fixed now...

@mvdan

mvdan commented Oct 15, 2026

Copy link
Copy Markdown
Contributor Author

@AkihiroSuda friendly nudge :) is there anything else I should do here?

@tianon

tianon commented Oct 15, 2026

Copy link
Copy Markdown
Member

I'm not a maintainer here, but this was discussed in today's OCI call and I just want to point out that this new function is validating the set of allowable characters for algorithms, not the actual list of supported algorithms, so the likelihood of that list ever changing is essentially zero and this is pretty much a free speedup. 👍

@tianon

tianon commented Oct 15, 2026

Copy link
Copy Markdown
Member

(and benchmarks are in the commit message)

@thaJeztah

Copy link
Copy Markdown
Member

Oh! I saw this one fly by my notifications and wanted to look. Thanks @mvdan good seeing you here (big fan of your work and modules ❤️)

(and benchmarks are in the commit message)

I took the liberty of copy/pasting it in the PR description (GitHub still doesn't understand that commit messages are part of the PR, and just as relevant, if not more, than the code changes).

I just want to point out that this new function is validating the set of allowable characters for algorithms, not the actual list of supported algorithms, so the likelihood of that list ever changing is essentially zero and this is pretty much a free speedup. 👍

Yup! And the replacement (IMO) is readable enough to make it a good replacement for the regex.

Of many "bad" choices in this module; at least a good decision was to not export the regex, so 🎉 good to change the implementation.

Which reminds me, I still want to try if the Go maintainers are open to offering lazyregexp as part of stdlib. Regexes are (ab)used in many places, and having to pay the cost of a package-level regexp.MustCompile (even if not used in your codebase) is still a pain. Rewrites like this are not a luxury every project has (sometimes a regex is the most suitable choice), and while it's not rocket-science to implement a lazyregexp, it also has enough potential for footguns to do it wrong (and lose the benefits of MustCompile). Having lazyregexp in stdlib would make it a much easier "drive-by" pull request to make a project switch than dumping the maintenancce burden of a self-created one.

@thaJeztah thaJeztah left a comment

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.

(that's an LGTM from me)

thanks!

@thaJeztah

Copy link
Copy Markdown
Member

@sudo-bmitch wanted to look at this one, or good to go?

@sudo-bmitch

Copy link
Copy Markdown
Contributor

@sudo-bmitch wanted to look at this one, or good to go?

It's in my queue to look over, but not enough to block the merge.

@sudo-bmitch
sudo-bmitch merged commit 63db542 into opencontainers:master Oct 15, 2026
7 checks passed
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.

6 participants