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

From: Nicolas Pitre <nico@cam.org>
Date: Apr 6, 2009, 02:31 UTC
Message-ID: <alpine.LFD.2.00.0904052220060.6741@xanadu.home>
In-Reply-To: <449c10960904051909v5ec5d7danc10d13d9a1d613f0@mail.gmail.com>

On Sun, 5 Apr 2009, Dan McGee wrote:

Show 39 quoted lines

> 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