buffer layer to disk before registering in graph #7704
Conversation
| func (r *tempFileReader) Read(data []byte) (int, error) { | ||
| n, err := r.File.Read(data) | ||
| if err != nil { | ||
| r.File.Close() |
There was a problem hiding this comment.
I kinda prefer the explicit one.
There was a problem hiding this comment.
Okay, if you like it. Just thought that you missed this stuff :)
|
@tianon This is just for testing right now. |
|
@unclejack thanks, but it's no quicker - in fact it's the same speed as it was originally https://gist.github.com/ross-w/407d7cbe3c16c592fbec The "downloading layer to temp file" (or whatever it is, it's almost too quick for me to read) step is very quick, but the loading layer step takes all the time. |
|
@unclejack fair enough ❤️ |
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
There was a problem hiding this comment.
I think here we need logic to remove tmp on error. Maybe like named returning err and
defer func() {
if err != nil {
os.RemoveAll(tmp)
}
}()
|
I've did little test and it is slightly faster for me with my 54 Mb wifi and 5400 HDD on small images. |
|
@unclejack For small images I experience an almost-negligible performance improvement (a few seconds), and it's hard for me to make sure it's not too random (I did multiple tries though). So maybe I'm testing it wrong, but from what I can see, I would not want to merge this PR. |
|
@tiborvass this is more or less my experience also, I can't help but feel that this isn't the bottleneck we are experiencing. Though it was very interesting to see how quick the download is compared to all the other stuff, so that definitely points the finger at the non-download parts being slow. |
|
@tiborvass Please try with a local registry. There are some bottlenecks outside of the download and they're going to be fixed by #7542. These two PRs should make pull faster. |
e59d7f5 to
82e7298
Compare
Docker-DCO-1.1-Signed-off-by: Cristian Staretu <cristian.staretu@gmail.com> (github: unclejack)
82e7298 to
ad23448
Compare
|
@LK4D4 Can you take a look at this PR, please? |
|
@unclejack So, we want merge this? |
|
@LK4D4 We need to consider merging this after proper review. |
|
Tests passing, code LGTM |
|
So this could segregate failures in pulling the images, from the registering of the image (like a race in a failed image being cleaned up before it is usable). I'm going to try again with a local registry, but just did a few pulls from the hub, and did not ever get better time with this PR than without. Doing a pull of the centos:latest image, I did 1 untimed pull, to ensure the CDN didn't interfere with the result, the: The times are not so greatly different, but still. Also, another benefit of having this temp buffering could be one less things to worry about when (if) we reorganize graph for checksum based layer paths. |
|
similar when pulling from a local registry Where all 24s are with this PR, and 19 is from 1.2.0-dev // ed7fb3b |
|
I'm going to close this now. I can open this PR again if we don't change the code in the meantime. |
This WIP PR makes Docker buffer layers to disk before registering them. This helps speed up the layer download.
Please note that this PR doesn't attempt to do anything about the performance of layer loading. That might still be slow. Registering layers in the graph can still take a long time if the system's performance and disk IO isn't that good.
If this proves to improve performance for some, a flag to bypass the buffering to disk should be added.
The code of the TempFileReader can probably be moved elsewhere.
helps with #7291 by improving pull