[PATCH 1/2] git-repack: use non-dashed update-server-info

[PATCH 1/2] git-repack: use non-dashed update-server-info

[Dan McGee]

Signed-off-by: Dan McGee <dpmcgee@gmail.com>
---
 git-repack.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/git-repack.sh b/git-repack.sh
index 1782a23..0868734 100755
--- a/git-repack.sh
+++ b/git-repack.sh
@@ -181,5 +181,5 @@ fi
 
 case "$no_update_info" in
 t) : ;;
-*) git-update-server-info ;;
+*) git update-server-info ;;
 esac
-- 
1.6.2.2.404.ge96f3.dirty

[PATCH 2/2] pack-objects: report actual number of threads to be used

[Dan McGee]

In the case of a small repository, pack-objects is smart enough to not start
more threads than necessary. However, the output to the user always reports
the value of the pack.threads configuration and not the real number of
threads to be used. This is easily fixed by moving the printing of the
message after we have partitioned our work.

(pack.threads is on autodetect and would be set to 4)
$ git-repack -a -d -f
Counting objects: 55, done.
Delta compression using 2 threads.
Compressing objects: 100% (48/48), done.
Writing objects: 100% (55/55), done.
Total 55 (delta 10), reused 0 (delta 0)

Signed-off-by: Dan McGee <dpmcgee@gmail.com>
---
 builtin-pack-objects.c |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
index 9fc3b35..8c1b036 100644
--- a/builtin-pack-objects.c
+++ b/builtin-pack-objects.c
@@ -1611,9 +1611,6 @@ static void ll_find_deltas(struct object_entry **list, unsigned list_size,
 		find_deltas(list, &list_size, window, depth, processed);
 		return;
 	}
-	if (progress > pack_to_stdout)
-		fprintf(stderr, "Delta compression using %d threads.\n",
-				delta_search_threads);
 
 	/* Partition the work amongst work threads. */
 	for (i = 0; i < delta_search_threads; i++) {
@@ -1638,11 +1635,18 @@ static void ll_find_deltas(struct object_entry **list, unsigned list_size,
 		p[i].list = list;
 		p[i].list_size = sub_size;
 		p[i].remaining = sub_size;
+		if(sub_size)
+			active_threads++;
 
 		list += sub_size;
 		list_size -= sub_size;
 	}
 
+	if (progress > pack_to_stdout)
+		fprintf(stderr, "Delta compression using %d threads.\n",
+				active_threads);
+	active_threads = 0;
+
 	/* Start work threads. */
 	for (i = 0; i < delta_search_threads; i++) {
 		if (!p[i].list_size)
-- 
1.6.2.2.404.ge96f3.dirty

Re: [PATCH 2/2] pack-objects: report actual number of threads to be used

[Jeff King]

On Sat, Apr 04, 2009 at 11:59:56AM -0500, Dan McGee wrote:

> In the case of a small repository, pack-objects is smart enough to not start
> more threads than necessary. However, the output to the user always reports
> the value of the pack.threads configuration and not the real number of
> threads to be used. This is easily fixed by moving the printing of the
> message after we have partitioned our work.
> 
> (pack.threads is on autodetect and would be set to 4)
> $ git-repack -a -d -f
> Counting objects: 55, done.
> Delta compression using 2 threads.
> Compressing objects: 100% (48/48), done.
> Writing objects: 100% (55/55), done.
> Total 55 (delta 10), reused 0 (delta 0)

That makes sense to me, though I wonder if it may confuse and frustrate
users who are expecting their awesome quad-core machine to be using 4
threads when it only uses 2. Is it worth printing both values, or some
indicator that we could have been using more?

-Peff

Re: [PATCH 2/2] pack-objects: report actual number of threads to be used

[Dan McGee]

On Sat, Apr 4, 2009 at 1:06 PM, Jeff King <peff@peff.net> wrote:
> On Sat, Apr 04, 2009 at 11:59:56AM -0500, Dan McGee wrote:
>
>> In the case of a small repository, pack-objects is smart enough to not start
>> more threads than necessary. However, the output to the user always reports
>> the value of the pack.threads configuration and not the real number of
>> threads to be used. This is easily fixed by moving the printing of the
>> message after we have partitioned our work.
>>
>> (pack.threads is on autodetect and would be set to 4)
>> $ git-repack -a -d -f
>> Counting objects: 55, done.
>> Delta compression using 2 threads.
>> Compressing objects: 100% (48/48), done.
>> Writing objects: 100% (55/55), done.
>> Total 55 (delta 10), reused 0 (delta 0)
>
> That makes sense to me, though I wonder if it may confuse and frustrate
> users who are expecting their awesome quad-core machine to be using 4
> threads when it only uses 2. Is it worth printing both values, or some
> indicator that we could have been using more?

I thought of this, but decided it wasn't really worth it. The default
window size of 10 makes it a very rare case that you will use fewer
than 4 threads. With the default, each thread needs a minimum of 20
objects, so even a 100-object repository would spawn the 4 threads.

I wouldn't be opposed to printing something special when
active_threads != delta_search_threads if other people do think it to
be necessary though.

-Dan

Re: [PATCH 2/2] pack-objects: report actual number of threads to be used

[Jeff King]

On Sat, Apr 04, 2009 at 01:20:18PM -0500, Dan McGee wrote:

> > That makes sense to me, though I wonder if it may confuse and frustrate
> > users who are expecting their awesome quad-core machine to be using 4
> > threads when it only uses 2. Is it worth printing both values, or some
> > indicator that we could have been using more?
> 
> I thought of this, but decided it wasn't really worth it. The default
> window size of 10 makes it a very rare case that you will use fewer
> than 4 threads. With the default, each thread needs a minimum of 20
> objects, so even a 100-object repository would spawn the 4 threads.

Good point. Though by that logic, isn't your patch also not worth it
(i.e., it is unlikely not to fill the threads, so the output will be the
same with or without it)?

I still think yours is an improvement, though, however slight.

-Peff

Re: [PATCH 2/2] pack-objects: report actual number of threads to be used

[Nicolas Pitre]

On Sat, 4 Apr 2009, Jeff King wrote:

> On Sat, Apr 04, 2009 at 01:20:18PM -0500, Dan McGee wrote:
> 
> > > That makes sense to me, though I wonder if it may confuse and frustrate
> > > users who are expecting their awesome quad-core machine to be using 4
> > > threads when it only uses 2. Is it worth printing both values, or some
> > > indicator that we could have been using more?
> > 
> > I thought of this, but decided it wasn't really worth it. The default
> > window size of 10 makes it a very rare case that you will use fewer
> > than 4 threads. With the default, each thread needs a minimum of 20
> > objects, so even a 100-object repository would spawn the 4 threads.
> 
> Good point. Though by that logic, isn't your patch also not worth it
> (i.e., it is unlikely not to fill the threads, so the output will be the
> same with or without it)?
> 
> I still think yours is an improvement, though, however slight.

I don't think this is worth it at all.

This display is there mainly to confirm expected number of available 
threads.  The number of actually active threads is an implementation 
detail.  Sure if the number of objects is too low, or if the window size 
is too large, then the number of active threads will be lower.  But in 
practice it is also possible that with some patological object set you 
end up with 2 threads out of 4 completing very quickly and the other 2 
threads still busy with big objects and total remaining work set too 
small to split it further amongst idle threads, meaning that you'll end 
up with only 2 busy CPUs even though the display said 4 threads 
initially even with this patch.

In other words I don't think this patch is a good idea as we don't 
update the display with remaining active threads along the way.


Nicolas

Re: [PATCH 2/2] pack-objects: report actual number of threads to be used

[Dan McGee]

On Sat, Apr 4, 2009 at 7:11 PM, Nicolas Pitre <nico@cam.org> wrote:
> On Sat, 4 Apr 2009, Jeff King wrote:
>
>> On Sat, Apr 04, 2009 at 01:20:18PM -0500, Dan McGee wrote:
>>
>> > > That makes sense to me, though I wonder if it may confuse and frustrate
>> > > users who are expecting their awesome quad-core machine to be using 4
>> > > threads when it only uses 2. Is it worth printing both values, or some
>> > > indicator that we could have been using more?
>> >
>> > I thought of this, but decided it wasn't really worth it. The default
>> > window size of 10 makes it a very rare case that you will use fewer
>> > than 4 threads. With the default, each thread needs a minimum of 20
>> > objects, so even a 100-object repository would spawn the 4 threads.
>>
>> Good point. Though by that logic, isn't your patch also not worth it
>> (i.e., it is unlikely not to fill the threads, so the output will be the
>> same with or without it)?
>>
>> I still think yours is an improvement, though, however slight.
>
> I don't think this is worth it at all.
>
> This display is there mainly to confirm expected number of available
> threads.  The number of actually active threads is an implementation
> detail.  Sure if the number of objects is too low, or if the window size
> is too large, then the number of active threads will be lower.  But in
> practice it is also possible that with some patological object set you
> end up with 2 threads out of 4 completing very quickly and the other 2
> threads still busy with big objects and total remaining work set too
> small to split it further amongst idle threads, meaning that you'll end
> up with only 2 busy CPUs even though the display said 4 threads
> initially even with this patch.
>
> In other words I don't think this patch is a good idea as we don't
> update the display with remaining active threads along the way.

Why do we show this misleading at best piece of information at all
then? I'd rather completely remove it than show lies to the user. It
sounds like it is only there for debugging purposes.

If we choose to keep it, I propose either accepting my patch so we are
not mislead, or dropping the thread count completely from the output
and saying only something like "Using multi-threaded delta
compression."

-Dan

Re: [PATCH 2/2] pack-objects: report actual number of threads to be used

[Nicolas Pitre]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2871 bytes --]

On Sun, 5 Apr 2009, Dan McGee wrote:

> On Sat, Apr 4, 2009 at 7:11 PM, Nicolas Pitre <nico@cam.org> wrote:
> > On Sat, 4 Apr 2009, Jeff King wrote:
> >
> >> On Sat, Apr 04, 2009 at 01:20:18PM -0500, Dan McGee wrote:
> >>
> >> > > That makes sense to me, though I wonder if it may confuse and frustrate
> >> > > users who are expecting their awesome quad-core machine to be using 4
> >> > > threads when it only uses 2. Is it worth printing both values, or some
> >> > > indicator that we could have been using more?
> >> >
> >> > I thought of this, but decided it wasn't really worth it. The default
> >> > window size of 10 makes it a very rare case that you will use fewer
> >> > than 4 threads. With the default, each thread needs a minimum of 20
> >> > objects, so even a 100-object repository would spawn the 4 threads.
> >>
> >> Good point. Though by that logic, isn't your patch also not worth it
> >> (i.e., it is unlikely not to fill the threads, so the output will be the
> >> same with or without it)?
> >>
> >> I still think yours is an improvement, though, however slight.
> >
> > I don't think this is worth it at all.
> >
> > This display is there mainly to confirm expected number of available
> > threads.  The number of actually active threads is an implementation
> > detail.  Sure if the number of objects is too low, or if the window size
> > is too large, then the number of active threads will be lower.  But in
> > practice it is also possible that with some patological object set you
> > end up with 2 threads out of 4 completing very quickly and the other 2
> > threads still busy with big objects and total remaining work set too
> > small to split it further amongst idle threads, meaning that you'll end
> > up with only 2 busy CPUs even though the display said 4 threads
> > initially even with this patch.
> >
> > In other words I don't think this patch is a good idea as we don't
> > update the display with remaining active threads along the way.
> 
> Why do we show this misleading at best piece of information at all
> then? I'd rather completely remove it than show lies to the user.

As you might imagine, I don't share your above appreciation.

> It
> sounds like it is only there for debugging purposes.

... which is still worthwhile nevertheless.

> If we choose to keep it, I propose either accepting my patch so we are
> not mislead, or dropping the thread count completely from the output
> and saying only something like "Using multi-threaded delta
> compression."

Your patch is not better.  Instead, it will confuse people who 
explicitly told git to use x threads but the display might say x-y 
threads, with 0 <= y < x.

The number currently displayed has real meaning: this is the number of 
threads git is allowed to use.  The number of threads it will actually 
use is variable and it changes with time.


Nicolas

Re: [PATCH 2/2] pack-objects: report actual number of threads to be used

[Dan McGee]

On Sun, Apr 5, 2009 at 9:31 PM, Nicolas Pitre <nico@cam.org> wrote:
> On Sun, 5 Apr 2009, Dan McGee wrote:
>
>> On Sat, Apr 4, 2009 at 7:11 PM, Nicolas Pitre <nico@cam.org> wrote:
>> > On Sat, 4 Apr 2009, Jeff King wrote:
>> >
>> >> On Sat, Apr 04, 2009 at 01:20:18PM -0500, Dan McGee wrote:
>> >>
>> >> > > That makes sense to me, though I wonder if it may confuse and frustrate
>> >> > > users who are expecting their awesome quad-core machine to be using 4
>> >> > > threads when it only uses 2. Is it worth printing both values, or some
>> >> > > indicator that we could have been using more?
>> >> >
>> >> > I thought of this, but decided it wasn't really worth it. The default
>> >> > window size of 10 makes it a very rare case that you will use fewer
>> >> > than 4 threads. With the default, each thread needs a minimum of 20
>> >> > objects, so even a 100-object repository would spawn the 4 threads.
>> >>
>> >> Good point. Though by that logic, isn't your patch also not worth it
>> >> (i.e., it is unlikely not to fill the threads, so the output will be the
>> >> same with or without it)?
>> >>
>> >> I still think yours is an improvement, though, however slight.
>> >
>> > I don't think this is worth it at all.
>> >
>> > This display is there mainly to confirm expected number of available
>> > threads.  The number of actually active threads is an implementation
>> > detail.  Sure if the number of objects is too low, or if the window size
>> > is too large, then the number of active threads will be lower.  But in
>> > practice it is also possible that with some patological object set you
>> > end up with 2 threads out of 4 completing very quickly and the other 2
>> > threads still busy with big objects and total remaining work set too
>> > small to split it further amongst idle threads, meaning that you'll end
>> > up with only 2 busy CPUs even though the display said 4 threads
>> > initially even with this patch.
>> >
>> > In other words I don't think this patch is a good idea as we don't
>> > update the display with remaining active threads along the way.
>>
>> Why do we show this misleading at best piece of information at all
>> then? I'd rather completely remove it than show lies to the user.
>
> As you might imagine, I don't share your above appreciation.
>
>> It
>> sounds like it is only there for debugging purposes.
>
> ... which is still worthwhile nevertheless.
>
>> If we choose to keep it, I propose either accepting my patch so we are
>> not mislead, or dropping the thread count completely from the output
>> and saying only something like "Using multi-threaded delta
>> compression."
>
> Your patch is not better.  Instead, it will confuse people who
> explicitly told git to use x threads but the display might say x-y
> threads, with 0 <= y < x.
>
> The number currently displayed has real meaning: this is the number of
> threads git is allowed to use.  The number of threads it will actually
> use is variable and it changes with time.

Would something like this be more ideal then? I wouldn't be so
persistent here if the current text wasn't misleading in a case like
the following:

dmcgee@galway ~/projects/devtools (master)
$ git push origin
Counting objects: 13, done.
Delta compression using 4 threads.
Compressing objects: 100% (8/8), done.
Writing objects: 100% (8/8), 1.28 KiB, done.
Total 8 (delta 6), reused 0 (delta 0)
To archlinux.org:/srv/projects/git/devtools.git
   bcb0e39..ea73c2b  master -> master

diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
index 9fc3b35..99181fd 100644
--- a/builtin-pack-objects.c
+++ b/builtin-pack-objects.c
@@ -1612,7 +1612,7 @@ static void ll_find_deltas(struct object_entry
**list, unsigned list_size,
                return;
        }
        if (progress > pack_to_stdout)
-               fprintf(stderr, "Delta compression using %d threads.\n",
+               fprintf(stderr, "Delta compression using up to %d threads.\n",
                                delta_search_threads);

        /* Partition the work amongst work threads. */

Re: [PATCH 2/2] pack-objects: report actual number of threads to be used

[Nicolas Pitre]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1516 bytes --]

On Sun, 5 Apr 2009, Dan McGee wrote:

> On Sun, Apr 5, 2009 at 9:31 PM, Nicolas Pitre <nico@cam.org> wrote:
> > The number currently displayed has real meaning: this is the number of
> > threads git is allowed to use.  The number of threads it will actually
> > use is variable and it changes with time.
> 
> Would something like this be more ideal then? I wouldn't be so
> persistent here if the current text wasn't misleading in a case like
> the following:
> 
> dmcgee@galway ~/projects/devtools (master)
> $ git push origin
> Counting objects: 13, done.
> Delta compression using 4 threads.
> Compressing objects: 100% (8/8), done.
> Writing objects: 100% (8/8), 1.28 KiB, done.
> Total 8 (delta 6), reused 0 (delta 0)
> To archlinux.org:/srv/projects/git/devtools.git
>    bcb0e39..ea73c2b  master -> master
> 
> diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
> index 9fc3b35..99181fd 100644
> --- a/builtin-pack-objects.c
> +++ b/builtin-pack-objects.c
> @@ -1612,7 +1612,7 @@ static void ll_find_deltas(struct object_entry
> **list, unsigned list_size,
>                 return;
>         }
>         if (progress > pack_to_stdout)
> -               fprintf(stderr, "Delta compression using %d threads.\n",
> +               fprintf(stderr, "Delta compression using up to %d threads.\n",
>                                 delta_search_threads);
> 
>         /* Partition the work amongst work threads. */

This I have absolutely no issue with.

Acked-by: Nicolas Pitre <nico@cam.org>


Nicolas

Re: [PATCH 2/2] pack-objects: report actual number of threads to be used

[Junio C Hamano]

Nicolas Pitre <nico@cam.org> writes:

>> diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
>> index 9fc3b35..99181fd 100644
>> --- a/builtin-pack-objects.c
>> +++ b/builtin-pack-objects.c
>> @@ -1612,7 +1612,7 @@ static void ll_find_deltas(struct object_entry
>> **list, unsigned list_size,
>>                 return;
>>         }
>>         if (progress > pack_to_stdout)
>> -               fprintf(stderr, "Delta compression using %d threads.\n",
>> +               fprintf(stderr, "Delta compression using up to %d threads.\n",
>>                                 delta_search_threads);
>> 
>>         /* Partition the work amongst work threads. */
>
> This I have absolutely no issue with.
>
> Acked-by: Nicolas Pitre <nico@cam.org>

I do not have problem with the wording either, but then the commit log
message needs to change, I think.  Care to re-submit?

[PATCH] Update delta compression message to be less misleading

[Dan McGee]

Signed-off-by: Dan McGee <dpmcgee@gmail.com>
---
 builtin-pack-objects.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
index 9fc3b35..99181fd 100644
--- a/builtin-pack-objects.c
+++ b/builtin-pack-objects.c
@@ -1612,7 +1612,7 @@ static void ll_find_deltas(struct object_entry **list, unsigned list_size,
 		return;
 	}
 	if (progress > pack_to_stdout)
-		fprintf(stderr, "Delta compression using %d threads.\n",
+		fprintf(stderr, "Delta compression using up to %d threads.\n",
 				delta_search_threads);
 
 	/* Partition the work amongst work threads. */
-- 
1.6.2.2

Re: [PATCH] Update delta compression message to be less misleading

[Junio C Hamano]

Dan McGee <dpmcgee@gmail.com> writes:

> Signed-off-by: Dan McGee <dpmcgee@gmail.com>

Empty log message?

I am scratching my head because this came as a reply to:

    ...
    > This I have absolutely no issue with.
    >
    > Acked-by: Nicolas Pitre <nico@cam.org>

    I do not have problem with the wording either, but then the commit log
    message needs to change, I think.  Care to re-submit?

and your original read quite nicely:

    Subject: [PATCH 2/2] pack-objects: report actual number of threads to be used
    Date: Sat,  4 Apr 2009 11:59:56 -0500
    Message-ID: <1238864396-8964-2-git-send-email-dpmcgee@gmail.com>

    In the case of a small repository, pack-objects is smart enough to not start
    more threads than necessary. However, the output to the user always reports
    the value of the pack.threads configuration and not the real number of
    threads to be used. This is easily fixed by moving the printing of the
    message after we have partitioned our work.

    (pack.threads is on autodetect and would be set to 4)
    $ git-repack -a -d -f
    Counting objects: 55, done.
    Delta compression using 2 threads.
    Compressing objects: 100% (48/48), done.
    Writing objects: 100% (55/55), done.
    Total 55 (delta 10), reused 0 (delta 0)

    Signed-off-by: Dan McGee <dpmcgee@gmail.com>

I was expecting to see something with a similar structure.  An updated
title, an introductory text and the problem description, and the
description of the solution.  Then "Acked-by:" you already received.

The new title looks correct, the problem description from the original
should still apply, but the solution is different, and that needs to
change.

I've committed it like this:

    Author: Dan McGee <dpmcgee@gmail.com>
    Date:   Thu Apr 9 10:45:39 2009 -0500

    Update delta compression message to be less misleading
    
    In the case of a small repository, pack-objects is smart enough to not
    start more threads than necessary. However, the output to the user always
    reports the value of the pack.threads configuration and not the real
    number of threads to be used.
    
    Signed-off-by: Dan McGee <dpmcgee@gmail.com>
    Acked-by: Nicolas Pitre <nico@cam.org>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>

Thanks.

Re: [PATCH] Update delta compression message to be less misleading

[Dan McGee]

On Sat, Apr 11, 2009 at 2:24 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Dan McGee <dpmcgee@gmail.com> writes:
>
>> Signed-off-by: Dan McGee <dpmcgee@gmail.com>
>
> Empty log message?
>
> I am scratching my head because this came as a reply to:
>
>    ...
>    > This I have absolutely no issue with.
>    >
>    > Acked-by: Nicolas Pitre <nico@cam.org>
>
>    I do not have problem with the wording either, but then the commit log
>    message needs to change, I think.  Care to re-submit?
>
> and your original read quite nicely:
>
>    Subject: [PATCH 2/2] pack-objects: report actual number of threads to be used
>    Date: Sat,  4 Apr 2009 11:59:56 -0500
>    Message-ID: <1238864396-8964-2-git-send-email-dpmcgee@gmail.com>
>
>    In the case of a small repository, pack-objects is smart enough to not start
>    more threads than necessary. However, the output to the user always reports
>    the value of the pack.threads configuration and not the real number of
>    threads to be used. This is easily fixed by moving the printing of the
>    message after we have partitioned our work.
>
>    (pack.threads is on autodetect and would be set to 4)
>    $ git-repack -a -d -f
>    Counting objects: 55, done.
>    Delta compression using 2 threads.
>    Compressing objects: 100% (48/48), done.
>    Writing objects: 100% (55/55), done.
>    Total 55 (delta 10), reused 0 (delta 0)
>
>    Signed-off-by: Dan McGee <dpmcgee@gmail.com>
>
> I was expecting to see something with a similar structure.  An updated
> title, an introductory text and the problem description, and the
> description of the solution.  Then "Acked-by:" you already received.
>
> The new title looks correct, the problem description from the original
> should still apply, but the solution is different, and that needs to
> change.
>
> I've committed it like this:
>
>    Author: Dan McGee <dpmcgee@gmail.com>
>    Date:   Thu Apr 9 10:45:39 2009 -0500
>
>    Update delta compression message to be less misleading
>
>    In the case of a small repository, pack-objects is smart enough to not
>    start more threads than necessary. However, the output to the user always
>    reports the value of the pack.threads configuration and not the real
>    number of threads to be used.
>
>    Signed-off-by: Dan McGee <dpmcgee@gmail.com>
>    Acked-by: Nicolas Pitre <nico@cam.org>
>    Signed-off-by: Junio C Hamano <gitster@pobox.com>
>
> Thanks.

Sorry about that. I felt like the patch was a lot more
self-explanatory now, since it was a text change and not a behavior
change. It was my fault for forgetting the Ack.

-Dan