Date: Wed, 28 Nov 2007 14:17:24 -0800 From: Alfred Perlstein <alfred@freebsd.org> To: "M. Warner Losh" <imp@bsdimp.com> Cc: arch@FreeBSD.org Subject: Re: Code review request: small optimization to localtime.c Message-ID: <20071128221724.GS71382@elvis.mu.org> In-Reply-To: <20071128.151021.709401576.imp@bsdimp.com> References: <20071128.151021.709401576.imp@bsdimp.com>
next in thread | previous in thread | raw e-mail | index | archive | help
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@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@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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20071128221724.GS71382>