Expose a large number of ports should not slow down builder #9122
Conversation
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 <dqminh89@gmail.com> (github: dqminh)
Docker-DCO-1.1-Signed-off-by: Daniel, Dao Quang Minh <dqminh89@gmail.com> (github: dqminh)
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 <dqminh89@gmail.com> (github: dqminh)
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 <dqminh89@gmail.com> (github: dqminh)
|
looks good, but I would like to see https://gist.github.com/karalabe/451f01479f8848973331#file-dockerfile in test form somehow instead of what you currently have. I imagine making parser tests won't work, so maybe you could make an integration-cli test that allocates the range of 1-65535 and then ensures that's in the expose list? |
|
Oh. Don't actually include the file. Generate it, please. :) |
|
One last thing: drone is not passing currently, but I don't believe that's your fault. It'll have to pass, though, before we can merge it. |
| // 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)) |
There was a problem hiding this comment.
Just idea: probably portList := make([]string, 0, len(ports)) and append allow you to drop i var and have same performance, but I'm not sure.
There was a problem hiding this comment.
i think that's faster than the old way, but allocating a fixed length slice is still the fastest and append has some extra overhead. I wrote a sample program to benchmark here http://play.golang.org/p/dbG3JseujF
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 <dqminh89@gmail.com> (github: dqminh)
@erikh i added a test but only expose 1-5000 ports. I guess that should be enough since exposing more ports will take more time but probably will not buy us anything more. |
|
SGTM
|
|
Collective LGTM! |
Expose a large number of ports should not slow down builder
|
LGTM |
Related to #9021
This improves the behavior of builder when handling a (large) list of ports by preallocating some data when possible. It improves the build speed of https://gist.github.com/karalabe/451f01479f8848973331#file-dockerfile from 20+ seconds to a few seconds on my machine
EXPOSEorder now will also be more resistant to change as we sort the ports before saving them as image's config.cc @erikh