From: Andreas Ericsson <ae@op5.se>
Date: Tue, 12 Feb 2008 05:53:00 GMT
Subject: Re: [PATCH 1/2] pack-objects: Allow setting the #threads equal to #cpus automatically
Message-ID: <47B1343C.8070405@op5.se>
In-Reply-To: <47B10A9D.7000702@nrlssc.navy.mil>

Brandon Casey wrote:
> Allow pack.threads config option and --threads command line option to
> accept '0' as an argument and set the number of created threads equal
> to the number of online processors in this case.
> 
> Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
> ---
> 
> 
> I was preparing this patch when I saw your email. I looked up your
> the old email you were talking about. Your function is better since
> it is cross platform.
> 
> When you redo your patch, you may want to adopt one aspect of this
> one. I used a setting of zero to imply "set number of threads to
> number of cpus". This allows the user to specifically set pack.threads
> in the config file to zero with the above mentioned meaning, or to
> override a setting in the config file from the command line with
> --threads=0. This is rather than having to delete the option from the
> config file.
> 

That make sense. Perhaps even go so far as to allow 'auto' as a
keyword would be nifty.

>  
> +#ifdef THREADED_DELTA_SEARCH
> +	if (!delta_search_threads) {
> +#if defined _SC_NPROCESSORS_ONLN
> +		delta_search_threads = sysconf(_SC_NPROCESSORS_ONLN);
> +#elif defined _SC_NPROC_ONLN
> +		delta_search_threads = sysconf(_SC_NPROC_ONLN);
> +#endif
> +		if (delta_search_threads == -1)
> +			perror("Could not detect number of processors");
> +		if (delta_search_threads <= 0)
> +			delta_search_threads = 1;
> +	}
> +#endif
> +

But this is not so good. For one thing you've dropped windows support
entirely. The last comment on my own patch was that get_num_active_cpus()
should live in a file of its own. You've taken one step back from that
and not even kept it in its own function.

I think perhaps it's time to introduce thread-compat.[ch] to deal with
thread-related cross-platform things like this.

I'll recook my patch and send it in a few minutes, using your suggestions
and Nicolas combined.

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231