Re: Code review request: small optimization to localtime.c

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@xxxxxxxxxx> [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@xxxxxxxxxxx mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-arch
To unsubscribe, send any mail to "freebsd-arch-unsubscribe@xxxxxxxxxxx"

--
- Alfred Perlstein
_______________________________________________
freebsd-arch@xxxxxxxxxxx mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-arch
To unsubscribe, send any mail to "freebsd-arch-unsubscribe@xxxxxxxxxxx"

Relevant Pages