archive: use pigz|zstd if available

[giuseppe]

use the command line tools when available as they are much faster than the Go libraries.

Especially for gzip, I've registered a 50% improvement

use zstd if available

the performance improvement is not as clear as with pigz, but it is
still measurable difference.

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: giuseppe

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [giuseppe]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[rhatdan]

LGTM
Very cool
Would need to put in a Recommends for Podman and Buildah

[rhatdan]

@mtrmac @vrothberg @cgwalters @baude @nalind @alexlarsson @saschagrunert PTAL

[mtrmac]

Taking the reported speedup on faith, sure, why not.

Tests are unhappy, I didn’t check the details of the failure.

[mtrmac]

I don’t think this function should be in the business of managing pool; let the caller worry about that, probably using NewReadCloserWrapper like on all the other paths.

(Uh… is the existing code in the caller safe? Shouldn’t it trigger on close of the input to the decompression, rather than the output? It’s not obvious to me that they always happen in the expected order.

If it is not safe, that’s also a reason for the caller to worry about that … and at least a separate commit.)

[mtrmac]

• This is a data race vs the consumer goroutine
• I don’t immediately see that this filterReader is necessary: Isn’t it enough to do

var err = errors.New("internal error: should never be seen")
defer w.CloseWithError(err) // CloseWithErr(nil) == Close()
err = cmd.Run()

[giuseppe]

under what conditions will err ever be errors.New("internal error: should never be seen")?

[mtrmac]

Panic before Run returns.

[mtrmac]

… and in this case that should be an uncaught panic and terminate the whole process, so it doesn’t matter very much what err is (I guess? I’m not sure whether processing a panic is concurrent with the pipe consumer goroutine running).

It might actually matter if there were a recover somewhere on the call stack.

For me, this is now just a pattern, to try very hard to ensure that a pipe is always closed, so that the consumer will never be left hanging without a visible reason.

[mtrmac]

… and it’s a belt-and-suspenders thing: this makes sure to avoid a hypothetical CloseWithError(nil) on the panic path — whether or not panic is relevant, the point is that the claim “the pipe is successfully closed only on success” can be easily verified.

[cgwalters]

One side effect of using pigz is definitely that for consumers this will result in unreproducible gzip output compared to the previous Go gzip. And pigz in particular almost always for not-small input produces different output. Not fatal, but something to be aware of, and I suspect there are some consumers that are partially relying on the gzip "semi-reproducible" status quo, where the build context isn't caching the previous state via diffids.

[giuseppe]

One side effect of using pigz is definitely that for consumers this will result in unreproducible gzip output compared to the previous Go gzip. And pigz in particular almost always for not-small input produces different output. Not fatal, but something to be aware of, and I suspect there are some consumers that are partially relying on the gzip "semi-reproducible" status quo, where the build context isn't caching the previous state via diffids.

Thanks for the information, I didn't think of that problem and something to consider if we plan to use pigz also for the compressor. The current PR changes only the decompression side

[rhatdan]

I think the pull side is much more important then the push side. You usually push once and pull many times. Stick to only the uncompress.

[giuseppe]

ready for review, CI is green

[rhatdan]

LGTM

[mtrmac]

One side effect of using pigz is definitely that for consumers this will result in unreproducible gzip output compared to the previous Go gzip.

FWIW we never promised the compressed blobs to be reproducible (and we can’t, compression implementations make no such promises — and e.g. https://github.com/klauspost/compress is changing too often for me to think that the output never changes).

And yes, from time to time we see users who say “it’s been working for us fine for a year”, and it’s a struggle to convince them them that is it might very well break the next week and they need to change their design.

But it does break from time to time, and yes they do need to change their design; and preventing any single output change would not help avoid that need to change the design.

[mtrmac]

ACK to implementation.

[giuseppe]

I've added a new patch to the PR. I've realized the existing implementation was leaking the buffer on errors

[mtrmac]

/lgtm

(Purely personal aesthetic preference: differentiate the err and Err variables more. Also, named return values are a bit dangerous, so emphasizing that it is a return value might help. But that’s fully up to the maintainers of c/storage.)

[vrothberg]

@edsantiago, did you see performance improvements after vendoring this change into Podman CI?

Nice work, @giuseppe !

[edsantiago]

@vrothberg I'm not ignoring your question. (1) this is not yet vendored into podman, and (2) our PR merge rate is so low these days that it's hard to get performance numbers. I am keeping this on my TODO list and will report back once there's something to learn.