Skip site navigation (1)Skip section navigation (2) 
Date:      Wed, 28 Nov 2007 15:10:21 -0700 (MST)
From:      "M. Warner Losh" <imp@bsdimp.com>
To:        arch@FreeBSD.org
Subject:   Code review request: small optimization to localtime.c
Message-ID:  <20071128.151021.709401576.imp@bsdimp.com>
next in thread | raw e-mail | index | archive | help 
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.

Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20071128.151021.709401576.imp>