From 3aae63f4528ab1d7e1e12cc4e2e7bacd97991d43 Mon Sep 17 00:00:00 2001 From: "Daniel, Dao Quang Minh" Date: Tue, 11 Nov 2014 14:37:47 -0500 Subject: [PATCH 1/5] speed up creation of args and msg for huge cmds Whenever a command arguments is formed by a large linked list, repeatedly appending to arguments and displayed messages took a long time because go will have to allocate/copy a lot of times. This speeds up the allocation by preallocate arrays of correct size for args and msg Docker-DCO-1.1-Signed-off-by: Daniel, Dao Quang Minh (github: dqminh) --- builder/evaluator.go | 23 ++++++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-) diff --git a/builder/evaluator.go b/builder/evaluator.go index 645038bb1d..3f2600e29d 100644 --- a/builder/evaluator.go +++ b/builder/evaluator.go @@ -211,6 +211,21 @@ func (b *Builder) dispatch(stepN int, ast *parser.Node) error { msg += " " + ast.Value } + // count the number of nodes that we are going to traverse first + // so we can pre-create the argument and message array. This speeds up the + // allocation of those list a lot when they have a lot of arguments + cursor := ast + var n int + for cursor.Next != nil { + cursor = cursor.Next + n++ + } + l := len(strs) + strList := make([]string, n+l) + copy(strList, strs) + msgList := make([]string, n) + + var i int for ast.Next != nil { ast = ast.Next var str string @@ -218,16 +233,18 @@ func (b *Builder) dispatch(stepN int, ast *parser.Node) error { if _, ok := replaceEnvAllowed[cmd]; ok { str = b.replaceEnv(ast.Value) } - strs = append(strs, str) - msg += " " + ast.Value + strList[i+l] = str + msgList[i] = ast.Value + i++ } + msg += " " + strings.Join(msgList, " ") fmt.Fprintln(b.OutStream, msg) // XXX yes, we skip any cmds that are not valid; the parser should have // picked these out already. if f, ok := evaluateTable[cmd]; ok { - return f(b, strs, attrs, original) + return f(b, strList, attrs, original) } fmt.Fprintf(b.ErrStream, "# Skipping unknown instruction %s\n", strings.ToUpper(cmd)) From a2a50aa35ab58f52b68a65acfe668c26b765608f Mon Sep 17 00:00:00 2001 From: "Daniel, Dao Quang Minh" Date: Tue, 11 Nov 2014 15:15:00 -0500 Subject: [PATCH 2/5] use cached images instead of fetching the same image again Docker-DCO-1.1-Signed-off-by: Daniel, Dao Quang Minh (github: dqminh) --- daemon/daemon.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/daemon/daemon.go b/daemon/daemon.go index b0feae917b..39e816c52b 100644 --- a/daemon/daemon.go +++ b/daemon/daemon.go @@ -1090,9 +1090,9 @@ func (daemon *Daemon) ImageGetCached(imgID string, config *runconfig.Config) (*i // Loop on the children of the given image and check the config var match *image.Image for elem := range imageMap[imgID] { - img, err := daemon.Graph().Get(elem) - if err != nil { - return nil, err + img, ok := images[elem] + if !ok { + return nil, fmt.Errorf("unable to find image %q", elem) } if runconfig.Compare(&img.ContainerConfig, config) { if match == nil || match.Created.Before(img.Created) { From 1e7ba09b60aa0bc527dc7e0159071a6d47796de4 Mon Sep 17 00:00:00 2001 From: "Daniel, Dao Quang Minh" Date: Wed, 12 Nov 2014 09:13:47 -0500 Subject: [PATCH 3/5] add a test case for EXPOSE ports order changing order of EXPOSE ports should not invalidate the cache as the content doesnt change Docker-DCO-1.1-Signed-off-by: Daniel, Dao Quang Minh (github: dqminh) --- integration-cli/docker_cli_build_test.go | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/integration-cli/docker_cli_build_test.go b/integration-cli/docker_cli_build_test.go index de60a8017f..1f23c85162 100644 --- a/integration-cli/docker_cli_build_test.go +++ b/integration-cli/docker_cli_build_test.go @@ -1732,6 +1732,29 @@ func TestBuildExpose(t *testing.T) { logDone("build - expose") } +func TestBuildExposeOrder(t *testing.T) { + buildID := func(name, exposed string) string { + _, err := buildImage(name, fmt.Sprintf(`FROM scratch + EXPOSE %s`, exposed), true) + if err != nil { + t.Fatal(err) + } + id, err := inspectField(name, "Id") + if err != nil { + t.Fatal(err) + } + return id + } + + id1 := buildID("testbuildexpose1", "80 2375") + id2 := buildID("testbuildexpose2", "2375 80") + defer deleteImages("testbuildexpose1", "testbuildexpose2") + if id1 != id2 { + t.Errorf("EXPOSE should invalidate the cache only when ports actually changed") + } + logDone("build - expose order") +} + func TestBuildEmptyEntrypointInheritance(t *testing.T) { name := "testbuildentrypointinheritance" name2 := "testbuildentrypointinheritance2" From 87d0562c61b80aba05f4c3b4f49f5527afb55989 Mon Sep 17 00:00:00 2001 From: "Daniel, Dao Quang Minh" Date: Wed, 12 Nov 2014 03:22:08 -0500 Subject: [PATCH 4/5] expose sorts its ports before saving as comment Saving ports as `map[nat.Port]struct{}` directly has ordering issue which is more replicatable where we expose a huge number of ports at the same time. As a result, the cache will be burst whenever the map order is different from the previous build. This sorts the ports first and save them as a whitespace-separated list instead of the map representation, so the order will always be consistent if the port list isnt changed. NOTICE: this will burst the old expose caches Docker-DCO-1.1-Signed-off-by: Daniel, Dao Quang Minh (github: dqminh) --- builder/dispatchers.go | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/builder/dispatchers.go b/builder/dispatchers.go index f2fdd35955..b138fe3031 100644 --- a/builder/dispatchers.go +++ b/builder/dispatchers.go @@ -12,6 +12,7 @@ import ( "io/ioutil" "path/filepath" "regexp" + "sort" "strings" log "github.com/Sirupsen/logrus" @@ -302,14 +303,21 @@ func expose(b *Builder, args []string, attributes map[string]bool, original stri return err } + // instead of using ports directly, we build a list of ports and sort it so + // the order is consistent. This prevents cache burst where map ordering + // changes between builds + portList := make([]string, len(ports)) + var i int for port := range ports { if _, exists := b.Config.ExposedPorts[port]; !exists { b.Config.ExposedPorts[port] = struct{}{} } + portList[i] = string(port) + i++ } + sort.Strings(portList) b.Config.PortSpecs = nil - - return b.commit("", b.Config.Cmd, fmt.Sprintf("EXPOSE %v", ports)) + return b.commit("", b.Config.Cmd, fmt.Sprintf("EXPOSE %s", strings.Join(portList, " "))) } // USER foo From 29be7b439ec4d0c8a54852ccbbe7b6bcf040e13f Mon Sep 17 00:00:00 2001 From: "Daniel, Dao Quang Minh" Date: Wed, 12 Nov 2014 22:14:15 -0500 Subject: [PATCH 5/5] add test for exposing large number of ports this test checks if exposing a large number of ports in Dockerfile properly saves the port in configs. We dont actually expose a VERY large number of ports here because the result is the same and it increases the test time by a few seconds Docker-DCO-1.1-Signed-off-by: Daniel, Dao Quang Minh (github: dqminh) --- integration-cli/docker_cli_build_test.go | 59 ++++++++++++++++++++++++ 1 file changed, 59 insertions(+) diff --git a/integration-cli/docker_cli_build_test.go b/integration-cli/docker_cli_build_test.go index 1f23c85162..0331691940 100644 --- a/integration-cli/docker_cli_build_test.go +++ b/integration-cli/docker_cli_build_test.go @@ -2,6 +2,7 @@ package main import ( "archive/tar" + "bytes" "encoding/json" "fmt" "io/ioutil" @@ -10,9 +11,11 @@ import ( "path" "path/filepath" "regexp" + "strconv" "strings" "syscall" "testing" + "text/template" "time" "github.com/docker/docker/pkg/archive" @@ -1732,6 +1735,62 @@ func TestBuildExpose(t *testing.T) { logDone("build - expose") } +func TestBuildExposeMorePorts(t *testing.T) { + // start building docker file with a large number of ports + portList := make([]string, 50) + line := make([]string, 100) + expectedPorts := make([]int, len(portList)*len(line)) + for i := 0; i < len(portList); i++ { + for j := 0; j < len(line); j++ { + p := i*len(line) + j + 1 + line[j] = strconv.Itoa(p) + expectedPorts[p-1] = p + } + if i == len(portList)-1 { + portList[i] = strings.Join(line, " ") + } else { + portList[i] = strings.Join(line, " ") + ` \` + } + } + + dockerfile := `FROM scratch + EXPOSE {{range .}} {{.}} + {{end}}` + tmpl := template.Must(template.New("dockerfile").Parse(dockerfile)) + buf := bytes.NewBuffer(nil) + tmpl.Execute(buf, portList) + + name := "testbuildexpose" + defer deleteImages(name) + _, err := buildImage(name, buf.String(), true) + if err != nil { + t.Fatal(err) + } + + // check if all the ports are saved inside Config.ExposedPorts + res, err := inspectFieldJSON(name, "Config.ExposedPorts") + if err != nil { + t.Fatal(err) + } + var exposedPorts map[string]interface{} + if err := json.Unmarshal([]byte(res), &exposedPorts); err != nil { + t.Fatal(err) + } + + for _, p := range expectedPorts { + ep := fmt.Sprintf("%d/tcp", p) + if _, ok := exposedPorts[ep]; !ok { + t.Errorf("Port(%s) is not exposed", ep) + } else { + delete(exposedPorts, ep) + } + } + if len(exposedPorts) != 0 { + t.Errorf("Unexpected extra exposed ports %v", exposedPorts) + } + logDone("build - expose large number of ports") +} + func TestBuildExposeOrder(t *testing.T) { buildID := func(name, exposed string) string { _, err := buildImage(name, fmt.Sprintf(`FROM scratch