Add ability to cache/push compressed layers

[choo-stripe]

Hey!

I did a bunch of profiling on the library and made some optimizations. For the image I was experimenting with, push timing went from 2mins, 25 seconds to 49 seconds.

I know I've done some bad things here, but would love to work together to get some of these savings merged in some form or another.

I'll leave some comments throughout to share some thoughts so we can move forward.

[googlebot]

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it!) and we'll verify it.

---

What to do if you already signed the CLA

Individual signers

• It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

• Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
• The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
• The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

[choo-stripe]

We don't pass options into manifest generation, so we do some hackery here. Calling i.Layers() is slow, so this saved 10s of seconds.

[jonjohnsonjr]

Why is Layers slow?

[choo-stripe]

I imagine it was calculating digests for the first time (since we called CompressedLayers above but I'm not 100% sure.

[choo-stripe]

Parallelizing here saved >1 minute.

[jonjohnsonjr]

I think we might actually want to move the parallelization into here?

go-containerregistry/pkg/v1/mutate/mutate.go

Lines 153 to 190 in b02d448

	for _, add := range i.adds {
	diffID, err := add.Layer.DiffID()
	if err != nil {
	return err
	}
	diffIDs = append(diffIDs, diffID)
	history = append(history, add.History)
	diffIDMap[diffID] = add.Layer
	}
	m, err := i.base.Manifest()
	if err != nil {
	return err
	}
	manifest := m.DeepCopy()
	manifestLayers := manifest.Layers
	for _, add := range i.adds {
	d := v1.Descriptor{}
	var err error
	if d.Size, err = add.Layer.Size(); err != nil {
	return err
	}
	if d.Digest, err = add.Layer.Digest(); err != nil {
	return err
	}
	if d.MediaType, err = add.Layer.MediaType(); err != nil {
	return err
	}
	d.Annotations = add.Annotations
	d.URLs = add.URLs
	manifestLayers = append(manifestLayers, d)
	digestMap[d.Digest] = add.Layer
	}

[choo-stripe]

Do uncompressed images back on to mutate? I haven't traced it deep enough into the stack.

[choo-stripe]

I was thinking about users passing in a io.ReadWriter to allocate their own storage or something. Any feedback welcome.

[jonjohnsonjr]

I'm not 100% sure what's going on here.

[choo-stripe]

This reads the compressed layer into RAM - which comes with tradeoffs. Could run out of RAM, but also get super fast layer contents access.

[choo-stripe]

Writing multiple tags seemed to be expensive (digesting, compressing, whatever). Adding the ability to just push refs cut about 20 seconds at the end.

[jonjohnsonjr]

I think we can partially solve some of this by just implementing an explicit tagging operation, see: #349

[choo-stripe]

Yep!

[choo-stripe]

deduping is important for rate limits and for the network calls for checking if blobs exist. A lot of layers can really have an impact here - especially when pushing a bunch of tags to one repo.

[jonjohnsonjr]

Deduping per registry is interesting... I hadn't thought of that. I think this is mostly addressed by #349, but I can imagine deduping blobs across multiple images to avoid extraneous existence checks (but that's not covered here).

[choo-stripe]

yep!

[jonjohnsonjr]

Why is this better than just calling Compressed on each layer that gets returned? It's unclear to me why this is needed.

[choo-stripe]

Through profiling today, I found out that Layers() was called multiple times and returned new layers. So the speedup was because CompressedLayers cached the compressed layers, while Layers did not.

[jonjohnsonjr]

Why is Layers slow?

[jonjohnsonjr]

I think we might actually want to move the parallelization into here?

go-containerregistry/pkg/v1/mutate/mutate.go

Lines 153 to 190 in b02d448

	for _, add := range i.adds {
	diffID, err := add.Layer.DiffID()
	if err != nil {
	return err
	}
	diffIDs = append(diffIDs, diffID)
	history = append(history, add.History)
	diffIDMap[diffID] = add.Layer
	}
	m, err := i.base.Manifest()
	if err != nil {
	return err
	}
	manifest := m.DeepCopy()
	manifestLayers := manifest.Layers
	for _, add := range i.adds {
	d := v1.Descriptor{}
	var err error
	if d.Size, err = add.Layer.Size(); err != nil {
	return err
	}
	if d.Digest, err = add.Layer.Digest(); err != nil {
	return err
	}
	if d.MediaType, err = add.Layer.MediaType(); err != nil {
	return err
	}
	d.Annotations = add.Annotations
	d.URLs = add.URLs
	manifestLayers = append(manifestLayers, d)
	digestMap[d.Digest] = add.Layer
	}

[jonjohnsonjr]

I'm not 100% sure what's going on here.

[jonjohnsonjr]

I think we can partially solve some of this by just implementing an explicit tagging operation, see: #349

[jonjohnsonjr]

Deduping per registry is interesting... I hadn't thought of that. I think this is mostly addressed by #349, but I can imagine deduping blobs across multiple images to avoid extraneous existence checks (but that's not covered here).

[jonjohnsonjr]

Hey!

Hello! 👋

I did a bunch of profiling on the library and made some optimizations. For the image I was experimenting with, push timing went from 2mins, 25 seconds to 49 seconds.

This is awesome, thanks for taking the time to experiment with this 😄

I know I've done some bad things here, but would love to work together to get some of these savings merged in some form or another.

I'll leave some comments throughout to share some thoughts so we can move forward.

Absolutely. I'm really interested in how you're using this package -- we might be able to optimize things further, or I could point you in a better direction. Could you put together a small, representative program that demonstrates some of the slowness you're seeing? I'd love to be able to have a reproducible test to benchmark any changes against. We have a small registry package for testing that implements most of the registry protocol, if that helps.

[jonjohnsonjr]

There's also https://godoc.org/github.com/google/go-containerregistry/pkg/v1/cache which might be useful for caching gzipped things.

[choo-stripe]

@jonjohnsonjr given your questions and ideas, I tried to come up with a less invasive way to speed things up (how to make the existing implementation fast instead of replacing it entirely). Given that, I did a bunch of profiling and clearly wrote out my findings for each commit. For the test harness that I wrote here, we went from ~1 minute to 20 seconds - which matches my old implementation.

I hope that by reading commit messages from oldest to newest, you'll be able to see at each stage what was slow and how the optimization helped.

I think this PR is now in a much better place.

Remaining issues that concern me:

1. We now push the config layer compressed instead of uncompressed. We need to verify that this doesn't break any assumptions (i.e. making sure the manifest doesn't use the uncompressed sha - I don't think it should)
2. We may not want to pull layers into ram before uploading. That Pull layers into RAM before sending them sped things up quite a bit for our specific test harness here, but I'm not convinced it would materially impact perf with a more robust/parallellizable registry. I image we want to revert d16f7f9.

[codecov-io]

Codecov Report

Merging #517 into master will decrease coverage by 0.04%.
The diff coverage is 57.14%.

@@            Coverage Diff             @@
##           master     #517      +/-   ##
==========================================
- Coverage   73.11%   73.07%   -0.05%     
==========================================
  Files          92       92              
  Lines        3917     3933      +16     
==========================================
+ Hits         2864     2874      +10     
- Misses        692      696       +4     
- Partials      361      363       +2

Impacted Files	Coverage Δ
pkg/v1/partial/uncompressed.go	56.25% <33.33%> (-1.53%)	⬇️
pkg/v1/remote/write.go	64.38% <84.61%> (+0.58%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 03a1f32...e4e5b52. Read the comment docs.

[jonjohnsonjr]

For that test image, you could drop the Dockerfile and just use this:

img, err := random.Image(50*1024*1024, 10)

Though, then we are benchmarking whatever we save to a tarball instead of whatever docker saves to a tarball, which might not be representative of what you're seeing.

Instead of writing a test, you can add a go benchmark that we could check in: https://golang.org/pkg/testing/#hdr-Benchmarks

[jonjohnsonjr]

We don't actually want to embed the cache into the writer -- callers should wrap the image they are going to write with a cache.Image. This isn't very discoverable, so I opened a PR to add an example to the godoc: #522

[choo-stripe]

Re using random.Image:
This is a great suggestion to clean up the tests. I still think it's valuable, but it's not representative of the perf issues I was seeing. I was seeing perf issues due to disk io. Caching the layer computation was only exposed because disk io was slow.

Re cache.Image:
This still combines risk read/compression/network into one unified op, which, from benchmarking, some registries don't deal with super well.

1. How do you feel about the updated PR?
2. How would you feel about a separate PR with an in-memory cache that could be pre-populated/pre-compressed so compression and digestion become constant-time ops?

Btw, I really appreciate you taking the time to work through this with me.

[choo-stripe]

Ah! One more thing - with the image cache, with uncompressed images being read of disk, they get compressed twice:

1. Once for Digest(), which throws away the compressed contents(!)
2. Once when you need to read the Compressed() contents

Do you have ideas on how to combine those ops?

• We could put them in v1/partial/uncompressed.go but that could allocate large amounts of memory in there which doesn't seem fitting.
• We could put this in an in-memory cache, but that would break interface boundaries - we'd no longer be calling layer.Digest(), we'd be calculating it ourselves as we read Compressed() to load the layer into cache.

I don't see a clean way to do this, but it would save us a lot of time.

[choo-stripe]

I made some minor modifications to the cache and added an in-memory cache which gave ~the same perf gains because it also dedupes a lot of the compression/digesting/etc - choo-stripe/go-containerregistry@choo/simplifying...choo-stripe:choo/inmemcache

[jonjohnsonjr]

Btw, I really appreciate you taking the time to work through this with me.

Of course! Thanks for taking the time to do this!

Going to just explain a few things that might answer most of your questions, let me know if I missed anything:

In the happy case, we know the digest ahead of time, and our upload sequence should look like this:

That HEAD is cheap if we already know the digest; we just get a 404 and subsequently do the complete upload. If we get a 200, we can skip the entire upload, so that's a huge win.

In the sad case (what you're hitting, I believe), we have uncompressed blobs, and we don't know the digest ahead of time. I think we're still hitting the above strategy because we're eagerly calculating the digests when we read layers from the tarball. Ideally, we wouldn't do that at all! We would just skip the first HEAD request and hash the contents as we upload them:

We actually have an implementation for this, stream.Layer, which @imjasonh implemented, but it has some problems and we're not using it anywhere by default.

Sometimes the best thing to do is digest the content locally before uploading, so you can do an existence check first. E.g. if your internet is slow, it's better trade CPU time just in case the registry already has the blob. In most cases, though, it's better to just upload it.

How do you feel about the updated PR?

Your pre-calculation of digests runs afoul of this code in remote for handling stream.Layer, so I'd be reluctant to merge that bit.

It might be helpful to explain what needs to happen during an image upload. Here's a diagram of data flow through the system for an image with one layer, expressed via analogous tools:

(this is basically the implementation of stream.Layer)

We need to hash the uncompressed contents, compress them, hash the compressed contents, then upload them to the registry. The hash of the uncompressed contents is called the diffid, which ends up in a config file. The hash of the compressed contents is usually called the layer digest, which ends up in the image manifest. The hash of the config file also ends up in the manifest. It's a bit of a juggling act.

How I imagine this being fixed is that we cache the uncompressed contents as we read them, and we cache the compressed contents as we read them. We should also be memoizing the digest and diffid so that we avoid re-compressing or re-hashing anything:

E.g., when we call Digest() on a layer, it might have to read the layer from disk, gzip it, and sha256sum it, just to compute the digest. We throw away all that work, though! Ideally, that call to Digest() should write the gzipped layer to a cache (on disk or in memory) and memoize the sha256 value for both its uncompressed and compressed contents. Subsequent calls for Digest() and DiffID() would then just return the memoized values. Subsequent calls of Compressed() should read the compressed contents from the cache instead of re-zipping everything.

How would you feel about a separate PR with an in-memory cache that could be pre-populated/pre-compressed so compression and digestion become constant-time ops?

I'm somewhat surprised that you think your disk is the bottleneck here -- I would suspect it's actually gzip. If you do have a slow disk, though, being able to supply an in-memory cache would help a lot with that!

So here's what I'm thinking:

1. An in-memory cache implementation sounds great, I'd love that in a separate PR.
2. Let's figure out why we're re-compressing things even with a cache. I think this is because the tarball implementation is too eager. We shouldn't be calculating the digest until we need it.

For 2, I think we probably want to switch over to using stream.Layer for the tarball image, but it might just be that we need to make some operations lazy that are currently eager. I haven't looked at it recently, but I have a hunch that we could make it more amenable to composition with cache.Image than it is today.

I think some of your issues could have been solved by better documentation, which is mostly my fault 😄 if there's anything that could be clarified, doc issues and doc PRs are also super helpful.

Thanks again, @choo-stripe

[jmillikin-stripe]

(quick drive-by comment; I haven't deeply reviewed the code yet)

Sometimes the best thing to do is digest the content locally before uploading, so you can do an existence check first. E.g. if your internet is slow, it's better trade CPU time just in case the registry already has the blob. In most cases, though, it's better to just upload it.

In our case both compression and uploading are slow, and pre-existing layers are common. We'd ideally like to compress the layers and compute all relevant checksums at build time, so that the push process is a stream of HEADs, then fetches of missing layers from our datastore, then pushing those (already compressed) layers to the image registry.

I'm somewhat surprised that you think your disk is the bottleneck here -- I would suspect it's actually gzip. If you do have a slow disk, though, being able to supply an in-memory cache would help a lot with that!

It depends on the image, but in general we're nearly maxed out on both disk IO and CPU time. For gzip we use a parallel implementation (https://zlib.net/pigz/), which can saturate a 48-thread CPU.

We need to hash the uncompressed contents, compress them, hash the compressed contents, then upload them to the registry.

In previous discussions with the maintainer of the Python containerregistry code, we brought up the idea of a more optimized image format -- one that stored compressed layers directly, and stored all of the relevant checksums. Do you think it would be practical to implement support for such a format in this new Go implementation?

[jonjohnsonjr]

In previous discussions with the maintainer of the Python containerregistry code, we brought up the idea of a more optimized image format -- one that stored compressed layers directly, and stored all of the relevant checksums. Do you think it would be practical to implement support for such a format in this new Go implementation?

Two things worth mentioning:

1. When we write out tarballs, they are actually in the compressed form. We have the digests and the config, already, so we can derive the diffids from the config without any additional work. (I'm assuming we don't do anything stupid like re-hash things, but I have not confirmed this). When docker writes out tarballs, it uses an old format that has uncompressed tar files and doesn't record the digests. Some more info in Add repositories files to image tarballs #526
2. IMO, the right answer to this is using an OCI image layout. This stores things (manifest, blobs, config) on disk in the same format as it's stored in the registry, so there's not any conversion that has to happen. We have an implementation of this. It has all the same benefits as the "faaaaast" format in containerregistry (I think this is what you're referring to?), with the added bonus of being an actual standard thing instead of something @mattmoor pulled out of his... head. Docker doesn't support this natively, but there's an open issue if you want to stir up trouble.

I'm not sure how y'all are consuming this code internally. If you're using crane, we could trivially add support for OCI layout in pull/push to crane. Let me know if that would help. If you have your own tools that rely on this library, please try it out!

[mattmoor]

with the added bonus of being an actual standard thing instead of something @mattmoor pulled out of his... head

I absolutely did not pull it out of my head 🙄

[jonjohnsonjr]

I had a long conversation with @imjasonh about how we could fix this "for real" and it's a bit involved. I'll write something up when I get a chance (possibly this week but probably next week).