From 42b2e906efa52619627635e8e88a9faf7083dece Mon Sep 17 00:00:00 2001 From: Colin Cross Date: Tue, 31 Oct 2023 16:16:21 -0700 Subject: [PATCH] Optimize NinjaEscapeList to avoid allocating an output slice NinjaEscapeList is called on every input or output of a rule, and most of the time does not escape anything. Optimize it by returning the input slice when nothing was escaped. This avoids 1.336 GB of allocations in my AOSP aosp_cf_x86_64_phone-userdebug build. Test: TestNinjaEscapeList Change-Id: I33b9e7b77b33d10401d1ec3546caa6794c567b16 --- proptools/escape.go | 64 +++++++++++++++++++-------- proptools/escape_test.go | 93 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 139 insertions(+), 18 deletions(-) diff --git a/proptools/escape.go b/proptools/escape.go index 9443a95..9792444 100644 --- a/proptools/escape.go +++ b/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 -// new slice containing the escaped strings is returned. +// on strings from properties in Blueprint files that are used as Args to ModuleContext.Build. If +// 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 -// new slice containing the escaped strings is returned. +// on strings from properties in Blueprint files that are used as Args to ModuleContext.Build. 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 -// quote, and then a single quote to restarting quoting. A new slice containing the escaped strings -// is returned. +// 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. If escaping modified any of the strings then a +// 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 { diff --git a/proptools/escape_test.go b/proptools/escape_test.go index 5823a05..72e3ddc 100644 --- a/proptools/escape_test.go +++ b/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") + } + } + }) + } + }) + } +}