threads / patch / 18715
Subject: [PATCH 1/2] git-repack: use non-dashed update-server-info
Dan McGeeSigned-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
Dan McGee[PATCH 2/2] pack-objects: report actual number of threads to be used
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
Jeff KingRe: [PATCH 2/2] pack-objects: report actual number of threads to be used
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
Dan McGeeRe: [PATCH 2/2] pack-objects: report actual number of threads to be used
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
Jeff KingRe: [PATCH 2/2] pack-objects: report actual number of threads to be used
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
Nicolas PitreRe: [PATCH 2/2] pack-objects: report actual number of threads to be used
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
Dan McGeeRe: [PATCH 2/2] pack-objects: report actual number of threads to be used
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
Nicolas PitreRe: [PATCH 2/2] pack-objects: report actual number of threads to be used
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
Dan McGeeRe: [PATCH 2/2] pack-objects: report actual number of threads to be used
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. */
Nicolas PitreRe: [PATCH 2/2] pack-objects: report actual number of threads to be used
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
Junio C HamanoRe: [PATCH 2/2] pack-objects: report actual number of threads to be used
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?
Dan McGee[PATCH] Update delta compression message to be less misleading
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
Junio C HamanoRe: [PATCH] Update delta compression message to be less misleading
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.
Dan McGeeRe: [PATCH] Update delta compression message to be less misleading
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