On Sun, Apr 5, 2009 at 9:31 PM, Nicolas Pitre <nico@xxxxxxx> wrote:
> On Sun, 5 Apr 2009, Dan McGee wrote:
>
>> On Sat, Apr 4, 2009 at 7:11 PM, Nicolas Pitre <nico@xxxxxxx> 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. */
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html