Skip to content

Commit

Permalink
Avoid redundant calls to filepath.Clean (#2652)
Browse files Browse the repository at this point in the history 
filepath.Clean shows up in profiles as a hot spot, and there seem to be
many redundant calls, particularly in ignorelist handling. We can avoid
these redundant calls by pre-cleaning entries in the ignore list, and
providing fast paths when we know we're already dealing with a cleaned
candidate path.

Before:

     580ms  3.03% 72.35%      590ms  3.08%  path/filepath.(*lazybuf).append (inline)
     390ms  2.03% 74.39%      990ms  5.16%  path/filepath.Clean

After:

     0.13s  0.69% 84.01%      0.17s  0.91%  path/filepath.(*lazybuf).append (inline)
     0.13s  0.69% 84.70%      0.31s  1.65%  path/filepath.Clean
  • Loading branch information
@aaronlehmann
aaronlehmann authored Aug 1, 2023
1 parent 81a7294 commit 32ce1bf
Show file tree
Hide file tree
Showing 5 changed files with 53 additions and 38 deletions.
8 changes: 4 additions & 4 deletions pkg/commands/copy_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@ func Test_CachingCopyCommand_ExecuteCommand(t *testing.T) {
extractedFiles: []string{"/foo.txt"},
contextFiles: []string{"foo.txt"},
}
c.extractFn = func(_ string, _ *tar.Header, _ io.Reader) error {
c.extractFn = func(_ string, _ *tar.Header, _ string, _ io.Reader) error {
*tc.count++
return nil
}
Expand All @@ -166,7 +166,7 @@ func Test_CachingCopyCommand_ExecuteCommand(t *testing.T) {
description: "with no image",
expectErr: true,
}
c.extractFn = func(_ string, _ *tar.Header, _ io.Reader) error {
c.extractFn = func(_ string, _ *tar.Header, _ string, _ io.Reader) error {
return nil
}
tc.command = c
Expand All @@ -176,7 +176,7 @@ func Test_CachingCopyCommand_ExecuteCommand(t *testing.T) {
c := &CachingCopyCommand{
img: fakeImage{},
}
c.extractFn = func(_ string, _ *tar.Header, _ io.Reader) error {
c.extractFn = func(_ string, _ *tar.Header, _ string, _ io.Reader) error {
return nil
}
return testCase{
Expand All @@ -193,7 +193,7 @@ func Test_CachingCopyCommand_ExecuteCommand(t *testing.T) {
},
},
}
c.extractFn = func(_ string, _ *tar.Header, _ io.Reader) error {
c.extractFn = func(_ string, _ *tar.Header, _ string, _ io.Reader) error {
return nil
}
tc := testCase{
Expand Down
8 changes: 4 additions & 4 deletions pkg/commands/run_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,7 @@ func Test_CachingRunCommand_ExecuteCommand(t *testing.T) {
extractedFiles: []string{"/foo.txt"},
contextFiles: []string{"foo.txt"},
}
c.extractFn = func(_ string, _ *tar.Header, _ io.Reader) error {
c.extractFn = func(_ string, _ *tar.Header, _ string, _ io.Reader) error {
*tc.count++
return nil
}
Expand All @@ -217,7 +217,7 @@ func Test_CachingRunCommand_ExecuteCommand(t *testing.T) {
desctiption: "with no image",
expectErr: true,
}
c.extractFn = func(_ string, _ *tar.Header, _ io.Reader) error {
c.extractFn = func(_ string, _ *tar.Header, _ string, _ io.Reader) error {
return nil
}
tc.command = c
Expand All @@ -228,7 +228,7 @@ func Test_CachingRunCommand_ExecuteCommand(t *testing.T) {
img: fakeImage{},
}

c.extractFn = func(_ string, _ *tar.Header, _ io.Reader) error {
c.extractFn = func(_ string, _ *tar.Header, _ string, _ io.Reader) error {
return nil
}

Expand All @@ -246,7 +246,7 @@ func Test_CachingRunCommand_ExecuteCommand(t *testing.T) {
},
},
}
c.extractFn = func(_ string, _ *tar.Header, _ io.Reader) error {
c.extractFn = func(_ string, _ *tar.Header, _ string, _ io.Reader) error {
return nil
}
tc := testCase{
Expand Down
2 changes: 1 addition & 1 deletion pkg/filesystem/resolve.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ func ResolvePaths(paths []string, wl []util.IgnoreListEntry) (pathsToAdd []strin

// If the given path is a symlink and the target is part of the ignorelist
// ignore the target
if util.CheckProvidedIgnoreList(evaled, wl) {
if util.CheckCleanedPathAgainstProvidedIgnoreList(evaled, wl) {
logrus.Debugf("Path %s is ignored, ignoring it", evaled)
continue
}
Expand Down
69 changes: 42 additions & 27 deletions pkg/util/fs_util.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ type IgnoreListEntry struct {

var defaultIgnoreList = []IgnoreListEntry{
{
Path: config.KanikoDir,
Path: filepath.Clean(config.KanikoDir),
PrefixMatchOnly: false,
},
{
Expand All @@ -86,7 +86,7 @@ type FileContext struct {
ExcludedFiles []string
}

type ExtractFunction func(string, *tar.Header, io.Reader) error
type ExtractFunction func(string, *tar.Header, string, io.Reader) error

type FSConfig struct {
includeWhiteout bool
Expand All @@ -100,11 +100,17 @@ func IgnoreList() []IgnoreListEntry {
}

func AddToIgnoreList(entry IgnoreListEntry) {
ignorelist = append(ignorelist, entry)
ignorelist = append(ignorelist, IgnoreListEntry{
Path: filepath.Clean(entry.Path),
PrefixMatchOnly: entry.PrefixMatchOnly,
})
}

func AddToDefaultIgnoreList(entry IgnoreListEntry) {
defaultIgnoreList = append(defaultIgnoreList, entry)
defaultIgnoreList = append(defaultIgnoreList, IgnoreListEntry{
Path: filepath.Clean(entry.Path),
PrefixMatchOnly: entry.PrefixMatchOnly,
})
}

func IncludeWhiteout() FSOpt {
Expand Down Expand Up @@ -175,7 +181,8 @@ func GetFSFromLayers(root string, layers []v1.Layer, opts ...FSOpt) ([]string, e
return nil, errors.Wrap(err, fmt.Sprintf("error reading tar %d", i))
}

path := filepath.Join(root, filepath.Clean(hdr.Name))
cleanedName := filepath.Clean(hdr.Name)
path := filepath.Join(root, cleanedName)
base := filepath.Base(path)
dir := filepath.Dir(path)

Expand All @@ -185,7 +192,7 @@ func GetFSFromLayers(root string, layers []v1.Layer, opts ...FSOpt) ([]string, e
name := strings.TrimPrefix(base, archive.WhiteoutPrefix)
path := filepath.Join(dir, name)

if CheckIgnoreList(path) {
if CheckCleanedPathAgainstIgnoreList(path) {
logrus.Tracef("Not deleting %s, as it's ignored", path)
continue
}
Expand All @@ -205,11 +212,11 @@ func GetFSFromLayers(root string, layers []v1.Layer, opts ...FSOpt) ([]string, e

}

if err := cfg.extractFunc(root, hdr, tr); err != nil {
if err := cfg.extractFunc(root, hdr, cleanedName, tr); err != nil {
return nil, err
}

extractedFiles = append(extractedFiles, filepath.Join(root, filepath.Clean(hdr.Name)))
extractedFiles = append(extractedFiles, filepath.Join(root, cleanedName))
}
}
return extractedFiles, nil
Expand All @@ -224,7 +231,7 @@ func DeleteFilesystem() error {
return nil //nolint:nilerr
}

if CheckIgnoreList(path) {
if CheckCleanedPathAgainstIgnoreList(path) {
if !isExist(path) {
logrus.Debugf("Path %s ignored, but not exists", path)
return nil
Expand Down Expand Up @@ -276,16 +283,17 @@ func UnTar(r io.Reader, dest string) ([]string, error) {
if err != nil {
return nil, err
}
if err := ExtractFile(dest, hdr, tr); err != nil {
cleanedName := filepath.Clean(hdr.Name)
if err := ExtractFile(dest, hdr, cleanedName, tr); err != nil {
return nil, err
}
extractedFiles = append(extractedFiles, filepath.Join(dest, filepath.Clean(hdr.Name)))
extractedFiles = append(extractedFiles, filepath.Join(dest, cleanedName))
}
return extractedFiles, nil
}

func ExtractFile(dest string, hdr *tar.Header, tr io.Reader) error {
path := filepath.Join(dest, filepath.Clean(hdr.Name))
func ExtractFile(dest string, hdr *tar.Header, cleanedName string, tr io.Reader) error {
path := filepath.Join(dest, cleanedName)
base := filepath.Base(path)
dir := filepath.Dir(path)
mode := hdr.FileInfo().Mode()
Expand All @@ -297,7 +305,7 @@ func ExtractFile(dest string, hdr *tar.Header, tr io.Reader) error {
return err
}

if CheckIgnoreList(abs) && !checkIgnoreListRoot(dest) {
if CheckCleanedPathAgainstIgnoreList(abs) && !checkIgnoreListRoot(dest) {
logrus.Debugf("Not adding %s because it is ignored", path)
return nil
}
Expand Down Expand Up @@ -358,7 +366,7 @@ func ExtractFile(dest string, hdr *tar.Header, tr io.Reader) error {
if err != nil {
return err
}
if CheckIgnoreList(abs) {
if CheckCleanedPathAgainstIgnoreList(abs) {
logrus.Tracef("Skipping link from %s to %s because %s is ignored", hdr.Linkname, path, hdr.Linkname)
return nil
}
Expand Down Expand Up @@ -399,6 +407,7 @@ func ExtractFile(dest string, hdr *tar.Header, tr io.Reader) error {
}

func IsInProvidedIgnoreList(path string, wl []IgnoreListEntry) bool {
path = filepath.Clean(path)
for _, entry := range wl {
if !entry.PrefixMatchOnly && path == entry.Path {
return true
Expand All @@ -412,9 +421,9 @@ func IsInIgnoreList(path string) bool {
return IsInProvidedIgnoreList(path, ignorelist)
}

func CheckProvidedIgnoreList(path string, wl []IgnoreListEntry) bool {
func CheckCleanedPathAgainstProvidedIgnoreList(path string, wl []IgnoreListEntry) bool {
for _, wl := range ignorelist {
if HasFilepathPrefix(path, wl.Path, wl.PrefixMatchOnly) {
if hasCleanedFilepathPrefix(path, wl.Path, wl.PrefixMatchOnly) {
return true
}
}
Expand All @@ -423,7 +432,11 @@ func CheckProvidedIgnoreList(path string, wl []IgnoreListEntry) bool {
}

func CheckIgnoreList(path string) bool {
return CheckProvidedIgnoreList(path, ignorelist)
return CheckCleanedPathAgainstIgnoreList(filepath.Clean(path))
}

func CheckCleanedPathAgainstIgnoreList(path string) bool {
return CheckCleanedPathAgainstProvidedIgnoreList(path, ignorelist)
}

func checkIgnoreListRoot(root string) bool {
Expand Down Expand Up @@ -463,7 +476,7 @@ func DetectFilesystemIgnoreList(path string) error {
}
if lineArr[4] != config.RootDir {
logrus.Tracef("Adding ignore list entry %s from line: %s", lineArr[4], line)
ignorelist = append(ignorelist, IgnoreListEntry{
AddToIgnoreList(IgnoreListEntry{
Path: lineArr[4],
PrefixMatchOnly: false,
})
Expand All @@ -480,12 +493,13 @@ func DetectFilesystemIgnoreList(path string) error {
func RelativeFiles(fp string, root string) ([]string, error) {
var files []string
fullPath := filepath.Join(root, fp)
cleanedRoot := filepath.Clean(root)
logrus.Debugf("Getting files and contents at root %s for %s", root, fullPath)
err := filepath.Walk(fullPath, func(path string, info os.FileInfo, err error) error {
if err != nil {
return err
}
if CheckIgnoreList(path) && !HasFilepathPrefix(path, root, false) {
if CheckCleanedPathAgainstIgnoreList(path) && !hasCleanedFilepathPrefix(filepath.Clean(path), cleanedRoot, false) {
return nil
}
relPath, err := filepath.Rel(root, path)
Expand Down Expand Up @@ -591,7 +605,7 @@ func CreateFile(path string, reader io.Reader, perm os.FileMode, uid uint32, gid
// AddVolumePath adds the given path to the volume ignorelist.
func AddVolumePathToIgnoreList(path string) {
logrus.Infof("Adding volume %s to ignorelist", path)
ignorelist = append(ignorelist, IgnoreListEntry{
AddToIgnoreList(IgnoreListEntry{
Path: path,
PrefixMatchOnly: true,
})
Expand Down Expand Up @@ -778,11 +792,12 @@ func (c FileContext) ExcludesFile(path string) bool {

// HasFilepathPrefix checks if the given file path begins with prefix
func HasFilepathPrefix(path, prefix string, prefixMatchOnly bool) bool {
prefix = filepath.Clean(prefix)
return hasCleanedFilepathPrefix(filepath.Clean(path), filepath.Clean(prefix), prefixMatchOnly)
}

func hasCleanedFilepathPrefix(path, prefix string, prefixMatchOnly bool) bool {
prefixArray := strings.Split(prefix, "/")
path = filepath.Clean(path)
pathArray := strings.SplitN(path, "/", len(prefixArray)+1)

if len(pathArray) < len(prefixArray) {
return false
}
Expand Down Expand Up @@ -968,7 +983,7 @@ func CopyOwnership(src string, destDir string, root string) error {
}
destPath := filepath.Join(destDir, relPath)

if CheckIgnoreList(src) && CheckIgnoreList(destPath) {
if CheckCleanedPathAgainstIgnoreList(src) && CheckCleanedPathAgainstIgnoreList(destPath) {
if !isExist(destPath) {
logrus.Debugf("Path %s ignored, but not exists", destPath)
return nil
Expand All @@ -979,7 +994,7 @@ func CopyOwnership(src string, destDir string, root string) error {
logrus.Debugf("Not copying ownership for %s, as it's ignored", destPath)
return nil
}
if CheckIgnoreList(destDir) && CheckIgnoreList(path) {
if CheckIgnoreList(destDir) && CheckCleanedPathAgainstIgnoreList(path) {
if !isExist(path) {
logrus.Debugf("Path %s ignored, but not exists", path)
return nil
Expand Down Expand Up @@ -1118,7 +1133,7 @@ func GetFSInfoMap(dir string, existing map[string]os.FileInfo) (map[string]os.Fi
timer := timing.Start("Walking filesystem with Stat")
godirwalk.Walk(dir, &godirwalk.Options{
Callback: func(path string, ent *godirwalk.Dirent) error {
if CheckIgnoreList(path) {
if CheckCleanedPathAgainstIgnoreList(path) {
if IsDestDir(path) {
logrus.Tracef("Skipping paths under %s, as it is a ignored directory", path)
return filepath.SkipDir
Expand Down
4 changes: 2 additions & 2 deletions pkg/util/fs_util_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -810,7 +810,7 @@ func TestExtractFile(t *testing.T) {
defer os.RemoveAll(r)

for _, hdr := range tc.hdrs {
if err := ExtractFile(r, hdr, bytes.NewReader(tc.contents)); err != nil {
if err := ExtractFile(r, hdr, filepath.Clean(hdr.Name), bytes.NewReader(tc.contents)); err != nil {
t.Fatal(err)
}
}
Expand Down Expand Up @@ -1008,7 +1008,7 @@ func Test_CopyFile_skips_self(t *testing.T) {
}
}

func fakeExtract(dest string, hdr *tar.Header, tr io.Reader) error {
func fakeExtract(_ string, _ *tar.Header, _ string, _ io.Reader) error {
return nil
}

Expand Down

0 comments on commit 32ce1bf

Please sign in to comment.