Skip to content

Commit

Permalink
ociregistry: deduplicate and lazily compile global regexes
Browse files Browse the repository at this point in the history 
cmd/cue imports the ociregistry and ociregistry/ociref packages,
which cost about 0.1ms and 0.04ms at init time respectively,
due to how they compile a few regular expressions at init time.

First, two of these are duplicates; ociregistry.IsValidRepoName
can be replaced by ociref.IsValidRepository, and ociregistry.IsValidTag
can be replaced by ociref.IsValidTag.
In both cases, ociref's regular expressions are better documented.

Second, ociregistry.IsValidDigest is not a duplicate, but it seems
to belong to ociref alongside all the other IsValid funcs,
plus ociref already performs similar validity checks in existing APIs.

To not break users with the changes above, keep the ociregistry APIs
as deprecated funcs which call the ociref APIs.
Note that this required reversing the import between the two packages,
which was easy enough given ociref only needed ociregistry.Digest,
and which seems like the better option given that ociref is lower level.

Third, hide the three global regexp.MustCompile calls in ociref
behind sync.OnceValue, meaning that the regular expressions only get
compiled when they are first used.

With all of the changes combined, we save about 0.15ms and 2300 allocs
at init time, which should be small but noticeable for cmd/cue.

    goos: linux
    goarch: amd64
    cpu: AMD Ryzen 7 PRO 5850U with Radeon Graphics
                                     │      old       │                 new                  │
                                     │     sec/op     │    sec/op      vs base               │
    CuelabsDevGoOciOciregistry         108690.0n ± 5%   103.0n ±  73%  -99.91% (p=0.002 n=6)
    CuelabsDevGoOciOciregistryOciref   46316.50n ± 4%   19.00n ± 195%  -99.96% (p=0.002 n=6)
    geomean                               70.95µ        44.24n         -99.94%

                                     │      old      │                new                │
                                     │     B/op      │    B/op     vs base               │
    CuelabsDevGoOciOciregistry         130259.0 ± 0%   947.5 ± 0%  -99.27% (p=0.002 n=6)
    CuelabsDevGoOciOciregistryOciref    61912.0 ± 0%   480.0 ± 0%  -99.22% (p=0.002 n=6)
    geomean                             87.70Ki        674.4       -99.25%

                                     │      old      │                new                │
                                     │   allocs/op   │ allocs/op   vs base               │
    CuelabsDevGoOciOciregistry         1855.000 ± 0%   2.000 ± 0%  -99.89% (p=0.002 n=6)
    CuelabsDevGoOciOciregistryOciref     442.00 ± 0%   21.00 ± 0%  -95.25% (p=0.002 n=6)
    geomean                               905.5        6.481       -99.28%

Signed-off-by: Daniel Martí <[email protected]>
Change-Id: I1be320904649374f768d392c7a104cd2f3c5021e
Reviewed-on: https://review.gerrithub.io/c/cue-labs/oci/+/1198992
TryBot-Result: CUE porcuepine <[email protected]>
Reviewed-by: Roger Peppe <[email protected]>
  • Loading branch information
@mvdan
mvdan committed Aug 7, 2024
1 parent fa95d05 commit 225cd5f
Show file tree
Hide file tree
Showing 7 changed files with 65 additions and 41 deletions.
4 changes: 2 additions & 2 deletions ociregistry/interface.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ import (
"context"
"io"

"github.com/opencontainers/go-digest"
"cuelabs.dev/go/oci/ociregistry/ociref"
ocispec "github.com/opencontainers/image-spec/specs-go/v1"
)

Expand All @@ -81,7 +81,7 @@ type ReadWriter interface {
}

type (
Digest = digest.Digest
Digest = ociref.Digest
Descriptor = ocispec.Descriptor
Manifest = ocispec.Manifest
)
Expand Down
29 changes: 15 additions & 14 deletions ociregistry/internal/ocirequest/request.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
"unicode/utf8"

"cuelabs.dev/go/oci/ociregistry"
"cuelabs.dev/go/oci/ociregistry/ociref"
)

// ParseError represents an error that can happen when parsing.
Expand Down Expand Up @@ -225,7 +226,7 @@ func parse(method string, u *url.URL) (*Request, error) {
}
if ok {
rreq.Repo = uploadPath
if !ociregistry.IsValidRepoName(rreq.Repo) {
if !ociref.IsValidRepository(rreq.Repo) {
return nil, ociregistry.ErrNameInvalid
}
if method != "POST" {
Expand All @@ -234,7 +235,7 @@ func parse(method string, u *url.URL) (*Request, error) {
if d := urlq.Get("mount"); d != "" {
// end-11
rreq.Digest = d
if !ociregistry.IsValidDigest(rreq.Digest) {
if !ociref.IsValidDigest(rreq.Digest) {
return nil, ociregistry.ErrDigestInvalid
}
rreq.FromRepo = urlq.Get("from")
Expand All @@ -246,7 +247,7 @@ func parse(method string, u *url.URL) (*Request, error) {
rreq.Digest = ""
return &rreq, nil
}
if !ociregistry.IsValidRepoName(rreq.FromRepo) {
if !ociref.IsValidRepository(rreq.FromRepo) {
return nil, ociregistry.ErrNameInvalid
}
rreq.Kind = ReqBlobMount
Expand All @@ -255,7 +256,7 @@ func parse(method string, u *url.URL) (*Request, error) {
if d := urlq.Get("digest"); d != "" {
// end-4b
rreq.Digest = d
if !ociregistry.IsValidDigest(d) {
if !ociref.IsValidDigest(d) {
return nil, ErrBadlyFormedDigest
}
rreq.Kind = ReqBlobUploadBlob
Expand All @@ -276,10 +277,10 @@ func parse(method string, u *url.URL) (*Request, error) {
switch lastButOne {
case "blobs":
rreq.Repo = path
if !ociregistry.IsValidDigest(last) {
if !ociref.IsValidDigest(last) {
return nil, ErrBadlyFormedDigest
}
if !ociregistry.IsValidRepoName(rreq.Repo) {
if !ociref.IsValidRepository(rreq.Repo) {
return nil, ociregistry.ErrNameInvalid
}
rreq.Digest = last
Expand All @@ -302,7 +303,7 @@ func parse(method string, u *url.URL) (*Request, error) {
return nil, ErrNotFound
}
rreq.Repo = repo
if !ociregistry.IsValidRepoName(rreq.Repo) {
if !ociref.IsValidRepository(rreq.Repo) {
return nil, ociregistry.ErrNameInvalid
}
uploadID64 := last
Expand All @@ -326,7 +327,7 @@ func parse(method string, u *url.URL) (*Request, error) {
case "PUT":
rreq.Kind = ReqBlobCompleteUpload
rreq.Digest = urlq.Get("digest")
if !ociregistry.IsValidDigest(rreq.Digest) {
if !ociref.IsValidDigest(rreq.Digest) {
return nil, ErrBadlyFormedDigest
}
default:
Expand All @@ -335,13 +336,13 @@ func parse(method string, u *url.URL) (*Request, error) {
return &rreq, nil
case "manifests":
rreq.Repo = path
if !ociregistry.IsValidRepoName(rreq.Repo) {
if !ociref.IsValidRepository(rreq.Repo) {
return nil, ociregistry.ErrNameInvalid
}
switch {
case ociregistry.IsValidDigest(last):
case ociref.IsValidDigest(last):
rreq.Digest = last
case ociregistry.IsValidTag(last):
case ociref.IsValidTag(last):
rreq.Tag = last
default:
return nil, ErrNotFound
Expand Down Expand Up @@ -371,20 +372,20 @@ func parse(method string, u *url.URL) (*Request, error) {
return nil, ErrMethodNotAllowed
}
rreq.Repo = path
if !ociregistry.IsValidRepoName(rreq.Repo) {
if !ociref.IsValidRepository(rreq.Repo) {
return nil, ociregistry.ErrNameInvalid
}
rreq.Kind = ReqTagsList
return &rreq, nil
case "referrers":
if !ociregistry.IsValidDigest(last) {
if !ociref.IsValidDigest(last) {
return nil, ErrBadlyFormedDigest
}
if method != "GET" {
return nil, ErrMethodNotAllowed
}
rreq.Repo = path
if !ociregistry.IsValidRepoName(rreq.Repo) {
if !ociref.IsValidRepository(rreq.Repo) {
return nil, ociregistry.ErrNameInvalid
}
// TODO is there any kind of pagination for referrers?
Expand Down
3 changes: 2 additions & 1 deletion ociregistry/ociclient/client.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ import (
"cuelabs.dev/go/oci/ociregistry"
"cuelabs.dev/go/oci/ociregistry/internal/ocirequest"
"cuelabs.dev/go/oci/ociregistry/ociauth"
"cuelabs.dev/go/oci/ociregistry/ociref"
)

// debug enables logging.
Expand Down Expand Up @@ -166,7 +167,7 @@ func descriptorFromResponse(resp *http.Response, knownDigest digest.Digest, requ
}
digest := digest.Digest(resp.Header.Get("Docker-Content-Digest"))
if digest != "" {
if !ociregistry.IsValidDigest(string(digest)) {
if !ociref.IsValidDigest(string(digest)) {
return ociregistry.Descriptor{}, fmt.Errorf("bad digest %q found in response", digest)
}
} else {
Expand Down
3 changes: 2 additions & 1 deletion ociregistry/ocimem/registry.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"sync"

"cuelabs.dev/go/oci/ociregistry"
"cuelabs.dev/go/oci/ociregistry/ociref"
"github.com/opencontainers/go-digest"
)

Expand Down Expand Up @@ -120,7 +121,7 @@ func (r *Registry) blobForDigest(repoName string, dig ociregistry.Digest) (*blob
}

func (r *Registry) makeRepo(repoName string) (*repository, error) {
if !ociregistry.IsValidRepoName(repoName) {
if !ociref.IsValidRepository(repoName) {
return nil, ociregistry.ErrNameInvalid
}
if r.repos == nil {
Expand Down
3 changes: 2 additions & 1 deletion ociregistry/ocimem/writer.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"io"

"cuelabs.dev/go/oci/ociregistry"
"cuelabs.dev/go/oci/ociregistry/ociref"
"github.com/opencontainers/go-digest"
)

Expand Down Expand Up @@ -107,7 +108,7 @@ func (r *Registry) PushManifest(ctx context.Context, repoName string, tag string
Size: int64(len(data)),
}
if tag != "" {
if !ociregistry.IsValidTag(tag) {
if !ociref.IsValidTag(tag) {
return ociregistry.Descriptor{}, fmt.Errorf("invalid tag")
}
if r.cfg.ImmutableTags {
Expand Down
49 changes: 32 additions & 17 deletions ociregistry/ociref/reference.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,9 @@ import (
"fmt"
"regexp"
"strings"
"sync"

"cuelabs.dev/go/oci/ociregistry"
"github.com/opencontainers/go-digest"
)

// The following regular expressions derived from code in the
Expand Down Expand Up @@ -91,17 +92,23 @@ const (
repoName = pathComponent + `(?:` + `/` + pathComponent + `)*`
)

var referencePat = regexp.MustCompile(
`^(?:` +
`(?:` + `(` + domainAndPort + `)` + `/` + `)?` + // capture 1: host
`(` + repoName + `)` + // capture 2: repository name
`(?:` + `:([^@]+))?` + // capture 3: tag; rely on Go logic to test validity.
`(?:` + `@(.+))?` + // capture 4: digest; rely on go-digest to find issues
`)$`,
)

var hostPat = regexp.MustCompile(`^(?:` + domainAndPort + `)$`)
var repoPat = regexp.MustCompile(`^(?:` + repoName + `)$`)
var referencePat = sync.OnceValue(func() *regexp.Regexp {
return regexp.MustCompile(
`^(?:` +
`(?:` + `(` + domainAndPort + `)` + `/` + `)?` + // capture 1: host
`(` + repoName + `)` + // capture 2: repository name
`(?:` + `:([^@]+))?` + // capture 3: tag; rely on Go logic to test validity.
`(?:` + `@(.+))?` + // capture 4: digest; rely on go-digest to find issues
`)$`,
)
})

var hostPat = sync.OnceValue(func() *regexp.Regexp {
return regexp.MustCompile(`^(?:` + domainAndPort + `)$`)
})
var repoPat = sync.OnceValue(func() *regexp.Regexp {
return regexp.MustCompile(`^(?:` + repoName + `)$`)
})

// Reference represents an entry in an OCI repository.
type Reference struct {
Expand All @@ -120,25 +127,33 @@ type Reference struct {

// Digest holds the DIGEST part of an @DIGEST reference
// or of a :TAG@DIGEST reference.
Digest ociregistry.Digest
Digest Digest
}

type Digest = digest.Digest

// IsValidHost reports whether s is a valid host (or host:port) part of a reference string.
func IsValidHost(s string) bool {
return hostPat.MatchString(s)
return hostPat().MatchString(s)
}

// IsValidHost reports whether s is a valid repository part
// of a reference string.
func IsValidRepository(s string) bool {
return repoPat.MatchString(s)
return repoPat().MatchString(s)
}

// IsValidTag reports whether s is a valid reference tag.
func IsValidTag(s string) bool {
return checkTag(s) == nil
}

// IsValidDigest reports whether the digest d is well formed.
func IsValidDigest(d string) bool {
_, err := digest.Parse(d)
return err == nil
}

// Parse parses a reference string that must include
// a host name (or host:port pair) component.
//
Expand All @@ -165,12 +180,12 @@ func Parse(refStr string) (Reference, error) {
// Unlike "docker pull" however, there is no default registry: when
// presented with a bare repository name, the Host field will be empty.
func ParseRelative(refStr string) (Reference, error) {
m := referencePat.FindStringSubmatch(refStr)
m := referencePat().FindStringSubmatch(refStr)
if m == nil {
return Reference{}, fmt.Errorf("invalid reference syntax (%q)", refStr)
}
var ref Reference
ref.Host, ref.Repository, ref.Tag, ref.Digest = m[1], m[2], m[3], ociregistry.Digest(m[4])
ref.Host, ref.Repository, ref.Tag, ref.Digest = m[1], m[2], m[3], Digest(m[4])
// Check lengths and digest: we don't check these as part of the regexp
// because it's more efficient to do it in Go and we get
// nicer error messages as a result.
Expand Down
15 changes: 10 additions & 5 deletions ociregistry/valid.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ package ociregistry
import (
"regexp"

"github.com/opencontainers/go-digest"
"cuelabs.dev/go/oci/ociregistry/ociref"
)

var (
Expand All @@ -13,18 +13,23 @@ var (

// IsValidRepoName reports whether the given repository
// name is valid according to the specification.
//
// Deprecated: use [ociref.IsValidRepository].
func IsValidRepoName(repoName string) bool {
return repoNamePattern.MatchString(repoName)
return ociref.IsValidRepository(repoName)
}

// IsValidTag reports whether the digest d is valid
// according to the specification.
//
// Deprecated: use [ociref.IsValidTag].
func IsValidTag(tag string) bool {
return tagPattern.MatchString(tag)
return ociref.IsValidTag(tag)
}

// IsValidDigest reports whether the digest d is well formed.
//
// Deprecated: use [ociref.IsValidDigest].
func IsValidDigest(d string) bool {
_, err := digest.Parse(d)
return err == nil
return ociref.IsValidDigest(d)
}

0 comments on commit 225cd5f

Please sign in to comment.