pkg/stringid: optimize GenerateRandomID

GenerateRandomID has a check to verify if the generated ID was numeric. This
check was added because a container's short-ID is used as default hostname for
containers, which isn't allowed to be consisting of only numbers (see [moby#3869]
and https://bugzilla.redhat.com/show_bug.cgi?id=1059122.

Producing an random ID with only numbers is a rare corner-case, but the check
would always be executed and wasn't optimized.

This patch applies some optimizations:

- The code was using `strconv.ParseUInt`, which has additional checks for
  signs ("+" or "-"); `hex.EncodeToString` would never produce these, so
  we can use `strconv.ParseInt` instead (which doesn't have these checks).
- The code was using `TruncateID(id)` to get the short-ID. The `TruncateID`
  function is designed to also handle digests, and for that checks for
  the given ID to contain colons (`:`), which it would split to remove
  the algorithm (`sha256:`) before truncating to the short-ID length.
  That check wasn't needed either, because those would not be produced
  by `hex.EncodeToString`, so instead, we can just truncate the ID.
- Finally, all we _really_ need to check for is if the ID consists of only
  numeric characters (`0-9`) so, let's do just that; if any non-numeric
  value is found, the ID is valid, and we can terminate the loop.

I did some basic benchmark to compare all of the above in isolation;

- BenchmarkParseInt: `strconv.ParseInt(TruncateID(id), 10, 64)`
- BenchmarkParseUInt: `strconv.ParseUint(TruncateID(id), 10, 64)`
- BenchmarkParseUIntNoTrunc: `strconv.ParseUint(id[:shortLen], 10, 64)`
- BenchmarkAllNum: `allNum(id[:shortLen])`

Results of the above:

    BenchmarkParseInt-10                1713937       691.0 ns/op     480 B/op      18 allocs/op
    BenchmarkParseIntNoTrunc-10         3385483       356.1 ns/op     480 B/op      18 allocs/op
    BenchmarkParseUInt-10               2112538       567.7 ns/op     384 B/op      12 allocs/op
    BenchmarkParseUIntNoTrunc-10        4325847       266.7 ns/op     384 B/op      12 allocs/op
    BenchmarkAllNum-10                 77277264        15.29 ns/op      0 B/op       0 allocs/op

Difference for `GenerateRandomID` as a whole is less dramatic, as in most
cases `ParseInt` would bail out early, but still saves some allocations, and
performance is ~14% better:

    BenchmarkGenerateRandomID-10        2807764       424.5 ns/op     240 B/op       6 allocs/op
    BenchmarkGenerateRandomIDNew-10     3288866       366.6 ns/op     160 B/op       3 allocs/op

[moby#3869]: https://github.com/moby/moby/issues/3869

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This commit is contained in:
Sebastiaan van Stijn
2024-10-20 16:39:29 +02:00
parent e0384101da
commit 0539b7073e
2 changed files with 53 additions and 6 deletions

View File

@@ -43,3 +43,33 @@ func TestShortenIdInvalid(t *testing.T) {
t.Fatalf("Id returned is incorrect: truncate on %s returned %s", id, truncID)
}
}
func TestAllNum(t *testing.T) {
tests := []struct {
doc, id string
expected bool
}{
{
doc: "mixed letters and numbers",
id: "4e38e38c8ce0",
expected: false,
},
{
doc: "letters only",
id: "deadbeefcafe",
expected: false,
},
{
doc: "numbers only",
id: "012345678912",
expected: true,
},
}
for _, tc := range tests {
t.Run(tc.doc, func(t *testing.T) {
if actual := allNum(tc.id); actual != tc.expected {
t.Errorf("expected %q to be %t, got %t, ", tc.id, !tc.expected, actual)
}
})
}
}