From 5702a89db6ce055c243a4197ba80738028aa5792 Mon Sep 17 00:00:00 2001 From: Brian Goff Date: Wed, 22 Apr 2020 10:51:03 -0700 Subject: [PATCH] Use strings.Index instead of strings.Split Since we don't need the actual split values, instead of calling `strings.Split`, which allocates new slices on each call, use `strings.Index`. This significantly reduces the allocations required when doing env value replacements. Additionally, pre-allocate the env var slice, even if we allocate a little more than we need, it keeps us from having to do multiple allocations while appending. ``` benchmark old ns/op new ns/op delta BenchmarkReplaceOrAppendEnvValues/0-8 486 313 -35.60% BenchmarkReplaceOrAppendEnvValues/100-8 10553 1535 -85.45% BenchmarkReplaceOrAppendEnvValues/1000-8 94275 12758 -86.47% BenchmarkReplaceOrAppendEnvValues/10000-8 1161268 129269 -88.87% benchmark old allocs new allocs delta BenchmarkReplaceOrAppendEnvValues/0-8 5 2 -60.00% BenchmarkReplaceOrAppendEnvValues/100-8 110 0 -100.00% BenchmarkReplaceOrAppendEnvValues/1000-8 1013 0 -100.00% BenchmarkReplaceOrAppendEnvValues/10000-8 10022 0 -100.00% benchmark old bytes new bytes delta BenchmarkReplaceOrAppendEnvValues/0-8 192 24 -87.50% BenchmarkReplaceOrAppendEnvValues/100-8 7360 0 -100.00% BenchmarkReplaceOrAppendEnvValues/1000-8 64832 0 -100.00% BenchmarkReplaceOrAppendEnvValues/10000-8 1146049 0 -100.00% ``` Signed-off-by: Brian Goff --- container/container.go | 18 ++++++++++----- container/env.go | 12 +++++----- container/env_test.go | 51 +++++++++++++++++++++++++++++++++++++++++- 3 files changed, 69 insertions(+), 12 deletions(-) diff --git a/container/container.go b/container/container.go index d7d6222b21..5610c9b431 100644 --- a/container/container.go +++ b/container/container.go @@ -723,12 +723,20 @@ func (container *Container) CreateDaemonEnvironment(tty bool, linkedEnv []string if os == "" { os = runtime.GOOS } - env := []string{} + + // Figure out what size slice we need so we can allocate this all at once. + envSize := len(container.Config.Env) if runtime.GOOS != "windows" || (runtime.GOOS == "windows" && os == "linux") { - env = []string{ - "PATH=" + system.DefaultPathEnv(os), - "HOSTNAME=" + container.Config.Hostname, - } + envSize += 2 + len(linkedEnv) + } + if tty { + envSize++ + } + + env := make([]string, 0, envSize) + if runtime.GOOS != "windows" || (runtime.GOOS == "windows" && os == "linux") { + env = append(env, "PATH="+system.DefaultPathEnv(os)) + env = append(env, "HOSTNAME="+container.Config.Hostname) if tty { env = append(env, "TERM=xterm") } diff --git a/container/env.go b/container/env.go index d225fd1471..e31088c885 100644 --- a/container/env.go +++ b/container/env.go @@ -9,22 +9,22 @@ import ( func ReplaceOrAppendEnvValues(defaults, overrides []string) []string { cache := make(map[string]int, len(defaults)) for i, e := range defaults { - parts := strings.SplitN(e, "=", 2) - cache[parts[0]] = i + index := strings.Index(e, "=") + cache[e[:index]] = i } for _, value := range overrides { // Values w/o = means they want this env to be removed/unset. - if !strings.Contains(value, "=") { + index := strings.Index(value, "=") + if index < 0 { + // no "=" in value if i, exists := cache[value]; exists { defaults[i] = "" // Used to indicate it should be removed } continue } - // Just do a normal set/update - parts := strings.SplitN(value, "=", 2) - if i, exists := cache[parts[0]]; exists { + if i, exists := cache[value[:index]]; exists { defaults[i] = value } else { defaults = append(defaults, value) diff --git a/container/env_test.go b/container/env_test.go index 77856284c2..432d43a4b2 100644 --- a/container/env_test.go +++ b/container/env_test.go @@ -1,6 +1,11 @@ package container // import "github.com/docker/docker/container" -import "testing" +import ( + "crypto/rand" + "testing" + + "gotest.tools/v3/assert" +) func TestReplaceAndAppendEnvVars(t *testing.T) { var ( @@ -22,3 +27,47 @@ func TestReplaceAndAppendEnvVars(t *testing.T) { t.Fatalf("expected TERM=xterm got '%s'", env[1]) } } + +func BenchmarkReplaceOrAppendEnvValues(b *testing.B) { + b.Run("0", func(b *testing.B) { + benchmarkReplaceOrAppendEnvValues(b, 0) + }) + b.Run("100", func(b *testing.B) { + benchmarkReplaceOrAppendEnvValues(b, 100) + }) + b.Run("1000", func(b *testing.B) { + benchmarkReplaceOrAppendEnvValues(b, 1000) + }) + b.Run("10000", func(b *testing.B) { + benchmarkReplaceOrAppendEnvValues(b, 10000) + }) +} + +func benchmarkReplaceOrAppendEnvValues(b *testing.B, extraEnv int) { + b.StopTimer() + // remove FOO from env + // remove BAR from env (nop) + o := []string{"HOME=/root", "TERM=xterm", "FOO", "BAR"} + + if extraEnv > 0 { + buf := make([]byte, 5) + for i := 0; i < extraEnv; i++ { + n, err := rand.Read(buf) + assert.NilError(b, err) + key := string(buf[:n]) + + n, err = rand.Read(buf) + assert.NilError(b, err) + val := string(buf[:n]) + + o = append(o, key+"="+val) + } + } + d := make([]string, 0, len(o)+2) + d = append(d, []string{"HOME=/", "FOO=foo_default"}...) + + b.StartTimer() + for i := 0; i < b.N; i++ { + _ = ReplaceOrAppendEnvValues(d, o) + } +}