dirent: no longer cache the results of Stat and Info

This commit changes fastwalk.DirEntry to no longer cache the result of
calling the Stat and Info methods.

Author: charlievieth

dirent.go

@@ -3,51 +3,26 @@ package fastwalk
 import (
 	"io/fs"
 	"os"
-	"sync"
-	"sync/atomic"
 	"syscall"
-	"unsafe"
 )
 
-type fileInfo struct {
-	once sync.Once
-	fs.FileInfo
-	err error
-}
-
-func loadFileInfo(pinfo **fileInfo) *fileInfo {
-	ptr := (*unsafe.Pointer)(unsafe.Pointer(pinfo))
-	fi := (*fileInfo)(atomic.LoadPointer(ptr))
-	if fi == nil {
-		fi = &fileInfo{}
-		if !atomic.CompareAndSwapPointer(
-			(*unsafe.Pointer)(unsafe.Pointer(pinfo)), // adrr
-			nil,                                      // old
-			unsafe.Pointer(fi),                       // new
-		) {
-			fi = (*fileInfo)(atomic.LoadPointer(ptr))
-		}
-	}
-	return fi
-}
-
-// StatDirEntry returns a [fs.FileInfo] describing the named file ([os.Stat]).
-// If de is a [fastwalk.DirEntry] its Stat method is used and the returned
-// FileInfo may be cached from a prior call to Stat. If a cached result is not
-// desired, users should just call [os.Stat] directly.
+// StatDirEntry returns a [fs.FileInfo] describing the named file. If de is a
+// [fastwalk.DirEntry] its Stat method is used. Otherwise, [os.Stat] is called
+// on the path. Therefore, de should be the DirEntry describing path.
 //
-// This is a helper function for calling Stat on the DirEntry passed to the
-// walkFn argument to [Walk].
+// Note: This function was added when fastwalk used to always cache the result
+// of DirEntry.Stat. Now that fastwalk no longer explicitly caches the result
+// of Stat this function is slightly less useful and is equivalent to the below
+// code:
 //
-// The path argument is only used if de is not of type [fastwalk.DirEntry].
-// Therefore, de should be the DirEntry describing path.
+//	if d, ok := de.(DirEntry); ok {
+//		return d.Stat()
+//	}
+//	return os.Stat(path)
 func StatDirEntry(path string, de fs.DirEntry) (fs.FileInfo, error) {
 	if de == nil {
 		return nil, &os.PathError{Op: "stat", Path: path, Err: syscall.EINVAL}
 	}
-	if de.Type()&os.ModeSymlink == 0 {
-		return de.Info()
-	}
 	if d, ok := de.(DirEntry); ok {
 		return d.Stat()
 	}

dirent_portable.go

@@ -8,6 +8,7 @@ package fastwalk
 import (
 	"io/fs"
 	"os"
+	"runtime"
 	"sort"
 	"sync"
 
@@ -19,7 +20,6 @@ var _ DirEntry = (*portableDirent)(nil)
 type portableDirent struct {
 	fs.DirEntry
 	parent string
-	stat   *fileInfo
 	depth  uint32
 }
 
@@ -32,14 +32,14 @@ func (d *portableDirent) Depth() int {
 }
 
 func (d *portableDirent) Stat() (fs.FileInfo, error) {
-	if d.DirEntry.Type()&os.ModeSymlink == 0 {
-		return d.DirEntry.Info()
+	if runtime.GOOS == "windows" {
+		// On Windows use Info() if the file is not a symlink since
+		// IIRC the result is cached when the FileInfo is created.
+		if d.DirEntry.Type()&fs.ModeSymlink != 0 {
+			return d.Info()
+		}
 	}
-	stat := loadFileInfo(&d.stat)
-	stat.once.Do(func() {
-		stat.FileInfo, stat.err = os.Stat(d.parent + string(os.PathSeparator) + d.Name())
-	})
-	return stat.FileInfo, stat.err
+	return os.Stat(d.parent + string(os.PathSeparator) + d.Name())
 }
 
 func newDirEntry(dirName string, info fs.DirEntry, depth int) DirEntry {

dirent_test.go

@@ -6,8 +6,6 @@ import (
 	"io/fs"
 	"os"
 	"path/filepath"
-	"runtime"
-	"sync"
 	"testing"
 
 	"github.com/charlievieth/fastwalk"
@@ -75,91 +73,5 @@ func TestDirent(t *testing.T) {
 			t.Errorf("lstat mismatch\n got:\n%s\nwant:\n%s",
 				fastwalk.FormatFileInfo(got), fastwalk.FormatFileInfo(want))
 		}
-		fi, err := fileEnt.Info()
-		if err != nil {
-			t.Fatal(err)
-		}
-		if fi != got {
-			t.Error("failed to return or cache FileInfo")
-		}
-		de := fileEnt.(fastwalk.DirEntry)
-		fi, err = de.Stat()
-		if err != nil {
-			t.Fatal(err)
-		}
-		if fi != got {
-			t.Error("failed to use cached Info result for non-symlink")
-		}
-	})
-
-	t.Run("Parallel", func(t *testing.T) {
-		testParallel := func(t *testing.T, de fs.DirEntry, fn func() (fs.FileInfo, error)) {
-			numCPU := runtime.NumCPU()
-
-			infos := make([][]fs.FileInfo, numCPU)
-			for i := range infos {
-				infos[i] = make([]fs.FileInfo, 100)
-			}
-
-			// Start all the goroutines at the same time to
-			// maximise the chance of a race
-			start := make(chan struct{})
-			var wg, ready sync.WaitGroup
-			ready.Add(numCPU)
-			wg.Add(numCPU)
-			for i := 0; i < numCPU; i++ {
-				go func(fis []fs.FileInfo, de fs.DirEntry) {
-					defer wg.Done()
-					ready.Done()
-					<-start
-					for i := range fis {
-						fis[i], _ = de.Info()
-					}
-				}(infos[i], de)
-			}
-
-			ready.Wait()
-			close(start) // start all goroutines at once
-			wg.Wait()
-
-			first := infos[0][0]
-			if first == nil {
-				t.Fatal("failed to stat file:", de.Name())
-			}
-			for _, fis := range infos {
-				for _, fi := range fis {
-					if fi != first {
-						t.Errorf("Expected the same fs.FileInfo to always "+
-							"be returned got: %#v want: %#v", fi, first)
-					}
-				}
-			}
-		}
-
-		t.Run("File", func(t *testing.T) {
-			t.Run("Stat", func(t *testing.T) {
-				_, fileEnt := getDirEnts(t)
-				de := fileEnt.(fastwalk.DirEntry)
-				testParallel(t, de, de.Stat)
-			})
-			t.Run("Lstat", func(t *testing.T) {
-				_, fileEnt := getDirEnts(t)
-				de := fileEnt.(fastwalk.DirEntry)
-				testParallel(t, de, de.Info)
-			})
-		})
-
-		t.Run("Link", func(t *testing.T) {
-			t.Run("Stat", func(t *testing.T) {
-				linkEnt, _ := getDirEnts(t)
-				de := linkEnt.(fastwalk.DirEntry)
-				testParallel(t, de, de.Stat)
-			})
-			t.Run("Lstat", func(t *testing.T) {
-				linkEnt, _ := getDirEnts(t)
-				de := linkEnt.(fastwalk.DirEntry)
-				testParallel(t, de, de.Info)
-			})
-		})
 	})
 }

dirent_unix.go

@@ -16,8 +16,6 @@ type unixDirent struct {
 	name   string
 	typ    fs.FileMode
 	depth  uint32 // uint32 so that we can pack it next to typ
-	info   *fileInfo
-	stat   *fileInfo
 }
 
 func (d *unixDirent) Name() string      { return d.name }
@@ -27,22 +25,11 @@ func (d *unixDirent) Depth() int        { return int(d.depth) }
 func (d *unixDirent) String() string    { return fmtdirent.FormatDirEntry(d) }
 
 func (d *unixDirent) Info() (fs.FileInfo, error) {
-	info := loadFileInfo(&d.info)
-	info.once.Do(func() {
-		info.FileInfo, info.err = os.Lstat(d.parent + "/" + d.name)
-	})
-	return info.FileInfo, info.err
+	return os.Lstat(d.parent + "/" + d.name)
 }
 
 func (d *unixDirent) Stat() (fs.FileInfo, error) {
-	if d.typ&os.ModeSymlink == 0 {
-		return d.Info()
-	}
-	stat := loadFileInfo(&d.stat)
-	stat.once.Do(func() {
-		stat.FileInfo, stat.err = os.Stat(d.parent + "/" + d.name)
-	})
-	return stat.FileInfo, stat.err
+	return os.Stat(d.parent + "/" + d.name)
 }
 
 func newUnixDirent(parent, name string, typ fs.FileMode, depth int) *unixDirent {
@@ -55,16 +42,7 @@ func newUnixDirent(parent, name string, typ fs.FileMode, depth int) *unixDirent
 }
 
 func fileInfoToDirEntry(dirname string, fi fs.FileInfo) DirEntry {
-	info := &fileInfo{
-		FileInfo: fi,
-	}
-	info.once.Do(func() {})
-	return &unixDirent{
-		parent: dirname,
-		name:   fi.Name(),
-		typ:    fi.Mode().Type(),
-		info:   info,
-	}
+	return newUnixDirent(dirname, fi.Name(), fi.Mode().Type(), 0)
 }
 
 var direntSlicePool = sync.Pool{

dirent_unix_test.go

@@ -10,10 +10,8 @@ import (
 	"reflect"
 	"runtime"
 	"sync"
-	"sync/atomic"
 	"testing"
 	"time"
-	"unsafe"
 )
 
 func testUnixDirentParallel(t *testing.T, ent *unixDirent, want fs.FileInfo,
@@ -23,7 +21,7 @@ func testUnixDirentParallel(t *testing.T, ent *unixDirent, want fs.FileInfo,
 		return fi1.Name() == fi2.Name() &&
 			fi1.Size() == fi2.Size() &&
 			fi1.Mode() == fi2.Mode() &&
-			fi1.ModTime() == fi2.ModTime() &&
+			fi1.ModTime().Equal(fi2.ModTime()) &&
 			fi1.IsDir() == fi2.IsDir() &&
 			os.SameFile(fi1, fi2)
 	}
@@ -38,10 +36,6 @@ func testUnixDirentParallel(t *testing.T, ent *unixDirent, want fs.FileInfo,
 
 	var wg sync.WaitGroup
 	start := make(chan struct{})
-	var mu sync.Mutex
-	infos := make(map[*fileInfo]int)
-	stats := make(map[*fileInfo]int)
-
 	for i := 0; i < numCPU; i++ {
 		wg.Add(1)
 		go func() {
@@ -53,12 +47,6 @@ func testUnixDirentParallel(t *testing.T, ent *unixDirent, want fs.FileInfo,
 					t.Error(err)
 					return
 				}
-				info := (*fileInfo)(atomic.LoadPointer((*unsafe.Pointer)(unsafe.Pointer(&ent.info))))
-				stat := (*fileInfo)(atomic.LoadPointer((*unsafe.Pointer)(unsafe.Pointer(&ent.stat))))
-				mu.Lock()
-				infos[info]++
-				stats[stat]++
-				mu.Unlock()
 				if !sameFile(fi, want) {
 					t.Errorf("FileInfo not equal:\nwant: %s\ngot:  %s\n",
 						FormatFileInfo(want), FormatFileInfo(fi))
@@ -70,8 +58,6 @@ func testUnixDirentParallel(t *testing.T, ent *unixDirent, want fs.FileInfo,
 
 	close(start)
 	wg.Wait()
-
-	t.Logf("Infos: %d Stats: %d\n", len(infos), len(stats))
 }
 
 func TestUnixDirent(t *testing.T) {
@@ -222,24 +208,6 @@ func TestSortDirents(t *testing.T) {
 	})
 }
 
-func BenchmarkUnixDirentLoadFileInfo(b *testing.B) {
-	wd, err := os.Getwd()
-	if err != nil {
-		b.Fatal(err)
-	}
-	fi, err := os.Lstat(wd)
-	if err != nil {
-		b.Fatal(err)
-	}
-	parent, name := filepath.Split(wd)
-	d := newUnixDirent(parent, name, fi.Mode().Type(), 0)
-
-	for i := 0; i < b.N; i++ {
-		loadFileInfo(&d.info)
-		d.info = nil
-	}
-}
-
 func BenchmarkUnixDirentInfo(b *testing.B) {
 	wd, err := os.Getwd()
 	if err != nil {
@@ -250,38 +218,26 @@ func BenchmarkUnixDirentInfo(b *testing.B) {
 		b.Fatal(err)
 	}
 	parent, name := filepath.Split(wd)
-	d := newUnixDirent(parent, name, fi.Mode().Type(), 0)
 
-	for i := 0; i < b.N; i++ {
-		fi, err := d.Info()
-		if err != nil {
-			b.Fatal(err)
-		}
-		if fi == nil {
-			b.Fatal("Nil FileInfo")
+	b.Run("Serial", func(b *testing.B) {
+		d := newUnixDirent(parent, name, fi.Mode().Type(), 0)
+		for i := 0; i < b.N; i++ {
+			_, err := d.Info()
+			if err != nil {
+				b.Fatal(err)
+			}
 		}
-	}
-}
-
-func BenchmarkUnixDirentStat(b *testing.B) {
-	wd, err := os.Getwd()
-	if err != nil {
-		b.Fatal(err)
-	}
-	fi, err := os.Lstat(wd)
-	if err != nil {
-		b.Fatal(err)
-	}
-	parent, name := filepath.Split(wd)
-	d := newUnixDirent(parent, name, fi.Mode().Type(), 0)
+	})
 
-	for i := 0; i < b.N; i++ {
-		fi, err := d.Stat()
-		if err != nil {
-			b.Fatal(err)
-		}
-		if fi == nil {
-			b.Fatal("Nil FileInfo")
-		}
-	}
+	b.Run("Parallel", func(b *testing.B) {
+		b.RunParallel(func(pb *testing.PB) {
+			d := newUnixDirent(parent, name, fi.Mode().Type(), 0)
+			for pb.Next() {
+				_, err := d.Info()
+				if err != nil {
+					b.Fatal(err)
+				}
+			}
+		})
+	})
 }