Optimize NinjaEscapeList to avoid allocating an output slice am: 42b2e906ef am: d6b9e69e76

Original change: https://android-review.googlesource.com/c/platform/build/blueprint/+/2813841

Change-Id: I21300a6179ecaaaaa7fe20cf9c9eabfcc4be776b
Signed-off-by: Automerger Merge Worker <android-build-automerger-merge-worker@system.gserviceaccount.com>

proptools/escape.go

@@ -14,26 +14,38 @@
 
 package proptools
 
-import "strings"
+import (
+	"strings"
+	"unsafe"
+)
 
 // NinjaEscapeList takes a slice of strings that may contain characters that are meaningful to ninja
 // ($), and escapes each string so they will be passed to bash.  It is not necessary on input,
 // output, or dependency names, those are handled by ModuleContext.Build.  It is generally required
-// on strings from properties in Blueprint files that are used as Args to ModuleContext.Build.  A
+// on strings from properties in Blueprint files that are used as Args to ModuleContext.Build.  If
-// new slice containing the escaped strings is returned.
+// escaping modified any of the strings then a new slice containing the escaped strings is returned,
+// otherwise the original slice is returned.
 func NinjaEscapeList(slice []string) []string {
-	slice = append([]string(nil), slice...)
+	sliceCopied := false
 	for i, s := range slice {
-		slice[i] = NinjaEscape(s)
+		escaped := NinjaEscape(s)
+		if unsafe.StringData(s) != unsafe.StringData(escaped) {
+			if !sliceCopied {
+				// If this was the first string that was modified by escaping then make a copy of the
+				// input slice to use as the output slice.
+				slice = append([]string(nil), slice...)
+				sliceCopied = true
+			}
+			slice[i] = escaped
+		}
 	}
 	return slice
 }
 
-// NinjaEscapeList takes a string that may contain characters that are meaningful to ninja
+// NinjaEscape takes a string that may contain characters that are meaningful to ninja
 // ($), and escapes it so it will be passed to bash.  It is not necessary on input,
 // output, or dependency names, those are handled by ModuleContext.Build.  It is generally required
-// on strings from properties in Blueprint files that are used as Args to ModuleContext.Build.  A
+// on strings from properties in Blueprint files that are used as Args to ModuleContext.Build.
-// new slice containing the escaped strings is returned.
 func NinjaEscape(s string) string {
 	return ninjaEscaper.Replace(s)
 }
@@ -43,23 +55,39 @@ var ninjaEscaper = strings.NewReplacer(
 
 // ShellEscapeList takes a slice of strings that may contain characters that are meaningful to bash and
 // escapes them if necessary by wrapping them in single quotes, and replacing internal single quotes with
-// '\'' (one single quote to end the quoting, a shell-escaped single quote to insert a real single
+// one single quote to end the quoting, a shell-escaped single quote to insert a real single
-// quote, and then a single quote to restarting quoting.  A new slice containing the escaped strings
+// quote, and then a single quote to restarting quoting.  If escaping modified any of the strings then a
-// is returned.
+// new slice containing the escaped strings is returned, otherwise the original slice is returned.
 func ShellEscapeList(slice []string) []string {
-	slice = append([]string(nil), slice...)
+	sliceCopied := false
-
 	for i, s := range slice {
-		slice[i] = ShellEscape(s)
+		escaped := ShellEscape(s)
+		if unsafe.StringData(s) != unsafe.StringData(escaped) {
+			if !sliceCopied {
+				// If this was the first string that was modified by escaping then make a copy of the
+				// input slice to use as the output slice.
+				slice = append([]string(nil), slice...)
+				sliceCopied = true
+			}
+			slice[i] = escaped
+		}
 	}
 	return slice
 }
 
 func ShellEscapeListIncludingSpaces(slice []string) []string {
-	slice = append([]string(nil), slice...)
+	sliceCopied := false
-
 	for i, s := range slice {
-		slice[i] = ShellEscapeIncludingSpaces(s)
+		escaped := ShellEscapeIncludingSpaces(s)
+		if unsafe.StringData(s) != unsafe.StringData(escaped) {
+			if !sliceCopied {
+				// If this was the first string that was modified by escaping then make a copy of the
+				// input slice to use as the output slice.
+				slice = append([]string(nil), slice...)
+				sliceCopied = true
+			}
+			slice[i] = escaped
+		}
 	}
 	return slice
 }
@@ -84,7 +112,7 @@ func shellUnsafeChar(r rune) bool {
 
 // ShellEscape takes string that may contain characters that are meaningful to bash and
 // escapes it if necessary by wrapping it in single quotes, and replacing internal single quotes with
-// '\'' (one single quote to end the quoting, a shell-escaped single quote to insert a real single
+// one single quote to end the quoting, a shell-escaped single quote to insert a real single
 // quote, and then a single quote to restarting quoting.
 func ShellEscape(s string) string {
 	shellUnsafeCharNotSpace := func(r rune) bool {

proptools/escape_test.go

@@ -16,7 +16,9 @@ package proptools
 
 import (
 	"os/exec"
+	"reflect"
 	"testing"
+	"unsafe"
 )
 
 type escapeTestCase struct {
@@ -167,3 +169,94 @@ func TestExternalShellEscapeIncludingSpaces(t *testing.T) {
 		}
 	}
 }
+
+func TestNinjaEscapeList(t *testing.T) {
+	type testCase struct {
+		name                 string
+		in                   []string
+		ninjaEscaped         []string
+		shellEscaped         []string
+		ninjaAndShellEscaped []string
+		sameSlice            bool
+	}
+	testCases := []testCase{
+		{
+			name:      "empty",
+			in:        []string{},
+			sameSlice: true,
+		},
+		{
+			name:      "nil",
+			in:        nil,
+			sameSlice: true,
+		},
+		{
+			name:      "no escaping",
+			in:        []string{"abc", "def", "ghi"},
+			sameSlice: true,
+		},
+		{
+			name:                 "escape first",
+			in:                   []string{`$\abc`, "def", "ghi"},
+			ninjaEscaped:         []string{`$$\abc`, "def", "ghi"},
+			shellEscaped:         []string{`'$\abc'`, "def", "ghi"},
+			ninjaAndShellEscaped: []string{`'$$\abc'`, "def", "ghi"},
+		},
+		{
+			name:                 "escape middle",
+			in:                   []string{"abc", `$\def`, "ghi"},
+			ninjaEscaped:         []string{"abc", `$$\def`, "ghi"},
+			shellEscaped:         []string{"abc", `'$\def'`, "ghi"},
+			ninjaAndShellEscaped: []string{"abc", `'$$\def'`, "ghi"},
+		},
+		{
+			name:                 "escape last",
+			in:                   []string{"abc", "def", `$\ghi`},
+			ninjaEscaped:         []string{"abc", "def", `$$\ghi`},
+			shellEscaped:         []string{"abc", "def", `'$\ghi'`},
+			ninjaAndShellEscaped: []string{"abc", "def", `'$$\ghi'`},
+		},
+	}
+
+	testFuncs := []struct {
+		name     string
+		f        func([]string) []string
+		expected func(tt testCase) []string
+	}{
+		{name: "NinjaEscapeList", f: NinjaEscapeList, expected: func(tt testCase) []string { return tt.ninjaEscaped }},
+		{name: "ShellEscapeList", f: ShellEscapeList, expected: func(tt testCase) []string { return tt.shellEscaped }},
+		{name: "NinjaAndShellEscapeList", f: NinjaAndShellEscapeList, expected: func(tt testCase) []string { return tt.ninjaAndShellEscaped }},
+	}
+
+	for _, tf := range testFuncs {
+		t.Run(tf.name, func(t *testing.T) {
+			for _, tt := range testCases {
+				t.Run(tt.name, func(t *testing.T) {
+					inCopy := append([]string(nil), tt.in...)
+
+					got := tf.f(tt.in)
+
+					want := tf.expected(tt)
+					if tt.sameSlice {
+						want = tt.in
+					}
+
+					if !reflect.DeepEqual(got, want) {
+						t.Errorf("incorrect output, want %q got %q", want, got)
+					}
+					if len(inCopy) != len(tt.in) && (len(tt.in) == 0 || !reflect.DeepEqual(inCopy, tt.in)) {
+						t.Errorf("input modified, want %#v, got %#v", inCopy, tt.in)
+					}
+
+					if (unsafe.SliceData(tt.in) == unsafe.SliceData(got)) != tt.sameSlice {
+						if tt.sameSlice {
+							t.Errorf("expected input and output slices to have the same backing arrays")
+						} else {
+							t.Errorf("expected input and output slices to have different backing arrays")
+						}
+					}
+				})
+			}
+		})
+	}
+}