git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / Atom feed
* [PATCH] midx: use buffered I/O to talk to pack-objects
@ 2020-08-02 14:38 René Scharfe
  2020-08-02 16:11 ` Chris Torek
                   ` (2 more replies)
  0 siblings, 3 replies; 14+ messages in thread
From: René Scharfe @ 2020-08-02 14:38 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Junio C Hamano

Like f0bca72dc77 (send-pack: use buffered I/O to talk to pack-objects,
2016-06-08), significantly reduce the number of system calls and
simplify the code for sending object IDs to pack-objects by using
stdio's buffering and handling errors after the loop.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 midx.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/midx.c b/midx.c
index 6d1584ca51d..742638c3e51 100644
--- a/midx.c
+++ b/midx.c
@@ -1383,6 +1383,7 @@ int midx_repack(struct repository *r, const char *object_dir, size_t batch_size,
 	uint32_t i;
 	unsigned char *include_pack;
 	struct child_process cmd = CHILD_PROCESS_INIT;
+	FILE *cmd_in;
 	struct strbuf base_name = STRBUF_INIT;
 	struct multi_pack_index *m = load_multi_pack_index(object_dir, 1);

@@ -1435,6 +1436,8 @@ int midx_repack(struct repository *r, const char *object_dir, size_t batch_size,
 		goto cleanup;
 	}

+	cmd_in = xfdopen(cmd.in, "w");
+
 	for (i = 0; i < m->num_objects; i++) {
 		struct object_id oid;
 		uint32_t pack_int_id = nth_midxed_pack_int_id(m, i);
@@ -1443,10 +1446,15 @@ int midx_repack(struct repository *r, const char *object_dir, size_t batch_size,
 			continue;

 		nth_midxed_object_oid(&oid, m, i);
-		xwrite(cmd.in, oid_to_hex(&oid), the_hash_algo->hexsz);
-		xwrite(cmd.in, "\n", 1);
+		fprintf(cmd_in, "%s\n", oid_to_hex(&oid));
+	}
+
+	if (fclose(cmd_in)) {
+		error_errno(_("could not close stdin of pack-objects"));
+		result = 1;
+		finish_command(&cmd);
+		goto cleanup;
 	}
-	close(cmd.in);

 	if (finish_command(&cmd)) {
 		error(_("could not finish pack-objects"));
--
2.28.0

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] midx: use buffered I/O to talk to pack-objects
  2020-08-02 14:38 [PATCH] midx: use buffered I/O to talk to pack-objects René Scharfe
@ 2020-08-02 16:11 ` Chris Torek
  2020-08-03 18:10   ` Johannes Sixt
  2020-08-03 12:39 ` Derrick Stolee
  2020-08-12 16:52 ` [PATCH v2] " René Scharfe
  2 siblings, 1 reply; 14+ messages in thread
From: Chris Torek @ 2020-08-02 16:11 UTC (permalink / raw)
  To: René Scharfe; +Cc: Git Mailing List, Junio C Hamano

On Sun, Aug 2, 2020 at 7:40 AM René Scharfe <l.s.r@web.de> wrote:
> @@ -1443,10 +1446,15 @@ int midx_repack(struct repository *r, const char *object_dir, size_t batch_size,
>                         continue;
>
>                 nth_midxed_object_oid(&oid, m, i);
> -               xwrite(cmd.in, oid_to_hex(&oid), the_hash_algo->hexsz);
> -               xwrite(cmd.in, "\n", 1);
> +               fprintf(cmd_in, "%s\n", oid_to_hex(&oid));
> +       }
> +
> +       if (fclose(cmd_in)) {
> +               error_errno(_("could not close stdin of pack-objects"));
> +               result = 1;
> +               finish_command(&cmd);
> +               goto cleanup;
>         }
> -       close(cmd.in);
>
>         if (finish_command(&cmd)) {
>                 error(_("could not finish pack-objects"));
> --
> 2.28.0

Here, we don't have any explicit errno checking, but
of course error_errno() uses errno.  This too needs
an ferror() (or fflush()) test before the final fclose(),
and then we just need to use plain error().  Otherwise
you'll need the clumsier test-after-each-fprintf() and
an explicit final fflush()-and-test.

Chris

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] midx: use buffered I/O to talk to pack-objects
  2020-08-02 14:38 [PATCH] midx: use buffered I/O to talk to pack-objects René Scharfe
  2020-08-02 16:11 ` Chris Torek
@ 2020-08-03 12:39 ` Derrick Stolee
  2020-08-11 16:08   ` René Scharfe
  2020-08-12 16:52 ` [PATCH v2] " René Scharfe
  2 siblings, 1 reply; 14+ messages in thread
From: Derrick Stolee @ 2020-08-03 12:39 UTC (permalink / raw)
  To: René Scharfe, Git Mailing List; +Cc: Junio C Hamano, Chris Torek

On 8/2/2020 10:38 AM, René Scharfe wrote:
> Like f0bca72dc77 (send-pack: use buffered I/O to talk to pack-objects,
> 2016-06-08), significantly reduce the number of system calls and
> simplify the code for sending object IDs to pack-objects by using
> stdio's buffering and handling errors after the loop.

Good find. Thanks for doing this important cleanup.

Outside of Chris's other feedback, this looks like an obviously
correct transformation.

Thanks,
-Stolee

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] midx: use buffered I/O to talk to pack-objects
  2020-08-02 16:11 ` Chris Torek
@ 2020-08-03 18:10   ` Johannes Sixt
  2020-08-03 22:27     ` René Scharfe
  0 siblings, 1 reply; 14+ messages in thread
From: Johannes Sixt @ 2020-08-03 18:10 UTC (permalink / raw)
  To: Chris Torek, René Scharfe; +Cc: Git Mailing List, Junio C Hamano

Am 02.08.20 um 18:11 schrieb Chris Torek:
> On Sun, Aug 2, 2020 at 7:40 AM René Scharfe <l.s.r@web.de> wrote:
>> @@ -1443,10 +1446,15 @@ int midx_repack(struct repository *r, const char *object_dir, size_t batch_size,
>>                         continue;
>>
>>                 nth_midxed_object_oid(&oid, m, i);
>> -               xwrite(cmd.in, oid_to_hex(&oid), the_hash_algo->hexsz);
>> -               xwrite(cmd.in, "\n", 1);
>> +               fprintf(cmd_in, "%s\n", oid_to_hex(&oid));
>> +       }
>> +
>> +       if (fclose(cmd_in)) {
>> +               error_errno(_("could not close stdin of pack-objects"));
>> +               result = 1;
>> +               finish_command(&cmd);
>> +               goto cleanup;
>>         }
>> -       close(cmd.in);
>>
>>         if (finish_command(&cmd)) {
>>                 error(_("could not finish pack-objects"));
>> --
>> 2.28.0
> 
> Here, we don't have any explicit errno checking, but
> of course error_errno() uses errno.  This too needs
> an ferror() (or fflush()) test before the final fclose(),
> and then we just need to use plain error().  Otherwise
> you'll need the clumsier test-after-each-fprintf() and
> an explicit final fflush()-and-test.

We need this explicit test after each fprintf anyway because SIGPIPE may
be ignored, and then writing fails with EPIPE. On Windows, this is
doubly important because we do not have SIGPIPE at all (and always see
EPIPE), but we see EPIPE only on the first failed write; subsequent
writes produce EINVAL.

-- Hannes

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] midx: use buffered I/O to talk to pack-objects
  2020-08-03 18:10   ` Johannes Sixt
@ 2020-08-03 22:27     ` René Scharfe
  2020-08-04  4:31       ` René Scharfe
  0 siblings, 1 reply; 14+ messages in thread
From: René Scharfe @ 2020-08-03 22:27 UTC (permalink / raw)
  To: Johannes Sixt, Chris Torek; +Cc: Git Mailing List, Junio C Hamano

Am 03.08.20 um 20:10 schrieb Johannes Sixt:
> Am 02.08.20 um 18:11 schrieb Chris Torek:
>> On Sun, Aug 2, 2020 at 7:40 AM René Scharfe <l.s.r@web.de> wrote:
>>> @@ -1443,10 +1446,15 @@ int midx_repack(struct repository *r, const char *object_dir, size_t batch_size,
>>>                         continue;
>>>
>>>                 nth_midxed_object_oid(&oid, m, i);
>>> -               xwrite(cmd.in, oid_to_hex(&oid), the_hash_algo->hexsz);
>>> -               xwrite(cmd.in, "\n", 1);
>>> +               fprintf(cmd_in, "%s\n", oid_to_hex(&oid));
>>> +       }
>>> +
>>> +       if (fclose(cmd_in)) {
>>> +               error_errno(_("could not close stdin of pack-objects"));
>>> +               result = 1;
>>> +               finish_command(&cmd);
>>> +               goto cleanup;
>>>         }
>>> -       close(cmd.in);
>>>
>>>         if (finish_command(&cmd)) {
>>>                 error(_("could not finish pack-objects"));
>>> --
>>> 2.28.0
>>
>> Here, we don't have any explicit errno checking, but
>> of course error_errno() uses errno.  This too needs
>> an ferror() (or fflush()) test before the final fclose(),
>> and then we just need to use plain error().  Otherwise
>> you'll need the clumsier test-after-each-fprintf() and
>> an explicit final fflush()-and-test.

OK, the implicit fflush() called by fclose() and thus fclose() itself
can succeed even if the error indicator is set, in particular if that
fflush() has nothing to do.  So we need to check ferror() before calling
fclose().

If ferror() tells us there was an error, errno might contain some random
error code, but not necessarily the root cause.  Thus we better keep
quiet about it and only use error() to tell the user we failed to talk
to our child but we don't know why.

We could fflush() explicitly before fclose(), but fclose() reports any
failure of its implicit fflush() anyway , so we don't gain anything by
doing so.

Did I get that right?

> We need this explicit test after each fprintf anyway because SIGPIPE may
> be ignored, and then writing fails with EPIPE. On Windows, this is
> doubly important because we do not have SIGPIPE at all (and always see
> EPIPE), but we see EPIPE only on the first failed write; subsequent
> writes produce EINVAL.

Why is this important?  The current code doesn't care about it, at
least.  It does care about EINTR, though.

René


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] midx: use buffered I/O to talk to pack-objects
  2020-08-03 22:27     ` René Scharfe
@ 2020-08-04  4:31       ` René Scharfe
  2020-08-04  4:37         ` Junio C Hamano
  0 siblings, 1 reply; 14+ messages in thread
From: René Scharfe @ 2020-08-04  4:31 UTC (permalink / raw)
  To: Johannes Sixt, Chris Torek; +Cc: Git Mailing List, Junio C Hamano

Am 04.08.20 um 00:27 schrieb René Scharfe:
> Am 03.08.20 um 20:10 schrieb Johannes Sixt:
>> Am 02.08.20 um 18:11 schrieb Chris Torek:
>>> On Sun, Aug 2, 2020 at 7:40 AM René Scharfe <l.s.r@web.de> wrote:
>>>> @@ -1443,10 +1446,15 @@ int midx_repack(struct repository *r, const char *object_dir, size_t batch_size,
>>>>                         continue;
>>>>
>>>>                 nth_midxed_object_oid(&oid, m, i);
>>>> -               xwrite(cmd.in, oid_to_hex(&oid), the_hash_algo->hexsz);
>>>> -               xwrite(cmd.in, "\n", 1);
>>>> +               fprintf(cmd_in, "%s\n", oid_to_hex(&oid));
>>>> +       }
>>>> +
>>>> +       if (fclose(cmd_in)) {
>>>> +               error_errno(_("could not close stdin of pack-objects"));
>>>> +               result = 1;
>>>> +               finish_command(&cmd);
>>>> +               goto cleanup;
>>>>         }
>>>> -       close(cmd.in);
>>>>
>>>>         if (finish_command(&cmd)) {
>>>>                 error(_("could not finish pack-objects"));
>>>> --
>>>> 2.28.0

>> We need this explicit test after each fprintf anyway because SIGPIPE may
>> be ignored, and then writing fails with EPIPE. On Windows, this is
>> doubly important because we do not have SIGPIPE at all (and always see
>> EPIPE), but we see EPIPE only on the first failed write; subsequent
>> writes produce EINVAL.
>
> Why is this important?  The current code doesn't care about it, at
> least.  It does care about EINTR, though.

Ah, that's the point, right?  You want to *ignore* EPIPE, because the
failed pack-objects process at the other end will have died with a
(hopefully) useful error message already.

OK, so we also need a fprintf() wrapper that retries on EINTR, ignores
EPIPE and exits early if the error indicator is set?

Somehow staying with write(2) and its friends and just adding strbuf
based buffering looks attractive to me now. :-/

René

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] midx: use buffered I/O to talk to pack-objects
  2020-08-04  4:31       ` René Scharfe
@ 2020-08-04  4:37         ` Junio C Hamano
  0 siblings, 0 replies; 14+ messages in thread
From: Junio C Hamano @ 2020-08-04  4:37 UTC (permalink / raw)
  To: René Scharfe; +Cc: Johannes Sixt, Chris Torek, Git Mailing List

René Scharfe <l.s.r@web.de> writes:

> Somehow staying with write(2) and its friends and just adding strbuf
> based buffering looks attractive to me now. :-/

Indeed :-/

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] midx: use buffered I/O to talk to pack-objects
  2020-08-03 12:39 ` Derrick Stolee
@ 2020-08-11 16:08   ` René Scharfe
  2020-08-11 17:14     ` Derrick Stolee
  0 siblings, 1 reply; 14+ messages in thread
From: René Scharfe @ 2020-08-11 16:08 UTC (permalink / raw)
  To: Derrick Stolee, Git Mailing List; +Cc: Junio C Hamano, Chris Torek

Am 03.08.20 um 14:39 schrieb Derrick Stolee:
> On 8/2/2020 10:38 AM, René Scharfe wrote:
>> Like f0bca72dc77 (send-pack: use buffered I/O to talk to pack-objects,
>> 2016-06-08), significantly reduce the number of system calls and
>> simplify the code for sending object IDs to pack-objects by using
>> stdio's buffering and handling errors after the loop.
>
> Good find. Thanks for doing this important cleanup.
>
> Outside of Chris's other feedback, this looks like an obviously
> correct transformation.

I spent a surprising amount of time trying to find a solution that is
easy to use and allows precise error handling.  But now I get second
thoughts.  The main selling point of buffering is better performance,
which is achieved by reducing the number of system calls.  How much
better actually?

So I get this in my Git repo clone without this patch:

  $ strace --summary-only --trace=write git multi-pack-index repack --no-progress
  % time     seconds  usecs/call     calls    errors syscall
  ------ ----------- ----------- --------- --------- ----------------
  100.00    2.237478           2    921650           write
  ------ ----------- ----------- --------- --------- ----------------
  100.00    2.237478                921650           total

And here's the same with the patch:

  % time     seconds  usecs/call     calls    errors syscall
  ------ ----------- ----------- --------- --------- ----------------
  100.00    0.013293           2      4613           write
  ------ ----------- ----------- --------- --------- ----------------
  100.00    0.013293                  4613           total

Awesome, right?  write(2) calls are down by a factor of almost 200 and
the time spent on them is reduced significantly, as advertised.  Let's
ask hyperfine for a second opinion though.  Without this patch:

  Benchmark #1: git multi-pack-index repack --no-progress
    Time (mean ± σ):      1.652 s ±  0.206 s    [User: 1.383 s, System: 0.317 s]
    Range (min … max):    1.426 s …  1.890 s    10 runs

And the same with this patch applied:

    Time (mean ± σ):      1.635 s ±  0.199 s    [User: 1.363 s, System: 0.204 s]
    Range (min … max):    1.430 s …  1.871 s    10 runs

OK, so system time is down by ca. 50%, but the total duration is
basically unchanged.  It seems strace added quite some overhead to our
measurement above.

Anyway, now I wonder if adding our own buffer on top if the
OS-internal pipe buffer is actually worth it.  The numbers above are
from Debian testing , by the way.  Perhaps buffering still pays off on
operating systems with slower pipes..

René

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] midx: use buffered I/O to talk to pack-objects
  2020-08-11 16:08   ` René Scharfe
@ 2020-08-11 17:14     ` Derrick Stolee
  0 siblings, 0 replies; 14+ messages in thread
From: Derrick Stolee @ 2020-08-11 17:14 UTC (permalink / raw)
  To: René Scharfe, Git Mailing List; +Cc: Junio C Hamano, Chris Torek

On 8/11/2020 12:08 PM, René Scharfe wrote:
> Am 03.08.20 um 14:39 schrieb Derrick Stolee:
>> On 8/2/2020 10:38 AM, René Scharfe wrote:
>>> Like f0bca72dc77 (send-pack: use buffered I/O to talk to pack-objects,
>>> 2016-06-08), significantly reduce the number of system calls and
>>> simplify the code for sending object IDs to pack-objects by using
>>> stdio's buffering and handling errors after the loop.
>>
>> Good find. Thanks for doing this important cleanup.
>>
>> Outside of Chris's other feedback, this looks like an obviously
>> correct transformation.
> 
> I spent a surprising amount of time trying to find a solution that is
> easy to use and allows precise error handling.  But now I get second
> thoughts.  The main selling point of buffering is better performance,
> which is achieved by reducing the number of system calls.  How much
> better actually?
> 
> So I get this in my Git repo clone without this patch:
> 
>   $ strace --summary-only --trace=write git multi-pack-index repack --no-progress
>   % time     seconds  usecs/call     calls    errors syscall
>   ------ ----------- ----------- --------- --------- ----------------
>   100.00    2.237478           2    921650           write
>   ------ ----------- ----------- --------- --------- ----------------
>   100.00    2.237478                921650           total
> 
> And here's the same with the patch:
> 
>   % time     seconds  usecs/call     calls    errors syscall
>   ------ ----------- ----------- --------- --------- ----------------
>   100.00    0.013293           2      4613           write
>   ------ ----------- ----------- --------- --------- ----------------
>   100.00    0.013293                  4613           total
> 
> Awesome, right?  write(2) calls are down by a factor of almost 200 and
> the time spent on them is reduced significantly, as advertised.  Let's
> ask hyperfine for a second opinion though.  Without this patch:
> 
>   Benchmark #1: git multi-pack-index repack --no-progress
>     Time (mean ± σ):      1.652 s ±  0.206 s    [User: 1.383 s, System: 0.317 s]
>     Range (min … max):    1.426 s …  1.890 s    10 runs
> 
> And the same with this patch applied:
> 
>     Time (mean ± σ):      1.635 s ±  0.199 s    [User: 1.363 s, System: 0.204 s]
>     Range (min … max):    1.430 s …  1.871 s    10 runs
> 
> OK, so system time is down by ca. 50%, but the total duration is
> basically unchanged.  It seems strace added quite some overhead to our
> measurement above.
> 
> Anyway, now I wonder if adding our own buffer on top if the
> OS-internal pipe buffer is actually worth it.  The numbers above are
> from Debian testing , by the way.  Perhaps buffering still pays off on
> operating systems with slower pipes..

For what it's worth, I took your patch and applied it on Git for Windows
and tested 'git multi-pack-index repack' on my copy of the Git repo
(which includes Git for Windows and microsoft/git for a total of
1.7 million objects) and saw the time improve from 22.3s to 16.6s!

The "Enumerating objects" progress bar was visibly faster when I was
watching the command.

I was not expecting such a huge speed bump, seeing how the objects
are being repacked, so this command includes complicated processes
like delta compression an zlib compression.

Thanks! This is definitely worth the speed boost on Windows.

-Stolee

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH v2] midx: use buffered I/O to talk to pack-objects
  2020-08-02 14:38 [PATCH] midx: use buffered I/O to talk to pack-objects René Scharfe
  2020-08-02 16:11 ` Chris Torek
  2020-08-03 12:39 ` Derrick Stolee
@ 2020-08-12 16:52 ` René Scharfe
  2020-08-12 20:28   ` Junio C Hamano
  2 siblings, 1 reply; 14+ messages in thread
From: René Scharfe @ 2020-08-12 16:52 UTC (permalink / raw)
  To: Git Mailing List
  Cc: Junio C Hamano, Chris Torek, Johannes Sixt, Derrick Stolee

Like f0bca72dc77 (send-pack: use buffered I/O to talk to pack-objects,
2016-06-08), significantly reduce the number of system calls and
simplify the code for sending object IDs to pack-objects by using
stdio's buffering.

Helped-by: Chris Torek <chris.torek@gmail.com>
Helped-by: Johannes Sixt <j6t@kdbg.org>
Encouraged-by: Derrick Stolee <stolee@gmail.com>
Signed-off-by: René Scharfe <l.s.r@web.de>
---
Change since v1:
- Removed error handling to match the original code.

 midx.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/midx.c b/midx.c
index a5fb797edee..0f95c3d8526 100644
--- a/midx.c
+++ b/midx.c
@@ -1383,6 +1383,7 @@ int midx_repack(struct repository *r, const char *object_dir, size_t batch_size,
 	uint32_t i;
 	unsigned char *include_pack;
 	struct child_process cmd = CHILD_PROCESS_INIT;
+	FILE *cmd_in;
 	struct strbuf base_name = STRBUF_INIT;
 	struct multi_pack_index *m = load_multi_pack_index(object_dir, 1);

@@ -1435,6 +1436,8 @@ int midx_repack(struct repository *r, const char *object_dir, size_t batch_size,
 		goto cleanup;
 	}

+	cmd_in = xfdopen(cmd.in, "w");
+
 	for (i = 0; i < m->num_objects; i++) {
 		struct object_id oid;
 		uint32_t pack_int_id = nth_midxed_pack_int_id(m, i);
@@ -1443,10 +1446,9 @@ int midx_repack(struct repository *r, const char *object_dir, size_t batch_size,
 			continue;

 		nth_midxed_object_oid(&oid, m, i);
-		xwrite(cmd.in, oid_to_hex(&oid), the_hash_algo->hexsz);
-		xwrite(cmd.in, "\n", 1);
+		fprintf(cmd_in, "%s\n", oid_to_hex(&oid));
 	}
-	close(cmd.in);
+	fclose(cmd_in);

 	if (finish_command(&cmd)) {
 		error(_("could not finish pack-objects"));
--
2.28.0

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2] midx: use buffered I/O to talk to pack-objects
  2020-08-12 16:52 ` [PATCH v2] " René Scharfe
@ 2020-08-12 20:28   ` Junio C Hamano
  2020-08-12 20:31     ` Junio C Hamano
  2020-08-13  9:06     ` Jeff King
  0 siblings, 2 replies; 14+ messages in thread
From: Junio C Hamano @ 2020-08-12 20:28 UTC (permalink / raw)
  To: René Scharfe
  Cc: Git Mailing List, Chris Torek, Johannes Sixt, Derrick Stolee

René Scharfe <l.s.r@web.de> writes:

>  		nth_midxed_object_oid(&oid, m, i);
> -		xwrite(cmd.in, oid_to_hex(&oid), the_hash_algo->hexsz);
> -		xwrite(cmd.in, "\n", 1);
> +		fprintf(cmd_in, "%s\n", oid_to_hex(&oid));

I do think it is silly to send an object name and terminating LF in
two different system calls per object.

The original uses xwrite() so that it does not have to worry about
having to restart interrupted system calls and such.  Do we need to
do that ourselves now or does the stdio layer take care of it for
us?

>  	}
> -	close(cmd.in);
> +	fclose(cmd_in);
>
>  	if (finish_command(&cmd)) {
>  		error(_("could not finish pack-objects"));
> --
> 2.28.0

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2] midx: use buffered I/O to talk to pack-objects
  2020-08-12 20:28   ` Junio C Hamano
@ 2020-08-12 20:31     ` Junio C Hamano
  2020-08-13  9:11       ` Jeff King
  2020-08-13  9:06     ` Jeff King
  1 sibling, 1 reply; 14+ messages in thread
From: Junio C Hamano @ 2020-08-12 20:31 UTC (permalink / raw)
  To: René Scharfe
  Cc: Git Mailing List, Chris Torek, Johannes Sixt, Derrick Stolee

Junio C Hamano <gitster@pobox.com> writes:

> René Scharfe <l.s.r@web.de> writes:
>
>>  		nth_midxed_object_oid(&oid, m, i);
>> -		xwrite(cmd.in, oid_to_hex(&oid), the_hash_algo->hexsz);
>> -		xwrite(cmd.in, "\n", 1);
>> +		fprintf(cmd_in, "%s\n", oid_to_hex(&oid));
>
> I do think it is silly to send an object name and terminating LF in
> two different system calls per object.
>
> The original uses xwrite() so that it does not have to worry about
> having to restart interrupted system calls and such.

Oops.  There is not much in "and such".  xwrite() only restarts but
the caller must deal with truncated write, which the original does
not do.


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2] midx: use buffered I/O to talk to pack-objects
  2020-08-12 20:28   ` Junio C Hamano
  2020-08-12 20:31     ` Junio C Hamano
@ 2020-08-13  9:06     ` Jeff King
  1 sibling, 0 replies; 14+ messages in thread
From: Jeff King @ 2020-08-13  9:06 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: René Scharfe, Git Mailing List, Chris Torek, Johannes Sixt,
	Derrick Stolee

On Wed, Aug 12, 2020 at 01:28:22PM -0700, Junio C Hamano wrote:

> René Scharfe <l.s.r@web.de> writes:
> 
> >  		nth_midxed_object_oid(&oid, m, i);
> > -		xwrite(cmd.in, oid_to_hex(&oid), the_hash_algo->hexsz);
> > -		xwrite(cmd.in, "\n", 1);
> > +		fprintf(cmd_in, "%s\n", oid_to_hex(&oid));
> 
> I do think it is silly to send an object name and terminating LF in
> two different system calls per object.
> 
> The original uses xwrite() so that it does not have to worry about
> having to restart interrupted system calls and such.  Do we need to
> do that ourselves now or does the stdio layer take care of it for
> us?

I think we're OK in this instance because we are not expecting writes to
happen at any given moment. We might do a partial write() for any given
fprintf(), but we're OK as long as by the fflush() at the end everything
is written.

For more general cases where we do care about ordering (e.g., if we were
interleaving writes and reads with something like cat-file), we'd need
to fflush() after each write. And I'd expect fflush() to retry across
partial write() or EINTR anyway as a quality-of-implementation thing.
Which doesn't make it true, but glibc empirically seems to behave that
way, and I think we'd be better off adding a transparent wrapper to
fflush() on any system that doesn't.

-Peff

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2] midx: use buffered I/O to talk to pack-objects
  2020-08-12 20:31     ` Junio C Hamano
@ 2020-08-13  9:11       ` Jeff King
  0 siblings, 0 replies; 14+ messages in thread
From: Jeff King @ 2020-08-13  9:11 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: René Scharfe, Git Mailing List, Chris Torek, Johannes Sixt,
	Derrick Stolee

On Wed, Aug 12, 2020 at 01:31:58PM -0700, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > René Scharfe <l.s.r@web.de> writes:
> >
> >>  		nth_midxed_object_oid(&oid, m, i);
> >> -		xwrite(cmd.in, oid_to_hex(&oid), the_hash_algo->hexsz);
> >> -		xwrite(cmd.in, "\n", 1);
> >> +		fprintf(cmd_in, "%s\n", oid_to_hex(&oid));
> >
> > I do think it is silly to send an object name and terminating LF in
> > two different system calls per object.
> >
> > The original uses xwrite() so that it does not have to worry about
> > having to restart interrupted system calls and such.
> 
> Oops.  There is not much in "and such".  xwrite() only restarts but
> the caller must deal with truncated write, which the original does
> not do.

Yeah, so I think the new code should be an improvement there, because it
will eventually retry the write when the buffer fills, or when we
eventually flush on close (my "everything written by the time we hit
fflush()" in the previous mail really should have been "...by the time
we hit fclose()").  Maybe another reason to use stdio. :)

The original also doesn't handle "real" errors at all (stuff besides
EINTR, etc). Nor does the rewrite. I guess it's unlikely to see a write
error here that doesn't also involve pack-objects returning a non-zero
exit code.

-Peff

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2020-08-13  9:11 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-02 14:38 [PATCH] midx: use buffered I/O to talk to pack-objects René Scharfe
2020-08-02 16:11 ` Chris Torek
2020-08-03 18:10   ` Johannes Sixt
2020-08-03 22:27     ` René Scharfe
2020-08-04  4:31       ` René Scharfe
2020-08-04  4:37         ` Junio C Hamano
2020-08-03 12:39 ` Derrick Stolee
2020-08-11 16:08   ` René Scharfe
2020-08-11 17:14     ` Derrick Stolee
2020-08-12 16:52 ` [PATCH v2] " René Scharfe
2020-08-12 20:28   ` Junio C Hamano
2020-08-12 20:31     ` Junio C Hamano
2020-08-13  9:11       ` Jeff King
2020-08-13  9:06     ` Jeff King

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for the project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git