Merge pull request #875 from Shopify/watcher_cpu_optimization

CPU usage optimization in watch command

changelog.txt

@@ -2,6 +2,7 @@ v1.1.5
 ======
 - Fix layout after renaming application.scss.css to application.css
 - Added more to customer templates
+- Optimization for watcher to reduce CPU usage.
 
 v1.1.4 (Dec 3, 2020)
 ====================

go.mod

@@ -13,7 +13,7 @@ require (
 	github.com/joho/godotenv v1.3.0
 	github.com/mattn/go-colorable v0.0.0-20180310133214-efa589957cd0
 	github.com/mattn/go-isatty v0.0.4 // indirect
-	github.com/radovskyb/watcher v1.0.6
+	github.com/radovskyb/watcher v1.0.7
 	github.com/ryanuber/go-glob v0.0.0-20160226084822-572520ed46db
 	github.com/shibukawa/configdir v0.0.0-20170330084843-e180dbdc8da0
 	github.com/skratchdot/open-golang v0.0.0-20160302144031-75fb7ed4208c

go.sum

@@ -34,6 +34,8 @@ github.com/radovskyb/watcher v1.0.2 h1:9L5TsZUbo1nKhQEQPtICVc+x9UZQ6VPdBepLHyGw/
 github.com/radovskyb/watcher v1.0.2/go.mod h1:78okwvY5wPdzcb1UYnip1pvrZNIVEIh/Cm+ZuvsUYIg=
 github.com/radovskyb/watcher v1.0.6 h1:8WIQ9UxEYMZjem1OwU7dVH94DXXk9mAIE1i8eqHD+IY=
 github.com/radovskyb/watcher v1.0.6/go.mod h1:78okwvY5wPdzcb1UYnip1pvrZNIVEIh/Cm+ZuvsUYIg=
+github.com/radovskyb/watcher v1.0.7 h1:AYePLih6dpmS32vlHfhCeli8127LzkIgwJGcwwe8tUE=
+github.com/radovskyb/watcher v1.0.7/go.mod h1:78okwvY5wPdzcb1UYnip1pvrZNIVEIh/Cm+ZuvsUYIg=
 github.com/ryanuber/go-glob v0.0.0-20160226084822-572520ed46db h1:ge9atzKq16843f793fDVxKUhmTb4H5muzjJQ6PgsnHg=
 github.com/ryanuber/go-glob v0.0.0-20160226084822-572520ed46db/go.mod h1:807d1WSdnB0XRJzKNil9Om6lcp/3a0v4qIHxIXzX/Yc=
 github.com/shibukawa/configdir v0.0.0-20170330084843-e180dbdc8da0 h1:Xuk8ma/ibJ1fOy4Ee11vHhUFHQNpHhrBneOCNHVXS5w=

src/file/watcher.go

@@ -6,7 +6,6 @@ import (
 	"io"
 	"os"
 	"path/filepath"
-	"strings"
 	"time"
 
 	"github.com/Shopify/themekit/src/env"
@@ -25,7 +24,6 @@ const (
 	Skip
 	// Get is when a file should be re-fetched, used in download operations
 	Get
-	filepathSplit = " -> "
 )
 
 var (
@@ -67,9 +65,15 @@ func NewWatcher(e *env.Env, configPath string, checksums map[string]string) (*Wa
 	}
 	fsWatcher.AddFilterHook(hook)
 
-	if err := fsWatcher.AddRecursive(e.Directory); err != nil {
+	if err := fsWatcher.Add(e.Directory); err != nil {
 		return nil, fmt.Errorf("Could not watch directory: %s", err)
 	}
+	for _, folder := range assetLocations {
+		path := filepath.Join(e.Directory, folder)
+		if err := fsWatcher.Add(path); err != nil && !os.IsNotExist(err) {
+			return nil, fmt.Errorf("Could not watch directory %s: %s", path, err)
+		}
+	}
 
 	return &Watcher{
 		Events:    make(chan Event),
@@ -85,7 +89,7 @@ func filterHook(e *env.Env, configPath string) (watcher.FilterFileHookFunc, erro
 		return nil, err
 	}
 	return func(info os.FileInfo, fullPath string) error {
-		if info.IsDir() || (configPath != fullPath && filter.Match(fullPath)) {
+		if configPath != fullPath && filter.Match(fullPath) {
 			return watcher.ErrSkip
 		}
 		return nil
@@ -109,11 +113,21 @@ func (w *Watcher) watchFsEvents() {
 		case <-w.fsWatcher.Closed:
 			w.Stop()
 			return
+		case <-w.fsWatcher.Error:
+			// discard errors, they are not useful for users and the watcher deadlocks
+			// if they are not read. The expected errors come from a directory being
+			// deleted while being watched
 		}
 	}
 }
 
 func (w *Watcher) onEvent(event watcher.Event) bool {
+	if event.IsDir() {
+		if isEventType(event.Op, watcher.Create) {
+			w.fsWatcher.Add(event.Path)
+		}
+		return false
+	}
 	events := map[string]Event{}
 	for _, event := range w.translateEvent(event) {
 		events[event.Path] = event
@@ -149,7 +163,7 @@ func (w *Watcher) updateChecksum(e Event) {
 }
 
 func (w *Watcher) translateEvent(event watcher.Event) []Event {
-	oldPath, currentPath := w.parsePath(event.Path)
+	oldPath, currentPath := w.parsePath(event.OldPath), w.parsePath(event.Path)
 	if isEventType(event.Op, watcher.Rename, watcher.Move) {
 		return []Event{{Op: Remove, Path: oldPath}, {Op: Update, Path: currentPath, LastKnownChecksum: w.checksums[currentPath]}}
 	} else if isEventType(event.Op, watcher.Remove) {
@@ -165,19 +179,12 @@ func (w *Watcher) translateEvent(event watcher.Event) []Event {
 	return []Event{}
 }
 
-func (w *Watcher) parsePath(path string) (old, current string) {
-	parts := strings.Split(path, filepathSplit)
-	for i, path := range parts {
-		projectPath := pathToProject(w.directory, path)
-		if projectPath == "" {
-			projectPath = path
-		}
-		parts[i] = projectPath
-	}
-	if len(parts) > 1 {
-		return parts[0], parts[1]
+func (w *Watcher) parsePath(path string) string {
+	projectPath := pathToProject(w.directory, path)
+	if projectPath == "" {
+		return path
 	}
-	return "", parts[0]
+	return projectPath
 }
 
 func isEventType(currentOp watcher.Op, allowedOps ...watcher.Op) bool {

src/file/watcher_test.go

@@ -80,7 +80,9 @@ func TestFileWatcher_filterHook(t *testing.T) {
 		{filename: "_testdata/project/config/settings.json", skip: true},
 		{filename: "_testdata/project/config.yml", skip: false},
 		{filename: "_testdata/project/templates/template.liquid", skip: false},
-		{filename: "_testdata/project/templates", skip: true},
+		{filename: "_testdata/project/templates", skip: false},
+		{filename: "_testdata", skip: true},
+		{filename: "_testdata/project/javascripts/app.js", skip: true},
 	}
 
 	hook := createTestFilterHook(t)
@@ -105,7 +107,6 @@ func TestFileWatcher_WatchFsEvents(t *testing.T) {
 		{expectedOp: Update, filename: "_testdata/project/templates/template.liquid", op: watcher.Write},
 		{expectedOp: Update, filename: "_testdata/project/templates/customers/test.liquid", op: watcher.Write},
 		{expectedOp: Remove, filename: "_testdata/project/templates/customers/test.liquid", op: watcher.Remove},
-		{expectedOp: Remove, filename: "_testdata/project/templates/customers/test.liquid", op: watcher.Rename},
 	}
 
 	for i, testcase := range testcases {
@@ -130,23 +131,23 @@ func (p OptSlice) Swap(i, j int)      { p[i], p[j] = p[j], p[i] }
 
 func TestFileWatcher_OnEvent(t *testing.T) {
 	testcases := []struct {
-		filename   string
-		op         watcher.Op
-		expectedOp OptSlice
+		filename, oldname string
+		op                watcher.Op
+		expectedOp        OptSlice
 	}{
 		{expectedOp: OptSlice{Update}, filename: "_testdata/project/templates/template.liquid", op: watcher.Write},
 		{expectedOp: OptSlice{Update}, filename: "_testdata/project/templates/customers/test.liquid", op: watcher.Create},
+		{expectedOp: OptSlice{}, filename: "_testdata/project/config", op: watcher.Create},
 		{expectedOp: OptSlice{Remove}, filename: "_testdata/project/templates/customers/test.liquid", op: watcher.Remove},
-		{expectedOp: OptSlice{Remove, Update}, filename: "_testdata/project/assets/application.js.liquid -> _testdata/project/assets/application.js", op: watcher.Rename},
-		{expectedOp: OptSlice{Remove, Update}, filename: "_testdata/project/assets/application.js.liquid -> _testdata/project/assets/application.js", op: watcher.Move},
+		{expectedOp: OptSlice{Remove, Update}, filename: "_testdata/project/assets/application.js.liquid", oldname: "_testdata/project/assets/application.js", op: watcher.Rename},
+		{expectedOp: OptSlice{Remove, Update}, filename: "_testdata/project/assets/application.js.liquid", oldname: "_testdata/project/assets/application.js", op: watcher.Move},
 	}
 
 	for i, testcase := range testcases {
 		w := createTestWatcher(t)
 		w.Events = make(chan Event, len(testcases))
-		_, currentPath := w.parsePath(testcase.filename)
-		info, _ := os.Stat(filepath.Join("_testdata", "project", currentPath))
-		w.onEvent(watcher.Event{Op: testcase.op, Path: testcase.filename, FileInfo: info})
+		info, _ := os.Stat(testcase.filename)
+		w.onEvent(watcher.Event{Op: testcase.op, Path: testcase.filename, OldPath: testcase.oldname, FileInfo: info})
 		assert.Equal(t, len(testcase.expectedOp), len(w.Events), fmt.Sprintf("testcase: %v", i))
 
 		recievedEvents := OptSlice{}
@@ -183,17 +184,14 @@ func TestFileWatcher_debouncing(t *testing.T) {
 
 func TestFileWatcher_ParsePath(t *testing.T) {
 	testcases := []struct {
-		input, oldpath, currentpath string
+		input, currentpath string
 	}{
-		{"_testdata/project/assets/app.js", "", "assets/app.js"},
-		{"_testdata/project/assets/app.js -> _testdata/project/assets/app.js.liquid", "assets/app.js", "assets/app.js.liquid"},
-		{"not/another/path/assets/app.js", "", "not/another/path/assets/app.js"},
+		{"_testdata/project/assets/app.js", "assets/app.js"},
+		{"not/another/path/assets/app.js", "not/another/path/assets/app.js"},
 	}
 	w := createTestWatcher(t)
 	for _, testcase := range testcases {
-		o, c := w.parsePath(testcase.input)
-		assert.Equal(t, o, testcase.oldpath)
-		assert.Equal(t, c, testcase.currentpath)
+		assert.Equal(t, w.parsePath(testcase.input), testcase.currentpath)
 	}
 }