copy: avoid write() when no progress bar needed

[runcom]

Observed some spikes of the progress bar while writing to /dev/null in CRI-O (since it has no progress bar).
This PR is a micro-optimization and will avoid a write() syscall when no progress bar is needed (and to keep the profiler to not show me that writing to /dev/null wastes some micro seconds).

@mrunalp @rhatdan @sameo PTAL this is needed by CRI-O post V1

Signed-off-by: Antonio Murdaca runcom@redhat.com

[sameo]

LGTM

[TomSweeneyRedHat]

The changes look OK to me, but TRAVIS CI isn't happy with them.

[mtrmac]

@runcom Could you explain this in more detail? What “spikes” in what progress bar (if it goes to ioutil.Discard, there is no progress bar AFAICT)

What write() syscall? Looking at https://golang.org/src/io/ioutil/ioutil.go?s=3442:3483#L105 devNull.write(), no syscall happens, so the syscall behavior should be exactly the same. The /dev/null path is not used at all AFAICT.

The only thing this PR actually does is avoid calling fmt.Fprintf and computing the output string.

Or is the important part in avoiding the pb.New progress bar? But even that seems to only use the provided io.Writer.

[runcom]

The only thing this PR actually does is avoid calling fmt.Fprintf and computing the output string.

Or is the important part in avoiding the pb.New progress bar? But even that seems to only use the provided io.Writer.

well yeah, I observed that when Discard is used, all the pb stuff were actually called and re-called. This PR basically avoids that as the go profiler showed that as being a busy path.

[mtrmac]

Per IRC conversation – the goal seems to be primarily to get rid of the progress bar. I’d be happier with actually finding out what is so expensive about the progress bar, so that users who do use it benefit as well.

But, *shrug*, one extra if in there doesn’t hurt that much. I am a bit concerned about keeping the writer members easy enough to understand.

[mtrmac]

It’s not immediately clear what is the difference between writer/reportWriter here. The naming is a bit confusing, as is hasReportWriter for a reportWriter which always exists

And if this is moving all users EDIT to call the function, and the io.Writer is used only for initializing the progress bar, we don’t really need hasReportWriter; we could just have an io.Writer which is either an implementation or nil.

[mtrmac]

See also 87283c0 (“Add copier.Printf” from #346). Not directly applicable, but using a real method, and using a name which is recognized by go tool vet would be nice.

[mtrmac]

If making the progress bar conditional is that important for performance, this needs a comment explaining why it is conditional; otherwise in a few months someone will notice that with an ioutil.Discard writer implementation the conditional is redundant and remove it again.