Re: [PATCH v4 01/12] pkt-line: extract set_packet_header()

[Jeff King <peff@peff.net>]

On Wed, Aug 03, 2016 at 01:18:55PM -0700, Junio C Hamano wrote:

> larsxschneider@gmail.com writes:
> 
> > From: Lars Schneider <larsxschneider@gmail.com>
> >
> > set_packet_header() converts an integer to a 4 byte hex string. Make
> > this function locally available so that other pkt-line functions can
> > use it.
> 
> Didn't I say that this is a bad idea already in an earlier review?
> 
> The only reason why you want it, together with direct_packet_write()
> (which I think is another bad idea), is because you use
> packet_buf_write() to create a "<header><payload>" in a buf in the
> usercode in step 11/12 like this:
> 
> +	packet_buf_write(&nbuf, "command=%s\n", filter_type);
> +	ret = !direct_packet_write(process->in, nbuf.buf, nbuf.len, 1);
> 
> which would be totally unnecessary if you just did strbuf_addf()
> into nbuf and used packet_write() like everybody else does.
> 
> Puzzled.  Why are steps 01/12 and 02/12 an improvement?

I think it is an attempt to avoid the extra memcpy() of the bytes into
another packet buffer.

I notice that the solution does still end up a using a double-write() in
some cases, though.  I was curious if this made any difference, though,
so I wrote a short test program:

-- >8 --
#include <unistd.h>
#include <string.h>

int main(int argc, char **argv)
{
        int type;

        if (argv[1] && !strcmp(argv[1], "prepend"))
                type = 0; /* size prepended to buffer */
        else if (argv[1] && !strcmp(argv[1], "write"))
                type = 1;
        else if (argv[1] && !strcmp(argv[1], "memcpy"))
                type = 2;
        else
                return 1;

        while (1) {
                char buf[65520];
                int r = read(0, buf + 4, sizeof(buf));
                if (r <= 0)
                        break;
                if (!type) {
                        memcpy(buf, "1234", 4);
                        write(1, buf, r + 4);
                } else if (type == 1) {
                        write(1, "1234", 4);
                        write(1, buf + 4, r);
                } else if (type == 2) {
                        char packet[sizeof(buf) + 4];
                        memcpy(packet, "1234", 4);
                        memcpy(packet + 4, buf + 4, r);
                        write(1, packet, r + 4);
                }
        }
        return 0;
}
-- >8 --

We'd expect "prepend" to be the fastest, as it does a single write and
zero-copy. And then it is a question of whether the double-write is
worse than the extra memcpy.

On Linux, feeding 100MB of zeroes into stdin, I got (best-of-five):

  - prepend: 11ms
  - write: 11ms
  - memcpy: 15ms

So it _does_ make a difference to avoid the memcpy, though 4ms per 100MB
does not seem like it is probably worth caring about. The double-write
also gets worse if you use a smaller buffer size (e.g., if you drop to
4K, that adds back in about 4ms of overhead because you're calling
write() a lot more times).

The cost of write() may vary on other platforms, but the cost of memcpy
generally shouldn't. So I'm inclined to say that it is not really worth
micro-optimizing the interface.

I think the other issue is that format_packet() only lets you send
string data via "%s", so it cannot be used for arbitrary data that may
contain NULs. So we do need _some_ other interface to let you send a raw
data packet, and it's going to look similar to the direct_packet_write()
thing.

The alternative is to hand-code it, which is what send_sideband() does
(it uses xsnprintf("%04x") to do the hex formatting, though).

-Peff