Re-implemented timer code to fix memory leaks and use generic linked list

httperf/ChangeLog

@@ -1,3 +1,15 @@
+2007-09-23  Ted Bullock <tbullock@canada.com>
+
+	* httperf.c: Add a check to return value of timer_schedule
+	* timer.c: Fix a crash associated with rescheduling a timer
+	* lib/list.c: NEW Generic linked list data structure implementation
+	* lib/list.h: NEW Generic linked list data structure API
+
+2007-09-18  Ted Bullock <tbullock@canada.com>
+
+	* timer.c: Remove the last of the old timer logic code and replace it
+	with simpler functionality
+
 2007-09-11  Ted Bullock <tbullock@canada.com>
 
 	* timer.c and timer.h: Use new heap and queue data 

httperf/NEWS

@@ -1,7 +1,10 @@
-* New in version 1.0:
-** timer memory leak fix (Ted Bullock)
+* New in version 0.9.1:
+** timer re-write to reduce memory and fix memory leaks (Ted Bullock)
+** Generic data structures and implementations (heap, queue, linked list)
+** New options (see man-page for details):
+	--period=vT1,D1,T2,D2...Tn,Dn
 
-* New in version 0.9:
+* New in version 0.9.0:
 ** Re-Factored build system now builds on the following platforms
    HP-UX 11i (64-bit PA-RISC and IA-64)
    Red Hat Enterprise Linux AS (AMD64 and IA-64)

httperf/configure.ac

@@ -2,13 +2,13 @@
 # Process this file with autoconf to produce a configure script.
 
 AC_PREREQ(2.60)
-AC_INIT(httperf, 0.9.99, httperf@linux.hpl.hp.com)
+AC_INIT(httperf, 0.9.1, httperf@linux.hpl.hp.com)
 AC_CONFIG_SRCDIR([src/httperf.c])
 AC_CONFIG_HEADER([config.h])
 
 AC_CANONICAL_TARGET
 
-AM_INIT_AUTOMAKE(httperf, 0.9.99)
+AM_INIT_AUTOMAKE(httperf, 0.9.1)
 
 # Checks for programs.
 AC_GNU_SOURCE
@@ -19,7 +19,7 @@ AC_PROG_INSTALL
 AC_PROG_LIBTOOL
 
 # Checks for libraries.
-AC_CHECK_LIB(event, event_init, , AC_MSG_ERROR([libevent is required to build httperf]) ) 
+# AC_CHECK_LIB(event, event_init, , AC_MSG_ERROR([libevent is required to build httperf]) ) 
 AC_CHECK_LIB(m, sqrt)
 AC_CHECK_LIB(crypto, main)
 AC_CHECK_LIB(ssl, SSL_version, , AC_MSG_WARN([SSL Disabled]) )

httperf/src/httperf.c

@@ -206,7 +206,8 @@ perf_sample (struct Timer *t, Any_Type regarg)
 
   /* prepare for next sample interval: */
   perf_sample_start = timer_now ();
-  timer_schedule (perf_sample, regarg, RATE_INTERVAL);
+  if( timer_schedule (perf_sample, regarg, RATE_INTERVAL) == NULL)
+	panic("%s(%d): Received NULL from timer_schedule\n", __func__, __LINE__);
 }
 
 int

httperf/src/lib/Makefile.am

@@ -4,4 +4,4 @@ AM_LDFLAGS =
 
 noinst_LIBRARIES = libutil.a
 libutil_a_SOURCES = getopt.c getopt.h getopt1.c ssl_writev.c generic_types.h \
-					queue.c queue.h heap.c heap.h
+	queue.c queue.h heap.c heap.h list.c list.h

httperf/src/lib/list.c

@@ -0,0 +1,153 @@
+/*
+ * Copyright (C) 2007 Ted Bullock <tbullock@canada.com>
+ * 
+ * This file is part of httperf, a web server performance measurment tool.
+ * 
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the Free
+ * Software Foundation; either version 2 of the License, or (at your option)
+ * any later version.
+ * 
+ * In addition, as a special exception, the copyright holders give permission
+ * to link the code of this work with the OpenSSL project's "OpenSSL" library
+ * (or with modified versions of it that use the same license as the "OpenSSL" 
+ * library), and distribute linked combinations including the two.  You must
+ * obey the GNU General Public License in all respects for all of the code
+ * used other than "OpenSSL".  If you modify this file, you may extend this
+ * exception to your version of the file, but you are not obligated to do so.
+ * If you do not wish to do so, delete this exception statement from your
+ * version.
+ * 
+ * This program is distributed in the hope that it will be useful, but WITHOUT 
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ * 
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc., 51
+ * Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA 
+ */
+
+#include "config.h"
+
+#include <stdlib.h>
+
+#include <generic_types.h>
+
+struct Node {
+	Any_Type        data;
+	struct Node    *next;
+};
+
+struct List {
+	struct Node    *dummy_head;
+};
+
+int
+is_list_empty(struct List *l)
+{
+
+	return l->dummy_head->next == NULL;
+}
+
+struct List    *
+list_create()
+{
+	struct List    *l;
+
+	if ((l = malloc(sizeof(struct List))) == NULL)
+		goto create_error;
+
+	if ((l->dummy_head = malloc(sizeof(struct Node))) == NULL)
+		goto create_error;
+
+	l->dummy_head->next = NULL;
+
+	return l;
+
+      create_error:
+	if (l != NULL)
+		free(l);
+
+	return NULL;
+}
+
+void
+list_free(struct List *l)
+{
+	free(l->dummy_head);
+	l->dummy_head = NULL;
+	free(l);
+}
+
+int
+list_push(struct List *l, Any_Type data)
+{
+	struct Node    *n;
+
+	/*
+	 * TODO: Implement caching so that we don't have to call
+	 * malloc every time we push a new node onto the list
+	 */
+	if ((n = malloc(sizeof(struct Node))) == NULL) {
+		return 0;
+	}
+
+	n->data = data;
+	n->next = l->dummy_head->next;
+	l->dummy_head->next = n;
+
+	return 1;
+}
+
+Any_Type
+list_top(struct List * l)
+{
+	return l->dummy_head->next->data;
+}
+
+Any_Type
+list_pop(struct List * l)
+{
+	Any_Type        data;
+	struct Node    *n;
+
+	n = l->dummy_head->next;
+	data = l->dummy_head->next->data;
+	l->dummy_head->next = l->dummy_head->next->next;
+
+	/*
+	 * TODO: As per above, implement caching here so that this memory
+	 * does not have to be freed
+	 */
+	free(n);
+
+	return data;
+}
+
+void
+list_remove_if_true(struct List *l, int (*action) (Any_Type))
+{
+	struct Node    *n = l->dummy_head;
+
+	while (n->next != NULL) {
+		if ((*action) (n->next->data)) {
+			struct Node    *oldnext = n->next;
+			n->next = n->next->next;
+			free(oldnext);
+		} else
+			n = n->next;
+	}
+
+}
+
+void
+list_for_each(struct List *l, int (*action) (Any_Type))
+{
+	struct Node    *n = l->dummy_head->next;
+
+	while (n != NULL) {
+		(*action) (n->data);
+		n = n->next;
+	}
+}

httperf/src/lib/list.h

@@ -0,0 +1,47 @@
+
+/*
+ * Copyright (C) 2007 Ted Bullock <tbullock@canada.com>
+ * 
+ * This file is part of httperf, a web server performance measurment tool.
+ * 
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the Free
+ * Software Foundation; either version 2 of the License, or (at your option)
+ * any later version.
+ * 
+ * In addition, as a special exception, the copyright holders give permission
+ * to link the code of this work with the OpenSSL project's "OpenSSL" library
+ * (or with modified versions of it that use the same license as the "OpenSSL" 
+ * library), and distribute linked combinations including the two.  You must
+ * obey the GNU General Public License in all respects for all of the code
+ * used other than "OpenSSL".  If you modify this file, you may extend this
+ * exception to your version of the file, but you are not obligated to do so.
+ * If you do not wish to do so, delete this exception statement from your
+ * version.
+ * 
+ * This program is distributed in the hope that it will be useful, but WITHOUT 
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ * 
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc., 51
+ * Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA 
+ */
+
+#ifndef list_h
+#define list_h
+
+typedef int     (*list_action) (Any_Type);
+struct List;
+
+struct List    *list_create();
+void            list_free(struct List *);
+void            list_push(struct List *, Any_Type);
+int             is_list_empty(struct List *);
+Any_Type        list_top(struct List *);
+Any_Type        list_pop(struct List *);
+void            list_remove_if_true(struct List *, list_action);
+void            list_for_each(struct List *, list_action);
+
+#endif /* list_h */

httperf/src/timer.c

@@ -1,6 +1,6 @@
 /*
- * Copyright (C) 2000-2007 Hewlett-Packard Company
  * Copyright (C) 2007 Ted Bullock <tbullock@canada.com>
+ * Copyright (C) 2000-2007 Hewlett-Packard Company
  * 
  * This file is part of httperf, a web server performance measurment tool.
  * 
@@ -38,20 +38,18 @@
 #include <sys/time.h>
 
 #include <generic_types.h>
-#include <heap.h>
-#include <queue.h>
+#include <list.h>
 #include <httperf.h>
 
-#define HEAP_SIZE	4096
-#define WHEEL_SIZE	4096
-
-#define TIMER_INTERVAL	(1.0/1000)	/* timer granularity in seconds */
+#define NUM_TIMERS 0
 
 static Time     now;
 static Time     next_tick;
 
 struct Timer {
-	u_long          delta;
+	Time            time_started;
+	Time            timeout_delay;
+	bool            has_expired;
 
 	/*
 	 * Callback function called when timer expires (timeout) 
@@ -65,26 +63,17 @@ struct Timer {
 };
 
 /*
- * FIFO Queue of inactive timers 
+ * inactive timers 
  */
-static struct Queue *passive_timers = NULL;
+static struct List *passive_timers = NULL;
 /*
- * Min heap of active timers 
+ * active timers 
  */
-static struct Heap *active_timers = NULL;
-
+static struct List *active_timers = NULL;
 /*
- * Executed once a timer has expired, enqueues the timer back into
- * the passive_timers queue for later use
+ * active timeoutless timers 
  */
-static void
-done(struct Timer *t)
-{
-	/*
-	 * Double cast.  Irritating but does the trick 
-	 */
-	enqueue((Any_Type) (void *) t, passive_timers);
-}
+static struct List *persistent_timers = NULL;
 
 /*
  * Returns the time and calls the syscall gettimeofday.  This is an expensive
@@ -112,21 +101,6 @@ timer_now(void)
 		return timer_now_forced();
 }
 
-/*
- * Comparison callback function used by the heap data structure to correctly
- * insert the timer in the proper order (min order as in this case).
- */
-_Bool
-comparator(Any_Type a, Any_Type b)
-{
-	struct Timer   *timer_a, *timer_b;
-
-	timer_a = (struct Timer *) a.vp;
-	timer_b = (struct Timer *) b.vp;
-
-	return timer_a->delta < timer_b->delta;
-}
-
 /*
  * Initializes a large timer pool cache
  * This is a very expensive function.  Call before beginning measurements.
@@ -135,22 +109,26 @@ comparator(Any_Type a, Any_Type b)
 _Bool
 timer_init(void)
 {
-	passive_timers = create_queue(WHEEL_SIZE);
+	passive_timers = list_create();
 	if (passive_timers == NULL)
 		goto init_failure;
 
-	active_timers = create_heap(HEAP_SIZE, &comparator);
+	active_timers = list_create();
 	if (active_timers == NULL)
 		goto init_failure;
 
-	while (!is_queue_full(passive_timers)) {
+	persistent_timers = list_create();
+	if (persistent_timers == NULL)
+		goto init_failure;
+
+	for (int i = 0; i < NUM_TIMERS; i++) {
 		Any_Type        a;
 		a.vp = malloc(sizeof(struct Timer));
 
 		if (a.vp == NULL)
 			goto init_failure;
 
-		enqueue(a, passive_timers);
+		list_push(passive_timers, a);
 	}
 
 	now = timer_now_forced();
@@ -162,63 +140,78 @@ timer_init(void)
 	return false;
 
 }
+
 /*
  * Frees all allocated timers, and timer queues
  */
 void
 timer_free_all(void)
 {
-	while (!is_queue_empty(passive_timers)) {
-		Any_Type        a = get_front_and_dequeue(passive_timers);
+	int             count = 0;
+
+	while (!is_list_empty(passive_timers)) {
+		Any_Type        a = list_pop(passive_timers);
+		fprintf(stderr,
+			"passive_timers Freeing counter at address %p\n",
+			a.vp);
 		free(a.vp);
+		count++;
 	}
-	free_queue(passive_timers);
+	list_free(passive_timers);
+	passive_timers = NULL;
 
-	while (!is_heap_empty(active_timers)) {
-		Any_Type        a = remove_min(active_timers);
+	while (!is_list_empty(active_timers)) {
+		Any_Type        a = list_pop(active_timers);
+		fprintf(stderr,
+			"active_timers Freeing counter at address %p\n", a.vp);
 		free(a.vp);
+		count++;
 	}
-	free_heap(active_timers);
-}
-
-/*
- * Checks for timers which have had their timeout value pass and executes their
- * callback function.  The timer is then removed from the active timer list
- * and then enqueued back into the passive timer queue
- */
-static void
-expire_complete_timers(Any_Type a)
-{
-	struct Timer   *t = (struct Timer *) a.vp;
+	list_free(active_timers);
+	active_timers = NULL;
 
-	if (t->delta == 0) {
-		(*t->timeout_callback) (t, t->timer_subject);
+	while (!is_list_empty(persistent_timers)) {
+		Any_Type        a = list_pop(persistent_timers);
+		fprintf(stderr,
+			"persistent_timers Freeing counter at address %p\n",
+			a.vp);
+		free(a.vp);
+		count++;
+	}
+	list_free(persistent_timers);
+	persistent_timers = NULL;
 
-		Any_Type        verify = remove_min(active_timers);
+	if (DBG > 2)
+		fprintf(stderr, "Freed a total of %d counters\n", count);
+}
 
-		if (verify.vp != t)
-			fprintf(stderr,
-				"Active timer heap is out of min order!\n\t%s.%s(%d): %s\n",
-				__FILE__, __func__, __LINE__, strerror(errno));
+static int
+timer_has_expired(Any_Type a)
+{
+	struct Timer   *t = a.vp;
 
-		/*
-		 * Double cast.  Irritating but does the trick 
-		 */
-		enqueue((Any_Type) (void *) t, passive_timers);
-	}
+	/*
+	 * Only expire currently processing timers 
+	 */
+	if (t->has_expired == false) {
+		if (t->time_started + t->timeout_delay < timer_now()) {
+			t->has_expired = true;
+			(*t->timeout_callback) (t, t->timer_subject);
+			return 1;
+		}
+	} else
+		return 0;
 }
 
-/*
- * To be used to decrement a single timer delta value with the heap_for_each
- * function via a function pointer
- */
-static void
-decrement_timers(Any_Type a)
+static int
+timer_deactivate(Any_Type a)
 {
-	struct Timer   *t = (struct Timer *) a.vp;
+	struct Timer   *t = a.vp;
+
+	if (t->has_expired == true)
+		list_push(passive_timers, a);
 
-	if (t != 0)
-		t->delta--;
+	return t->has_expired;
 }
 
 /*
@@ -230,20 +223,8 @@ void
 timer_tick(void)
 {
 	now = timer_now_forced();
-
-	while (timer_now() >= next_tick) {
-		/*
-		 * Check for timers that have timed out and expire them 
-		 */
-		heap_for_each(active_timers, &expire_complete_timers);
-
-		/*
-		 * Decrement remaining timers
-		 */
-		heap_for_each(active_timers, &decrement_timers);
-
-		next_tick += TIMER_INTERVAL;
-	}
+	list_for_each(active_timers, &timer_has_expired);
+	list_remove_if_true(active_timers, &timer_deactivate);
 }
 
 struct Timer   *
@@ -251,37 +232,24 @@ timer_schedule(void (*timeout) (struct Timer * t, Any_Type arg),
 	       Any_Type subject, Time delay)
 {
 	struct Timer   *t;
-	u_long          ticks;
-	u_long          delta;
-	Time            behind;
 
-	if (!is_queue_empty(passive_timers)) {
-		Any_Type        a = get_front_and_dequeue(passive_timers);
+	if (!is_list_empty(passive_timers)) {
+		Any_Type        a = list_pop(passive_timers);
 		t = (struct Timer *) a.vp;
-	} else
+	} else if ((t = malloc(sizeof(struct Timer))) == NULL)
 		return NULL;
 
 	memset(t, 0, sizeof(struct Timer));
 	t->timeout_callback = timeout;
+	t->has_expired = false;
 	t->timer_subject = subject;
+	t->time_started = timer_now();
+	t->timeout_delay = delay;
 
-	behind = (timer_now() - next_tick);
-	if (behind > 0.0)
-		delay += behind;
-
-	if (delay < 0.0)
-		ticks = 1;
-	else {
-		ticks =
-		    (delay + TIMER_INTERVAL / 2.0) * (1.0 / TIMER_INTERVAL);
-		if (!ticks)
-			ticks = 1;	/* minimum delay is a tick */
-	}
-
-	delta = ticks / WHEEL_SIZE;
-	t->delta = delta;
-
-	insert((Any_Type) (void *) t, active_timers);
+	if (delay > 0)
+		list_push(active_timers, (Any_Type) (void *) t);
+	else
+		list_push(persistent_timers, (Any_Type) (void *) t);
 
 	if (DBG > 2)
 		fprintf(stderr,
@@ -302,5 +270,5 @@ timer_cancel(struct Timer *t)
 	 * being processed (whose timeout has expired).  
 	 */
 
-	done(t);
+	t->has_expired = true;
 }