[lm-sensors] Do we need regex in libsensors?

[lm-sensors] Do we need regex in libsensors?

[Jean Delvare]

Hi all, hi Hans,

I've made a performance regression test on the new libsensors. It shows
a significant performance drop. Valgrind showed that almost half of the
total CPU cycles were spent in sensors_feature_get_type(), and in
particular in regexec(). Looking at what the code does, I don't think
that we need the power of the regex engine; sscanf() should be enough.
I've reimplemented the function using sscanf(), it seems to work just
fine, and is much faster (although not yet as fast as the original, but
I guess this is expected due to the dynamic feature gathering.)

Index: lib/access.c
=================================--- lib/access.c	(révision 4520)
+++ lib/access.c	(copie de travail)
@@ -20,7 +20,6 @@
 #include <stdlib.h>
 #include <string.h>
 #include <math.h>
-#include <regex.h>
 #include "access.h"
 #include "sensors.h"
 #include "data.h"
@@ -28,8 +27,6 @@
 #include "proc.h"
 #include "general.h"
 
-#define GET_TYPE_REGEX "\\([[:alpha:]]\\{1,\\}\\)[[:digit:]]\\{0,\\}\\(_\\([[:alpha:]]\\{1,\\}\\)\\)\\{0,1\\}"
-
 static int sensors_do_this_chip_sets(sensors_chip_name name);
 
 /* Compare two chips name descriptions, to see whether they could match.
@@ -541,11 +538,10 @@
 };
 
 static struct feature_type_match matches[] = { 
-	{ "temp", SENSORS_FEATURE_TEMP, temp_matches },
-	{ "in", SENSORS_FEATURE_IN, in_matches },
-	{ "fan", SENSORS_FEATURE_FAN, fan_matches },
-	{ "cpu", SENSORS_FEATURE_UNKNOWN, cpu_matches },
-	{ "vrm", SENSORS_FEATURE_VRM, 0 },
+	{ "temp%d%c", SENSORS_FEATURE_TEMP, temp_matches },
+	{ "in%d%c", SENSORS_FEATURE_IN, in_matches },
+	{ "fan%d%c", SENSORS_FEATURE_FAN, fan_matches },
+	{ "cpu%d%c", SENSORS_FEATURE_UNKNOWN, cpu_matches },
 	{ 0 }
 };
 
@@ -553,39 +549,30 @@
 sensors_feature_type sensors_feature_get_type(
 	const sensors_feature_data *feature)
 {
-	regmatch_t pmatch[4];
-	int size_first, size_second, retval, i;
+	char c;
+	int i, nr, count;
 	struct feature_type_match *submatches;
-	static regex_t reg;
-	static regex_t *preg = NULL;
 	
-	if (!preg) {
-		regcomp(&reg, GET_TYPE_REGEX, 0);
-		preg = &reg;
-	}
+	/* vrm is special */
+	if (!strcmp(feature->name, "vrm"))
+		return SENSORS_FEATURE_VRM;
 	
-	retval = regexec(preg, feature->name, 4, pmatch, 0);
-	
-	if (retval = -1)
-		return SENSORS_FEATURE_UNKNOWN;
-	
-	size_first = pmatch[1].rm_eo - pmatch[1].rm_so;
-	size_second = pmatch[3].rm_eo - pmatch[3].rm_so;
-	
 	for(i = 0; matches[i].name != 0; i++)
-		if (!strncmp(feature->name, matches[i].name, size_first))
+		if ((count = sscanf(feature->name, matches[i].name, &nr, &c)))
 			break;
 	
 	if (matches[i].name = NULL) /* no match */
 		return SENSORS_FEATURE_UNKNOWN;
-	else if (size_second = 0) /* single type */
+	else if (count = 1) /* single type */
 		return matches[i].type;
-	else if (matches[i].submatches = NULL) /* not single type, but no submatches */
+
+	/* assert: count = 2 */
+	if (c != '_')
 		return SENSORS_FEATURE_UNKNOWN;
 
 	submatches = matches[i].submatches;
 	for(i = 0; submatches[i].name != 0; i++)
-		if (!strcmp(feature->name + pmatch[3].rm_so, submatches[i].name))
+		if (!strcmp(strchr(feature->name, '_') + 1, submatches[i].name))
 			return submatches[i].type;
 	
 	return SENSORS_FEATURE_UNKNOWN;

Is it OK to apply this, or does anyone have a good reason to believe
that sscanf() won't be sufficient in some (present or future) case?

Thanks,
-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] Do we need regex in libsensors?

[Hans de Goede]

Jean Delvare wrote:
> Hi all, hi Hans,
> 
> I've made a performance regression test on the new libsensors. It shows
> a significant performance drop. Valgrind showed that almost half of the
> total CPU cycles were spent in sensors_feature_get_type(), and in
> particular in regexec(). Looking at what the code does, I don't think
> that we need the power of the regex engine; sscanf() should be enough.
> I've reimplemented the function using sscanf(), it seems to work just
> fine, and is much faster (although not yet as fast as the original, but
> I guess this is expected due to the dynamic feature gathering.)
> 

<snip>

> Is it OK to apply this, or does anyone have a good reason to believe
> that sscanf() won't be sufficient in some (present or future) case?
> 

The patch looks fine to me. I applied it over here and did some testing, I 
couldn't find any issues. So as far as I'm concerned its ok to apply.


Regards,

Hans

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] Do we need regex in libsensors?

[Jean Delvare]

On Fri, 29 Jun 2007 20:25:38 +0200, Hans de Goede wrote:
> Jean Delvare wrote:
> > I've made a performance regression test on the new libsensors. It shows
> > a significant performance drop. Valgrind showed that almost half of the
> > total CPU cycles were spent in sensors_feature_get_type(), and in
> > particular in regexec(). Looking at what the code does, I don't think
> > that we need the power of the regex engine; sscanf() should be enough.
> > I've reimplemented the function using sscanf(), it seems to work just
> > fine, and is much faster (although not yet as fast as the original, but
> > I guess this is expected due to the dynamic feature gathering.)
> (...)
> 
> The patch looks fine to me. I applied it over here and did some testing, I 
> couldn't find any issues. So as far as I'm concerned its ok to apply.

OK, done. Thanks for testing.

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors