Skip to content

Commit cad6546

Browse files
benbardinbenbardin
committed
Reduce allocations in Parse and Format
Hoist compiled regexes and phase-name lookups to package level so they aren't rebuilt on every call. Fix two unescaped dots in parse patterns and a broken error message in the unknown-phase branch. Add tests and a benchmark.
1 parent 54dac30 commit cad6546

2 files changed

Lines changed: 252 additions & 77 deletions

File tree

‎version.go‎

Lines changed: 95 additions & 77 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,63 @@ const (
4040
adhoc = releasePhase(6)
4141
)
4242

43+
var parsePatterns = []*regexp.Regexp{
44+
// these are roughly in "how often we expect to see them" order
45+
regexp.MustCompile(`^v(?P<year>[1-9][0-9]*)\.(?P<ordinal>[1-9][0-9]*)\.(?P<patch>(?:[1-9][0-9]*|0))(?:-fips)?$`),
46+
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)?$`),
47+
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)?$`),
48+
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)?$`),
49+
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))$`),
50+
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]+)$`),
51+
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]+)?$`),
52+
53+
// vX.Y.Z-<anything> will sort after the corresponding "plain" vX.Y.Z version
54+
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\.\+]+)$`),
55+
56+
// sha256:<hash>:latest-vX.Y-build will sort just after vX.Y.0, but before vX.Y.1
57+
regexp.MustCompile(`^sha256:(?P<adhocLabel>[^:]+):latest-v(?P<year>[1-9][0-9]*)\.(?P<ordinal>[1-9][0-9]*)-build$`),
58+
}
59+
60+
var formatPlaceholderRe = regexp.MustCompile("%[^%XYZpPosn]")
61+
62+
func preReleasePhaseByName(name string) (releasePhase, bool) {
63+
switch name {
64+
case "alpha":
65+
return alpha, true
66+
case "beta":
67+
return beta, true
68+
case "rc":
69+
return rc, true
70+
case "cloudonly":
71+
return cloudonly, true
72+
default:
73+
return 0, false
74+
}
75+
}
76+
77+
func releasePhaseString(p releasePhase) string {
78+
switch p {
79+
case alpha:
80+
return "alpha"
81+
case beta:
82+
return "beta"
83+
case rc:
84+
return "rc"
85+
case cloudonly:
86+
return "cloudonly"
87+
default:
88+
return ""
89+
}
90+
}
91+
92+
func submatch(pat *regexp.Regexp, matches []string, group string) string {
93+
index := pat.SubexpIndex(group)
94+
if index == -1 {
95+
return ""
96+
}
97+
return matches[index]
98+
}
99+
43100
// Version represents a CockroachDB (binary) version. Versions consist of three parts:
44101
// a major version, written as "vX.Y" (which is typically the year and release number
45102
// within the year), a patch version (the "Z" in "vX.Y.Z"), and sometimes one or more
@@ -85,26 +142,16 @@ func (v Version) Patch() int {
85142
// - %n: adhoc build ordinal (eg the 12 in "v24.1.0-12-gabcdef")
86143
// - %%: literal "%"
87144
func (v Version) Format(formatStr string) string {
88-
placeholderRe := regexp.MustCompile("%[^%XYZpPosn]")
89-
placeholders := placeholderRe.FindAllString(formatStr, -1)
145+
placeholders := formatPlaceholderRe.FindAllString(formatStr, -1)
90146
if len(placeholders) > 0 {
91147
panic(fmt.Sprintf("unknown placeholders in format string: %s", strings.Join(placeholders, ", ")))
92148
}
93149

94-
phaseName := map[releasePhase]string{
95-
alpha: "alpha",
96-
beta: "beta",
97-
rc: "rc",
98-
cloudonly: "cloudonly",
99-
adhoc: "",
100-
stable: "",
101-
}
102-
103150
formatStr = strings.ReplaceAll(formatStr, "%X", strconv.Itoa(v.year))
104151
formatStr = strings.ReplaceAll(formatStr, "%Y", strconv.Itoa(v.ordinal))
105152
formatStr = strings.ReplaceAll(formatStr, "%Z", strconv.Itoa(v.patch))
106153
formatStr = strings.ReplaceAll(formatStr, "%p", strconv.Itoa(int(v.phase)))
107-
formatStr = strings.ReplaceAll(formatStr, "%P", phaseName[v.phase])
154+
formatStr = strings.ReplaceAll(formatStr, "%P", releasePhaseString(v.phase))
108155
formatStr = strings.ReplaceAll(formatStr, "%o", strconv.Itoa(v.phaseOrdinal))
109156
formatStr = strings.ReplaceAll(formatStr, "%s", strconv.Itoa(v.phaseSubOrdinal))
110157
formatStr = strings.ReplaceAll(formatStr, "%n", strconv.Itoa(v.customOrdinal))
@@ -203,83 +250,54 @@ func (v Version) SafeFormat(p redact.SafePrinter, _ rune) {
203250

204251
// Parse creates a version from a string.
205252
func Parse(str string) (Version, error) {
206-
// these are roughly in "how often we expect to see them" order
207-
patterns := []*regexp.Regexp{
208-
regexp.MustCompile(`^v(?P<year>[1-9][0-9]*)\.(?P<ordinal>[1-9][0-9]*)\.(?P<patch>(?:[1-9][0-9]*|0))(?:-fips)?$`),
209-
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)?$`),
210-
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)?$`),
211-
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)?$`),
212-
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))$`),
213-
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]+)$`),
214-
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]+)?$`),
215-
216-
// vX.Y.Z-<anything> will sort after the corresponding "plain" vX.Y.Z version
217-
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\.\+]+)$`),
218-
219-
// sha256:<hash>:latest-vX.Y-build will sort just after vX.Y.0, but before vX.Y.1
220-
regexp.MustCompile(`^sha256:(?P<adhocLabel>[^:]+):latest-v(?P<year>[1-9][0-9]*)\.(?P<ordinal>[1-9][0-9]*)-build$`),
221-
}
222-
223-
preReleasePhase := map[string]releasePhase{
224-
"alpha": alpha,
225-
"beta": beta,
226-
"rc": rc,
227-
"cloudonly": cloudonly,
228-
}
253+
v := Version{raw: str, phase: stable}
229254

230-
submatch := func(pat *regexp.Regexp, matches []string, group string) string {
231-
index := pat.SubexpIndex(group)
232-
if index == -1 {
233-
return ""
255+
for _, pat := range parsePatterns {
256+
matches := pat.FindStringSubmatch(str)
257+
if matches == nil {
258+
continue
234259
}
235-
return matches[index]
236-
}
237-
238-
v := Version{raw: str, phase: stable}
239260

240-
for _, pat := range patterns {
241-
if pat.MatchString(str) {
242-
matches := pat.FindStringSubmatch(str)
261+
// all patterns have vX.Y
262+
v.year, _ = strconv.Atoi(submatch(pat, matches, "year"))
263+
v.ordinal, _ = strconv.Atoi(submatch(pat, matches, "ordinal"))
243264

244-
// all patterns have vX.Y
245-
v.year, _ = strconv.Atoi(submatch(pat, matches, "year"))
246-
v.ordinal, _ = strconv.Atoi(submatch(pat, matches, "ordinal"))
265+
// most have vX.Y.Z
266+
if patch := submatch(pat, matches, "patch"); patch != "" {
267+
v.patch, _ = strconv.Atoi(patch)
268+
}
247269

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

253-
// handle -alpha.1, -rc.3, etc
254-
if phase := submatch(pat, matches, "phase"); phase != "" {
255-
if phaseName, ok := preReleasePhase[phase]; !ok {
256-
return Version{}, errors.Newf("unknown phase '%s", phaseName)
257-
} else {
258-
v.phase = phaseName
259-
}
260-
261-
if ord := submatch(pat, matches, "phaseOrdinal"); ord != "" {
262-
v.phaseOrdinal, _ = strconv.Atoi(ord)
263-
}
264-
// -beta.1-cloudonly-rc1
265-
if subOrd := submatch(pat, matches, "phaseSubOrdinal"); subOrd != "" {
266-
v.phaseSubOrdinal, _ = strconv.Atoi(subOrd)
267-
}
280+
if ord := submatch(pat, matches, "phaseOrdinal"); ord != "" {
281+
v.phaseOrdinal, _ = strconv.Atoi(ord)
268282
}
269-
270-
// adhoc/adhoc builds, eg -10-g7890abcd
271-
if ord := submatch(pat, matches, "customOrdinal"); ord != "" {
272-
v.customOrdinal, _ = strconv.Atoi(ord)
283+
// -beta.1-cloudonly-rc1
284+
if subOrd := submatch(pat, matches, "phaseSubOrdinal"); subOrd != "" {
285+
v.phaseSubOrdinal, _ = strconv.Atoi(subOrd)
273286
}
287+
}
274288

275-
// arbitrary/adhoc build tags; we have these old versions and need to parse them
276-
if adhocLabel := submatch(pat, matches, "adhocLabel"); adhocLabel != "" {
277-
v.phase = adhoc
278-
v.adhocLabel = adhocLabel
279-
}
289+
// adhoc/adhoc builds, eg -10-g7890abcd
290+
if ord := submatch(pat, matches, "customOrdinal"); ord != "" {
291+
v.customOrdinal, _ = strconv.Atoi(ord)
292+
}
280293

281-
return v, nil
294+
// arbitrary/adhoc build tags; we have these old versions and need to parse them
295+
if adhocLabel := submatch(pat, matches, "adhocLabel"); adhocLabel != "" {
296+
v.phase = adhoc
297+
v.adhocLabel = adhocLabel
282298
}
299+
300+
return v, nil
283301
}
284302

285303
err := errors.Errorf("invalid version string '%s'", str)

‎version_test.go‎

Lines changed: 157 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -252,6 +252,9 @@ func TestParse(t *testing.T) {
252252
"v1.2.3+metadata",
253253
"v1.2.3+metadata-with-hyphen",
254254
"v1.2.3+metadata.with.dots",
255+
"v24.1.0-alpha:1",
256+
"v24.1.0-rc_1-12-gabcdef12",
257+
"v24.1.0-beta/1-cloudonly.2",
255258
}
256259
for _, str := range testData {
257260
_, err := Parse(str)
@@ -368,6 +371,160 @@ func TestParse(t *testing.T) {
368371
})
369372
}
370373

374+
func TestFormatPlaceholders(t *testing.T) {
375+
for _, tc := range []struct {
376+
rawVersion string
377+
wantPhase string
378+
}{
379+
{rawVersion: "v24.1.0-alpha.2", wantPhase: "alpha"},
380+
{rawVersion: "v24.1.0-beta.2", wantPhase: "beta"},
381+
{rawVersion: "v24.1.0-rc.2", wantPhase: "rc"},
382+
{rawVersion: "v24.1.0-cloudonly.2", wantPhase: "cloudonly"},
383+
{rawVersion: "v24.1.0", wantPhase: ""},
384+
{rawVersion: "v24.1.0-build-tag", wantPhase: ""},
385+
} {
386+
t.Run(tc.rawVersion, func(t *testing.T) {
387+
v := MustParse(tc.rawVersion)
388+
require.Equal(t, tc.wantPhase, v.Format("%P"))
389+
})
390+
}
391+
392+
require.PanicsWithValue(t, "unknown placeholders in format string: %Q", func() {
393+
_ = MustParse("v24.1.0").Format("v%X.%Y.%Z-%Q")
394+
})
395+
}
396+
397+
func TestParseSpecificPatternWins(t *testing.T) {
398+
for _, tc := range []struct {
399+
raw string
400+
phase releasePhase
401+
phaseOrdinal int
402+
phaseSubOrdinal int
403+
customOrdinal int
404+
adhocLabel string
405+
}{
406+
{
407+
raw: "v23.2.0-cloudonly2",
408+
phase: cloudonly,
409+
phaseOrdinal: 2,
410+
},
411+
{
412+
raw: "v23.2.0-cloudonly",
413+
phase: cloudonly,
414+
phaseOrdinal: 0,
415+
},
416+
{
417+
raw: "v23.2.0-cloudonly.1",
418+
phase: cloudonly,
419+
phaseOrdinal: 1,
420+
},
421+
{
422+
raw: "v24.1.0-rc.1-12-gabcdef12",
423+
phase: rc,
424+
phaseOrdinal: 1,
425+
customOrdinal: 12,
426+
},
427+
{
428+
raw: "v24.1.0-beta.1-cloudonly.2",
429+
phase: beta,
430+
phaseOrdinal: 1,
431+
phaseSubOrdinal: 2,
432+
},
433+
} {
434+
t.Run(tc.raw, func(t *testing.T) {
435+
v := MustParse(tc.raw)
436+
437+
require.Equal(t, tc.phase, v.phase)
438+
require.Equal(t, tc.phaseOrdinal, v.phaseOrdinal)
439+
require.Equal(t, tc.phaseSubOrdinal, v.phaseSubOrdinal)
440+
require.Equal(t, tc.customOrdinal, v.customOrdinal)
441+
require.Equal(t, tc.adhocLabel, v.adhocLabel)
442+
require.False(t, v.IsAdhocBuild())
443+
})
444+
}
445+
}
446+
447+
func BenchmarkParse(b *testing.B) {
448+
testData := []string{
449+
"v19.1.11",
450+
"v21.1.0-1-g9cbe7c5281",
451+
"v22.2.10-1-g7b8322d67c-fips",
452+
"v23.1.0-alpha.1-1643-gdf8e73734e-fips",
453+
"v23.2.0-rc.2-cloudonly-rc2",
454+
"v24.3.0-alpha.1-cloudonly.1",
455+
"v23.1.0-swenson-mr-4",
456+
"sha256:6bbf843734d11db9cc5eb8ea77f6974032e17ad216c91ccecfaf52a4890eaa11:latest-v22.2-build",
457+
}
458+
459+
b.ReportAllocs()
460+
for i := 0; i < b.N; i++ {
461+
_, err := Parse(testData[i%len(testData)])
462+
if err != nil {
463+
b.Fatal(err)
464+
}
465+
}
466+
}
467+
468+
func TestCompareTransitivitySmallSet(t *testing.T) {
469+
sign := func(i int) int {
470+
switch {
471+
case i < 0:
472+
return -1
473+
case i > 0:
474+
return 1
475+
default:
476+
return 0
477+
}
478+
}
479+
480+
rawVersions := []string{
481+
"v21.1.0-alpha.1",
482+
"v21.1.0-beta.1",
483+
"v21.1.0-rc.1",
484+
"v21.1.0-cloudonly.1",
485+
"v21.1.0",
486+
"v21.1.0-1-g9cbe7c5281",
487+
"v21.1.0-customLabel",
488+
"v21.1.1",
489+
}
490+
491+
versions := make([]Version, len(rawVersions))
492+
for i, raw := range rawVersions {
493+
versions[i] = MustParse(raw)
494+
}
495+
496+
for i := range versions {
497+
for j := range versions {
498+
ab := sign(versions[i].Compare(versions[j]))
499+
ba := sign(versions[j].Compare(versions[i]))
500+
require.Equalf(t, -ab, ba, "antisymmetry failed for (%s, %s)", rawVersions[i], rawVersions[j])
501+
}
502+
}
503+
504+
for i := range versions {
505+
for j := range versions {
506+
for k := range versions {
507+
ab := versions[i].Compare(versions[j])
508+
bc := versions[j].Compare(versions[k])
509+
ac := versions[i].Compare(versions[k])
510+
511+
if ab <= 0 && bc <= 0 {
512+
require.Truef(t, ac <= 0, "transitivity <= failed: %s <= %s <= %s", rawVersions[i], rawVersions[j], rawVersions[k])
513+
}
514+
if ab >= 0 && bc >= 0 {
515+
require.Truef(t, ac >= 0, "transitivity >= failed: %s >= %s >= %s", rawVersions[i], rawVersions[j], rawVersions[k])
516+
}
517+
if ab < 0 && bc < 0 {
518+
require.Truef(t, ac < 0, "strict transitivity < failed: %s < %s < %s", rawVersions[i], rawVersions[j], rawVersions[k])
519+
}
520+
if ab > 0 && bc > 0 {
521+
require.Truef(t, ac > 0, "strict transitivity > failed: %s > %s > %s", rawVersions[i], rawVersions[j], rawVersions[k])
522+
}
523+
}
524+
}
525+
}
526+
}
527+
371528
func TestVersionCompare(t *testing.T) {
372529
const aEqualsB = "equal"
373530
const aLessThanB = "less than"

0 commit comments

Comments
 (0)