Code review request: small optimization to localtime.c

[Alfred Perlstein]

See pthread_once...

Without pthread_once this is how it's suggested you do things
by just about every threading/smp book out there.

One thing you have to do is to make sure not to set "is_set"
until after the work is done.  Just use pthread_once.

-Alfred

* M. Warner Losh <imp at bsdimp.com> [071128 14:11] wrote:
> Please find enclosed some small optimizations.  I know they make a
> couple lines too long, I'll correct that before I commit.  They make
> the time functions do less redundant locking.
> 
> However, in many places we do the following:
> 
> 	pthread_mutex_lock();
> 	if (!is_set) {
> 		is_set = true;
> 		// do something
> 	}
> 	pthread_mutex_unlock();
> 
> This is wasteful.  We get locks ALL the time for every time operation,
> when in fact we need it more rarely.  If we can tolerate losing a
> race, we can eliminate the locking in all but the startup case and
> those threads racing the startup:
> 
> 	if (!is_set) {
> 		pthread_mutex_lock();
> 		if (!is_set) {
> 			is_set = true;
> 			// do something
> 		}
> 		pthread_mutex_unlock();
> 	}
> 
> here, we know that is_set only ever changes from false to true.  If it
> is already true, there's nothing to do.  If it is false, we may need
> to do something, so we lock, check to see if we really need to do it,
> etc.
> 
> Can anybody see a flaw in this logic?
> 
> Warner
> 
> Index: localtime.c
> ===================================================================
> RCS file: /cache/ncvs/src/lib/libc/stdtime/localtime.c,v
> retrieving revision 1.41
> diff -u -r1.41 localtime.c
> --- localtime.c	19 Jan 2007 01:16:35 -0000	1.41
> +++ localtime.c	28 Nov 2007 21:59:59 -0000
> @@ -1093,14 +1093,16 @@
>  	struct tm *p_tm;
>  
>  	if (__isthreaded != 0) {
> -		_pthread_mutex_lock(&localtime_mutex);
>  		if (localtime_key < 0) {
> -			if (_pthread_key_create(&localtime_key, free) < 0) {
> -				_pthread_mutex_unlock(&localtime_mutex);
> -				return(NULL);
> +			_pthread_mutex_lock(&localtime_mutex);
> +			if (localtime_key < 0) {
> +				if (_pthread_key_create(&localtime_key, free) < 0) {
> +					_pthread_mutex_unlock(&localtime_mutex);
> +					return(NULL);
> +				}
>  			}
> +			_pthread_mutex_unlock(&localtime_mutex);
>  		}
> -		_pthread_mutex_unlock(&localtime_mutex);
>  		p_tm = _pthread_getspecific(localtime_key);
>  		if (p_tm == NULL) {
>  			if ((p_tm = (struct tm *)malloc(sizeof(struct tm)))
> @@ -1146,16 +1148,18 @@
>  const long		offset;
>  struct tm * const	tmp;
>  {
> -	_MUTEX_LOCK(&gmt_mutex);
>  	if (!gmt_is_set) {
> -		gmt_is_set = TRUE;
> +		_MUTEX_LOCK(&gmt_mutex);
> +		if (!gmt_is_set) {
> +			gmt_is_set = TRUE;
>  #ifdef ALL_STATE
> -		gmtptr = (struct state *) malloc(sizeof *gmtptr);
> -		if (gmtptr != NULL)
> +			gmtptr = (struct state *) malloc(sizeof *gmtptr);
> +			if (gmtptr != NULL)
>  #endif /* defined ALL_STATE */
> -			gmtload(gmtptr);
> +				gmtload(gmtptr);
> +		}
> +		_MUTEX_UNLOCK(&gmt_mutex);
>  	}
> -	_MUTEX_UNLOCK(&gmt_mutex);
>  	timesub(timep, offset, gmtptr, tmp);
>  #ifdef TM_ZONE
>  	/*
> @@ -1187,14 +1191,16 @@
>  	struct tm *p_tm;
>  
>  	if (__isthreaded != 0) {
> -		_pthread_mutex_lock(&gmtime_mutex);
>  		if (gmtime_key < 0) {
> -			if (_pthread_key_create(&gmtime_key, free) < 0) {
> -				_pthread_mutex_unlock(&gmtime_mutex);
> -				return(NULL);
> +			_pthread_mutex_lock(&gmtime_mutex);
> +			if (gmtime_key < 0) {
> +				if (_pthread_key_create(&gmtime_key, free) < 0) {
> +					_pthread_mutex_unlock(&gmtime_mutex);
> +					return(NULL);
> +				}
>  			}
> +			_pthread_mutex_unlock(&gmtime_mutex);
>  		}
> -		_pthread_mutex_unlock(&gmtime_mutex);
>  		/*
>  		 * Changed to follow POSIX.1 threads standard, which
>  		 * is what BSD currently has.
> _______________________________________________
> freebsd-arch at freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-arch
> To unsubscribe, send any mail to "freebsd-arch-unsubscribe at freebsd.org"

-- 
- Alfred Perlstein