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

[Junio C Hamano]

On Fri, Aug 5, 2016 at 10:31 AM, Lars Schneider
 wrote:
>
>> On 04 Aug 2016, at 18:14, Junio C Hamano  wrote:
>>
>> signature would look more like write(2) and deserve to be called
>> packet_write() but unfortunately the name is taken by what should
>> have called packet_fmt() or something, but that squats on a good
>> name packet_write().  Sigh.
>
> "Sigh" means, a series preparation patch that renames "packet_write()"
> to "paket_write_fmt()" would not be a good idea? It is used 59 times
> currently...

It would be a good idea in the longer term, I would think. I just wasn't
sure if you are willing to volunteer, and in-flight topics will tolerate, such
a change right now. I have a feeling that all the current callsites are
fairly stable and no in-flight topic touches them, so if you feel like doing
so, please go ahead ;-)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Lars Schneider]

> On 04 Aug 2016, at 18:14, Junio C Hamano  wrote:
> 
> Jeff King  writes:
> 
>> 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.
> 
> OK.  That is a much better argument than "I already stuff the length
> bytes in my buffer" (which will invite "How about stop doing that?")
> to justify a new "I have N bytes of data, send it out", whose
> signature would look more like write(2) and deserve to be called
> packet_write() but unfortunately the name is taken by what should
> have called packet_fmt() or something, but that squats on a good
> name packet_write().  Sigh.

"Sigh" means, a series preparation patch that renames "packet_write()" 
to "paket_write_fmt()" would not be a good idea? It is used 59 times 
currently...

- Lars

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Junio C Hamano]

Lars Schneider  writes:

> However, besides the bogus performance argument I introduced that function
> to allow packet writs to fail using the `gentle` parameter:
> http://public-inbox.org/git/D116610C-F33A-43DA-A49D-0B33958822E5%40gmail.com/
>
> Would you be OK if I introduce packet_write_gently() that returns `0` if the
> write was OK and `-1` if it failed?

Yes, I agree with you that it would be a good thing to have a
_gently() variant that lets the caller deal with possible error
conditions itself instead of dying.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Lars Schneider]

> On 04 Aug 2016, at 18:14, Junio C Hamano  wrote:
> 
> Jeff King  writes:
> 
>> 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.
> 
> OK.  That is a much better argument than "I already stuff the length
> bytes in my buffer" (which will invite "How about stop doing that?")
> to justify a new "I have N bytes of data, send it out", whose
> signature would look more like write(2) and deserve to be called
> packet_write() but unfortunately the name is taken by what should
> have called packet_fmt() or something, but that squats on a good
> name packet_write().  Sigh.

Well, my argument wasn't meant to be offensive. It was just an idea that
I published the to get feedback. Now I understand that it wasn't a particular
good idea (thanks Peff for the performance test!).

However, besides the bogus performance argument I introduced that function
to allow packet writs to fail using the `gentle` parameter:
http://public-inbox.org/git/D116610C-F33A-43DA-A49D-0B33958822E5%40gmail.com/

Would you be OK if I introduce packet_write_gently() that returns `0` if the
write was OK and `-1` if it failed?

Thanks,
Lars
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Junio C Hamano]

Jeff King  writes:

> 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.

OK.  That is a much better argument than "I already stuff the length
bytes in my buffer" (which will invite "How about stop doing that?")
to justify a new "I have N bytes of data, send it out", whose
signature would look more like write(2) and deserve to be called
packet_write() but unfortunately the name is taken by what should
have called packet_fmt() or something, but that squats on a good
name packet_write().  Sigh.





--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Lars Schneider]

> On 03 Aug 2016, at 22:18, Junio C Hamano  wrote:
> 
> larsxschnei...@gmail.com writes:
> 
>> From: Lars Schneider 
>> 
>> 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?

Yes, but in that earlier version I made this function *publicly*
available. In this patch the function is only available and used
within pkt-line.c.


> 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 "" in a buf in the
> usercode in step 11/12 like this:
> 
> + packet_buf_write(, "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.

The usercode in step 11/12 could use packet_buf_write(). I am not
worried about performance here. What I am worried about is that
packet_buf_write() dies on error. Since direct_packet_write()
has a "gentle" parameter in can handle these cases. This is important
because a filter might be configured as "required=false" and then
errors are OK.

Would you prefer to see a packet_buf_write_gently() instead?

Thanks,
Lars
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Jeff King]

On Wed, Aug 03, 2016 at 05:12:21PM -0400, Jeff King wrote:

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

After seeing that, I wondered why we need set_packet_header() at all.
But we do for the case when we are filling in the size at the start of a
buffer, because xsnprintf() will write an extra NUL that we do not care
about. send_sideband() is happy to then overwrite it with data, but
code (like format_packet) that computes the buffer, then fills in the
size, must avoid overwriting the first byte of the buffer.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Jeff King]

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

> larsxschnei...@gmail.com writes:
> 
> > From: Lars Schneider 
> >
> > 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 "" in a buf in the
> usercode in step 11/12 like this:
> 
> + packet_buf_write(, "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 
#include 

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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Junio C Hamano]

larsxschnei...@gmail.com writes:

> From: Lars Schneider 
>
> 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 "" in a buf in the
usercode in step 11/12 like this:

+   packet_buf_write(, "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?

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[larsxschneider]

From: Lars Schneider 

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.

Signed-off-by: Lars Schneider 
---
 pkt-line.c | 18 --
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/pkt-line.c b/pkt-line.c
index 62fdb37..177dc73 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -97,10 +97,19 @@ void packet_buf_flush(struct strbuf *buf)
strbuf_add(buf, "", 4);
 }
 
-#define hex(a) (hexchar[(a) & 15])
-static void format_packet(struct strbuf *out, const char *fmt, va_list args)
+static void set_packet_header(char *buf, const int size)
 {
static char hexchar[] = "0123456789abcdef";
+   #define hex(a) (hexchar[(a) & 15])
+   buf[0] = hex(size >> 12);
+   buf[1] = hex(size >> 8);
+   buf[2] = hex(size >> 4);
+   buf[3] = hex(size);
+   #undef hex
+}
+
+static void format_packet(struct strbuf *out, const char *fmt, va_list args)
+{
size_t orig_len, n;
 
orig_len = out->len;
@@ -111,10 +120,7 @@ static void format_packet(struct strbuf *out, const char 
*fmt, va_list args)
if (n > LARGE_PACKET_MAX)
die("protocol error: impossibly long line");
 
-   out->buf[orig_len + 0] = hex(n >> 12);
-   out->buf[orig_len + 1] = hex(n >> 8);
-   out->buf[orig_len + 2] = hex(n >> 4);
-   out->buf[orig_len + 3] = hex(n);
+   set_packet_header(>buf[orig_len], n);
packet_trace(out->buf + orig_len + 4, n - 4, 1);
 }
 
-- 
2.9.0

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html