Skip to content

Reduce allocations in Parse and Format #6

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.

Sign up for GitHub

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

Merged
benbardin merged 1 commit into from
Feb 23, 2026
Merged

Reduce allocations in Parse and Format #6

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
172 changes: 95 additions & 77 deletions version.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,63 @@ const (
adhoc = releasePhase(6)
)

var parsePatterns = []*regexp.Regexp{
// these are roughly in "how often we expect to see them" order
regexp.MustCompile(`^v(?P<year>[1-9][0-9]*)\.(?P<ordinal>[1-9][0-9]*)\.(?P<patch>(?:[1-9][0-9]*|0))(?:-fips)?$`),
regexp.MustCompile(`^v(?P<year>[1-9][0-9]*)\.(?P<ordinal>[1-9][0-9]*)\.(?P<patch>(?:[1-9][0-9]*|0))-(?P<phase>alpha|beta|rc|cloudonly)\.(?P<phaseOrdinal>[0-9]+)(?:-fips)?$`),
regexp.MustCompile(`^v(?P<year>[1-9][0-9]*)\.(?P<ordinal>[1-9][0-9]*)\.(?P<patch>(?:[1-9][0-9]*|0))-(?P<customOrdinal>(?:[1-9][0-9]*|0))-g[a-f0-9]+(?:-fips)?$`),
regexp.MustCompile(`^v(?P<year>[1-9][0-9]*)\.(?P<ordinal>[1-9][0-9]*)\.(?P<patch>(?:[1-9][0-9]*|0))-(?P<phase>alpha|beta|rc|cloudonly)\.(?P<phaseOrdinal>[0-9]+)-(?P<customOrdinal>(?:[1-9][0-9]*|0))-g[a-f0-9]+(?:-fips)?$`),
regexp.MustCompile(`^v(?P<year>[1-9][0-9]*)\.(?P<ordinal>[1-9][0-9]*)\.(?P<patch>(?:[1-9][0-9]*|0))-(?P<phase>alpha|beta|rc|cloudonly)\.(?P<phaseOrdinal>[0-9]+)-cloudonly(-rc|\.)(?P<phaseSubOrdinal>(?:[1-9][0-9]*|0))$`),
regexp.MustCompile(`^v(?P<year>[1-9][0-9]*)\.(?P<ordinal>[1-9][0-9]*)\.(?P<patch>(?:[1-9][0-9]*|0))-(?P<phase>cloudonly)-rc(?P<phaseOrdinal>[0-9]+)$`),
regexp.MustCompile(`^v(?P<year>[1-9][0-9]*)\.(?P<ordinal>[1-9][0-9]*)\.(?P<patch>(?:[1-9][0-9]*|0))-(?P<phase>cloudonly)(?P<phaseOrdinal>[0-9]+)?$`),

// vX.Y.Z-<anything> will sort after the corresponding "plain" vX.Y.Z version
regexp.MustCompile(`^v(?P<year>[1-9][0-9]*)\.(?P<ordinal>[1-9][0-9]*)\.(?P<patch>(?:[1-9][0-9]*|0))-(?P<adhocLabel>[-a-zA-Z0-9\.\+]+)$`),

// sha256:<hash>:latest-vX.Y-build will sort just after vX.Y.0, but before vX.Y.1
regexp.MustCompile(`^sha256:(?P<adhocLabel>[^:]+):latest-v(?P<year>[1-9][0-9]*)\.(?P<ordinal>[1-9][0-9]*)-build$`),
}

var formatPlaceholderRe = regexp.MustCompile("%[^%XYZpPosn]")

func preReleasePhaseByName(name string) (releasePhase, bool) {
switch name {
case "alpha":
return alpha, true
case "beta":
return beta, true
case "rc":
return rc, true
case "cloudonly":
return cloudonly, true
default:
return 0, false
}
}

func releasePhaseString(p releasePhase) string {
switch p {
case alpha:
return "alpha"
case beta:
return "beta"
case rc:
return "rc"
case cloudonly:
return "cloudonly"
default:
return ""
}
}

func submatch(pat *regexp.Regexp, matches []string, group string) string {
index := pat.SubexpIndex(group)
if index == -1 {
return ""
}
return matches[index]
}

// Version represents a CockroachDB (binary) version. Versions consist of three parts:
// a major version, written as "vX.Y" (which is typically the year and release number
// within the year), a patch version (the "Z" in "vX.Y.Z"), and sometimes one or more
Expand Down Expand Up @@ -85,26 +142,16 @@ func (v Version) Patch() int {
// - %n: adhoc build ordinal (eg the 12 in "v24.1.0-12-gabcdef")
// - %%: literal "%"
func (v Version) Format(formatStr string) string {
placeholderRe := regexp.MustCompile("%[^%XYZpPosn]")
placeholders := placeholderRe.FindAllString(formatStr, -1)
placeholders := formatPlaceholderRe.FindAllString(formatStr, -1)
if len(placeholders) > 0 {
panic(fmt.Sprintf("unknown placeholders in format string: %s", strings.Join(placeholders, ", ")))
}

phaseName := map[releasePhase]string{
alpha: "alpha",
beta: "beta",
rc: "rc",
cloudonly: "cloudonly",
adhoc: "",
stable: "",
}

formatStr = strings.ReplaceAll(formatStr, "%X", strconv.Itoa(v.year))
formatStr = strings.ReplaceAll(formatStr, "%Y", strconv.Itoa(v.ordinal))
formatStr = strings.ReplaceAll(formatStr, "%Z", strconv.Itoa(v.patch))
formatStr = strings.ReplaceAll(formatStr, "%p", strconv.Itoa(int(v.phase)))
formatStr = strings.ReplaceAll(formatStr, "%P", phaseName[v.phase])
formatStr = strings.ReplaceAll(formatStr, "%P", releasePhaseString(v.phase))
formatStr = strings.ReplaceAll(formatStr, "%o", strconv.Itoa(v.phaseOrdinal))
formatStr = strings.ReplaceAll(formatStr, "%s", strconv.Itoa(v.phaseSubOrdinal))
formatStr = strings.ReplaceAll(formatStr, "%n", strconv.Itoa(v.customOrdinal))
Expand Down Expand Up @@ -203,83 +250,54 @@ func (v Version) SafeFormat(p redact.SafePrinter, _ rune) {

// Parse creates a version from a string.
func Parse(str string) (Version, error) {
// these are roughly in "how often we expect to see them" order
patterns := []*regexp.Regexp{
regexp.MustCompile(`^v(?P<year>[1-9][0-9]*)\.(?P<ordinal>[1-9][0-9]*)\.(?P<patch>(?:[1-9][0-9]*|0))(?:-fips)?$`),
regexp.MustCompile(`^v(?P<year>[1-9][0-9]*)\.(?P<ordinal>[1-9][0-9]*)\.(?P<patch>(?:[1-9][0-9]*|0))-(?P<phase>alpha|beta|rc|cloudonly)\.(?P<phaseOrdinal>[0-9]+)(?:-fips)?$`),
regexp.MustCompile(`^v(?P<year>[1-9][0-9]*)\.(?P<ordinal>[1-9][0-9]*)\.(?P<patch>(?:[1-9][0-9]*|0))-(?P<customOrdinal>(?:[1-9][0-9]*|0))-g[a-f0-9]+(?:-fips)?$`),
regexp.MustCompile(`^v(?P<year>[1-9][0-9]*)\.(?P<ordinal>[1-9][0-9]*)\.(?P<patch>(?:[1-9][0-9]*|0))-(?P<phase>alpha|beta|rc|cloudonly).(?P<phaseOrdinal>[0-9]+)-(?P<customOrdinal>(?:[1-9][0-9]*|0))-g[a-f0-9]+(?:-fips)?$`),
regexp.MustCompile(`^v(?P<year>[1-9][0-9]*)\.(?P<ordinal>[1-9][0-9]*)\.(?P<patch>(?:[1-9][0-9]*|0))-(?P<phase>alpha|beta|rc|cloudonly).(?P<phaseOrdinal>[0-9]+)-cloudonly(-rc|\.)(?P<phaseSubOrdinal>(?:[1-9][0-9]*|0))$`),
regexp.MustCompile(`^v(?P<year>[1-9][0-9]*)\.(?P<ordinal>[1-9][0-9]*)\.(?P<patch>(?:[1-9][0-9]*|0))-(?P<phase>cloudonly)-rc(?P<phaseOrdinal>[0-9]+)$`),
regexp.MustCompile(`^v(?P<year>[1-9][0-9]*)\.(?P<ordinal>[1-9][0-9]*)\.(?P<patch>(?:[1-9][0-9]*|0))-(?P<phase>cloudonly)(?P<phaseOrdinal>[0-9]+)?$`),
Copy link
Contributor Author

@benbardin benbardin Feb 20, 2026

Choose a reason for hiding this comment

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

Here, I think? Is where cloudonly with no dot was allowed


// vX.Y.Z-<anything> will sort after the corresponding "plain" vX.Y.Z version
regexp.MustCompile(`^v(?P<year>[1-9][0-9]*)\.(?P<ordinal>[1-9][0-9]*)\.(?P<patch>(?:[1-9][0-9]*|0))-(?P<adhocLabel>[-a-zA-Z0-9\.\+]+)$`),

// sha256:<hash>:latest-vX.Y-build will sort just after vX.Y.0, but before vX.Y.1
regexp.MustCompile(`^sha256:(?P<adhocLabel>[^:]+):latest-v(?P<year>[1-9][0-9]*)\.(?P<ordinal>[1-9][0-9]*)-build$`),
}

preReleasePhase := map[string]releasePhase{
"alpha": alpha,
"beta": beta,
"rc": rc,
"cloudonly": cloudonly,
}
v := Version{raw: str, phase: stable}

submatch := func(pat *regexp.Regexp, matches []string, group string) string {
index := pat.SubexpIndex(group)
if index == -1 {
return ""
for _, pat := range parsePatterns {
matches := pat.FindStringSubmatch(str)
if matches == nil {
continue
}
return matches[index]
}

v := Version{raw: str, phase: stable}

for _, pat := range patterns {
if pat.MatchString(str) {
matches := pat.FindStringSubmatch(str)
// all patterns have vX.Y
v.year, _ = strconv.Atoi(submatch(pat, matches, "year"))
v.ordinal, _ = strconv.Atoi(submatch(pat, matches, "ordinal"))

// all patterns have vX.Y
v.year, _ = strconv.Atoi(submatch(pat, matches, "year"))
v.ordinal, _ = strconv.Atoi(submatch(pat, matches, "ordinal"))
// most have vX.Y.Z
if patch := submatch(pat, matches, "patch"); patch != "" {
v.patch, _ = strconv.Atoi(patch)
}

// most have vX.Y.Z
if patch := submatch(pat, matches, "patch"); patch != "" {
v.patch, _ = strconv.Atoi(patch)
// handle -alpha.1, -rc.3, etc
if phase := submatch(pat, matches, "phase"); phase != "" {
// Unreachable today: the regexes only match known phase names.
// Kept as a safeguard against future pattern changes.
if phaseName, ok := preReleasePhaseByName(phase); !ok {
return Version{}, errors.Newf("unknown phase '%s'", phase)
} else {
v.phase = phaseName
}

// handle -alpha.1, -rc.3, etc
if phase := submatch(pat, matches, "phase"); phase != "" {
if phaseName, ok := preReleasePhase[phase]; !ok {
return Version{}, errors.Newf("unknown phase '%s", phaseName)
} else {
v.phase = phaseName
}

if ord := submatch(pat, matches, "phaseOrdinal"); ord != "" {
v.phaseOrdinal, _ = strconv.Atoi(ord)
}
// -beta.1-cloudonly-rc1
if subOrd := submatch(pat, matches, "phaseSubOrdinal"); subOrd != "" {
v.phaseSubOrdinal, _ = strconv.Atoi(subOrd)
}
if ord := submatch(pat, matches, "phaseOrdinal"); ord != "" {
v.phaseOrdinal, _ = strconv.Atoi(ord)
}

// adhoc/adhoc builds, eg -10-g7890abcd
if ord := submatch(pat, matches, "customOrdinal"); ord != "" {
v.customOrdinal, _ = strconv.Atoi(ord)
// -beta.1-cloudonly-rc1
if subOrd := submatch(pat, matches, "phaseSubOrdinal"); subOrd != "" {
v.phaseSubOrdinal, _ = strconv.Atoi(subOrd)
}
}

// arbitrary/adhoc build tags; we have these old versions and need to parse them
if adhocLabel := submatch(pat, matches, "adhocLabel"); adhocLabel != "" {
v.phase = adhoc
v.adhocLabel = adhocLabel
}
// adhoc/adhoc builds, eg -10-g7890abcd
if ord := submatch(pat, matches, "customOrdinal"); ord != "" {
v.customOrdinal, _ = strconv.Atoi(ord)
}

return v, nil
// arbitrary/adhoc build tags; we have these old versions and need to parse them
if adhocLabel := submatch(pat, matches, "adhocLabel"); adhocLabel != "" {
v.phase = adhoc
v.adhocLabel = adhocLabel
}

return v, nil
}

err := errors.Errorf("invalid version string '%s'", str)
Expand Down
97 changes: 97 additions & 0 deletions version_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,6 +252,9 @@ func TestParse(t *testing.T) {
"v1.2.3+metadata",
"v1.2.3+metadata-with-hyphen",
"v1.2.3+metadata.with.dots",
"v24.1.0-alpha:1",
"v24.1.0-rc_1-12-gabcdef12",
"v24.1.0-beta/1-cloudonly.2",
}
for _, str := range testData {
_, err := Parse(str)
Expand Down Expand Up @@ -368,6 +371,100 @@ func TestParse(t *testing.T) {
})
}

func TestFormatPlaceholders(t *testing.T) {
for _, tc := range []struct {
rawVersion string
wantPhase string
}{
{rawVersion: "v24.1.0-alpha.2", wantPhase: "alpha"},
{rawVersion: "v24.1.0-beta.2", wantPhase: "beta"},
{rawVersion: "v24.1.0-rc.2", wantPhase: "rc"},
{rawVersion: "v24.1.0-cloudonly.2", wantPhase: "cloudonly"},
{rawVersion: "v24.1.0", wantPhase: ""},
{rawVersion: "v24.1.0-build-tag", wantPhase: ""},
} {
t.Run(tc.rawVersion, func(t *testing.T) {
v := MustParse(tc.rawVersion)
require.Equal(t, tc.wantPhase, v.Format("%P"))
})
}

require.PanicsWithValue(t, "unknown placeholders in format string: %Q", func() {
_ = MustParse("v24.1.0").Format("v%X.%Y.%Z-%Q")
})
}

func TestParseSpecificPatternWins(t *testing.T) {
for _, tc := range []struct {
raw string
phase releasePhase
phaseOrdinal int
phaseSubOrdinal int
customOrdinal int
adhocLabel string
}{
{
raw: "v23.2.0-cloudonly2",
phase: cloudonly,
phaseOrdinal: 2,
Copy link
Contributor

@dcrosta dcrosta Feb 20, 2026

Choose a reason for hiding this comment

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

This is not a valid version, there must be a . after "cloudonly". This should not parse successfully.

(I think? What did the old code version do with this version string?)

Copy link
Contributor Author

@benbardin benbardin Feb 20, 2026

Choose a reason for hiding this comment

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

I think this matches the existing pattern? That accepts cloudonly with optional digits and no dot — it predates this PR. Do you know if these versions exist in the wild? If not, we could tighten the pattern to require a dot.

Copy link
Contributor

@dcrosta dcrosta Feb 23, 2026

Choose a reason for hiding this comment

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

Huh, you're right, and we have pre-existing test cases. My mistake! (I think as cloudonlys were being introduced there were some odd versions with inconsistent naming types; the set of tests in this package is based on a comprehensive review of the patterns of actual versions that existed in the CC control plane DB when I wrote this package, plus consultation with releng folks.)

},
{
raw: "v23.2.0-cloudonly",
phase: cloudonly,
phaseOrdinal: 0,
},
{
raw: "v23.2.0-cloudonly.1",
phase: cloudonly,
phaseOrdinal: 1,
},
{
raw: "v24.1.0-rc.1-12-gabcdef56",
phase: rc,
phaseOrdinal: 1,
customOrdinal: 12,
},
{
raw: "v24.1.0-beta.1-cloudonly.2",
phase: beta,
phaseOrdinal: 1,
phaseSubOrdinal: 2,
},
} {
t.Run(tc.raw, func(t *testing.T) {
v := MustParse(tc.raw)

require.Equal(t, tc.phase, v.phase)
require.Equal(t, tc.phaseOrdinal, v.phaseOrdinal)
require.Equal(t, tc.phaseSubOrdinal, v.phaseSubOrdinal)
require.Equal(t, tc.customOrdinal, v.customOrdinal)
require.Equal(t, tc.adhocLabel, v.adhocLabel)
require.False(t, v.IsAdhocBuild())
})
}
}

func BenchmarkParse(b *testing.B) {
testData := []string{
"v19.1.11",
"v21.1.0-1-g9cbe7c5281",
"v22.2.10-1-g7b8322d67c-fips",
"v23.1.0-alpha.1-1643-gdf8e73734e-fips",
"v23.2.0-rc.2-cloudonly-rc2",
"v24.3.0-alpha.1-cloudonly.1",
"v23.1.0-swenson-mr-4",
"sha256:6bbf843734d11db9cc5eb8ea77f6974032e17ad216c91ccecfaf52a4890eaa11:latest-v22.2-build",
}

b.ReportAllocs()
for i := 0; i < b.N; i++ {
_, err := Parse(testData[i%len(testData)])
if err != nil {
b.Fatal(err)
}
}
}

func TestVersionCompare(t *testing.T) {
const aEqualsB = "equal"
const aLessThanB = "less than"
Expand Down