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

From: Jeff King <peff@peff.net>
Date: Aug 3, 2016, 21:12 UTC
Message-ID: <20160803211221.t2zdhvwjum2baeqs@sigill.intra.peff.net>
In-Reply-To: <xmqqd1lp8v2o.fsf@gitster.mtv.corp.google.com>

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

Show 22 quoted lines

> 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