From owner-freebsd-arch@FreeBSD.ORG Wed Nov 28 22:17:24 2007 Return-Path: Delivered-To: arch@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id BD1A516A417 for ; Wed, 28 Nov 2007 22:17:24 +0000 (UTC) (envelope-from bright@elvis.mu.org) Received: from elvis.mu.org (elvis.mu.org [192.203.228.196]) by mx1.freebsd.org (Postfix) with ESMTP id AF3CA13C468 for ; Wed, 28 Nov 2007 22:17:24 +0000 (UTC) (envelope-from bright@elvis.mu.org) Received: by elvis.mu.org (Postfix, from userid 1192) id 8B2AC1A4D7E; Wed, 28 Nov 2007 14:17:24 -0800 (PST) Date: Wed, 28 Nov 2007 14:17:24 -0800 From: Alfred Perlstein To: "M. Warner Losh" Message-ID: <20071128221724.GS71382@elvis.mu.org> References: <20071128.151021.709401576.imp@bsdimp.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20071128.151021.709401576.imp@bsdimp.com> User-Agent: Mutt/1.4.2.3i Cc: arch@FreeBSD.org Subject: Re: Code review request: small optimization to localtime.c X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 28 Nov 2007 22:17:24 -0000 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 [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@freebsd.org mailing list > http://lists.freebsd.org/mailman/listinfo/freebsd-arch > To unsubscribe, send any mail to "freebsd-arch-unsubscribe@freebsd.org" -- - Alfred Perlstein