[lm-sensors] [PATCH 3/3] libsensors4: Optimize

[lm-sensors] [PATCH 3/3] libsensors4: Optimize

[Jean Delvare]

Minor optimizations to sensors_feature_get_type():
* We no longer support any single type, so no need to handle this case.
* We can start looking for "_" at offset 3 rather than the beginning
  of the string (the shortest valid prefix is "in0", 3 characters.)

---
 lib/access.c |   23 ++++++++---------------
 1 file changed, 8 insertions(+), 15 deletions(-)

--- lm-sensors-3.orig/lib/access.c	2007-07-16 14:48:55.000000000 +0200
+++ lm-sensors-3/lib/access.c	2007-07-16 14:51:19.000000000 +0200
@@ -522,8 +522,6 @@ struct feature_subtype_match
 struct feature_type_match
 {
 	const char *name;
-	sensors_feature_type type;
-	
 	const struct feature_subtype_match *submatches;
 };
 
@@ -568,10 +566,10 @@ static const struct feature_subtype_matc
 };
 
 static struct feature_type_match matches[] = { 
-	{ "temp%d%c", SENSORS_FEATURE_UNKNOWN, temp_matches },
-	{ "in%d%c", SENSORS_FEATURE_UNKNOWN, in_matches },
-	{ "fan%d%c", SENSORS_FEATURE_UNKNOWN, fan_matches },
-	{ "cpu%d%c", SENSORS_FEATURE_UNKNOWN, cpu_matches },
+	{ "temp%d%c", temp_matches },
+	{ "in%d%c", in_matches },
+	{ "fan%d%c", fan_matches },
+	{ "cpu%d%c", cpu_matches },
 };
 
 /* Return the feature type and channel number based on the feature name */
@@ -585,18 +583,13 @@ sensors_feature_type sensors_feature_get
 		if ((count = sscanf(name, matches[i].name, nr, &c)))
 			break;
 	
-	if (i = ARRAY_SIZE(matches)) /* no match */
-		return SENSORS_FEATURE_UNKNOWN;
-	else if (count = 1) /* single type */
-		return matches[i].type;
-
-	/* assert: count = 2 */
-	if (c != '_')
-		return SENSORS_FEATURE_UNKNOWN;
+	if (i = ARRAY_SIZE(matches) || count != 2 || c != '_')
+		return SENSORS_FEATURE_UNKNOWN;  /* no match */
 
 	submatches = matches[i].submatches;
+	name = strchr(name + 3, '_') + 1;
 	for (i = 0; submatches[i].name != NULL; i++)
-		if (!strcmp(strchr(name, '_') + 1, submatches[i].name))
+		if (!strcmp(name, submatches[i].name))
 			return submatches[i].type;
 	
 	return SENSORS_FEATURE_UNKNOWN;


-- 
Jean Delvare

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

Re: [lm-sensors] [PATCH 3/3] libsensors4: Optimize

[Jean Delvare]

Hi Hans,

On Mon, 16 Jul 2007 15:37:30 +0200, Hans de Goede wrote:
> Jean Delvare wrote:
> > Minor optimizations to sensors_feature_get_type():
> > * We no longer support any single type, so no need to handle this case.
> > * We can start looking for "_" at offset 3 rather than the beginning
> >   of the string (the shortest valid prefix is "in0", 3 characters.)
> 
> Nice cleanup!
> 
> I've checked the whole bunch of patches, and they look okay. Once you've 
> applied them to svn I'll do some testing to see if there are any regressions, 
> but I don't expect any,

Thanks for the review. I've committed the patches now. I hope to have
time for more interface cleanups during the next week-end.

My next victim will probably be sensors_get_ignored(), I see no valid
reason why each application has to care about ignore statements when
libsensors itself could hide the ignored chip features to start with.
I'm just not sure if the features should be skipped when the
application asks for the feature list, or even earlier, not inserted in
the feature list when walking the sysfs device directories.

-- 
Jean Delvare

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

Re: [lm-sensors] [PATCH 3/3] libsensors4: Optimize

[Hans de Goede]

Jean Delvare wrote:
> Hi Hans,
> 
> On Mon, 16 Jul 2007 15:37:30 +0200, Hans de Goede wrote:
>> Jean Delvare wrote:
>>> Minor optimizations to sensors_feature_get_type():
>>> * We no longer support any single type, so no need to handle this case.
>>> * We can start looking for "_" at offset 3 rather than the beginning
>>>   of the string (the shortest valid prefix is "in0", 3 characters.)
>> Nice cleanup!
>>
>> I've checked the whole bunch of patches, and they look okay. Once you've 
>> applied them to svn I'll do some testing to see if there are any regressions, 
>> but I don't expect any,
> 
> Thanks for the review. I've committed the patches now. I hope to have
> time for more interface cleanups during the next week-end.
> 

As promised, I've done some testing of the current svn with these patches 
integrated, everything still seems to work fine.

> My next victim will probably be sensors_get_ignored(), I see no valid
> reason why each application has to care about ignore statements when
> libsensors itself could hide the ignored chip features to start with.
> I'm just not sure if the features should be skipped when the
> application asks for the feature list, or even earlier, not inserted in
> the feature list when walking the sysfs device directories.
> 

Since both the config file loading and the sysfs reading happens only once, 
there is no advantage to delaying the hiding to the moment the applications 
asks for the feature list (If sensors.conf would/could be rescanned at that 
moment, that would be an argument to delay the hiding).

Thus this then purely becomes a question of where can it be implemented the 
cleanest, assuming that the current sensors.conf parsing code skips sections 
for not detected chips, doing this when parsing sysfs creates a chicken and egg 
problem, thus I think it would be best to handle the ignores when the apps asks 
for the feature list.

Regards,

Hans


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

Re: [lm-sensors] [PATCH 3/3] libsensors4: Optimize

[Jean Delvare]

Hi Hans,

On Fri, 20 Jul 2007 07:46:24 +0200, Hans de Goede wrote:
> Jean Delvare wrote:
> > Thanks for the review. I've committed the patches now. I hope to have
> > time for more interface cleanups during the next week-end.
> 
> As promised, I've done some testing of the current svn with these patches 
> integrated, everything still seems to work fine.

Great, thanks.

> > My next victim will probably be sensors_get_ignored(), I see no valid
> > reason why each application has to care about ignore statements when
> > libsensors itself could hide the ignored chip features to start with.
> > I'm just not sure if the features should be skipped when the
> > application asks for the feature list, or even earlier, not inserted in
> > the feature list when walking the sysfs device directories.
> 
> Since both the config file loading and the sysfs reading happens only once, 
> there is no advantage to delaying the hiding to the moment the applications 
> asks for the feature list (If sensors.conf would/could be rescanned at that 
> moment, that would be an argument to delay the hiding).

sensors.conf _can_ be rescanned at any time. It doesn't make sense for
"sensors", but it would make sense for GUI applications. That being
said, we have a single function (sensors_init) to initialize the list of
chips, the list of features, and do the configuration file parsing, so
we still have full control on the order in which we want to do it. So
your point is completely valid nevertheless.

> Thus this then purely becomes a question of where can it be implemented the 
> cleanest, assuming that the current sensors.conf parsing code skips sections 
> for not detected chips, doing this when parsing sysfs creates a chicken and egg 
> problem, thus I think it would be best to handle the ignores when the apps asks 
> for the feature list.

I don't think we skip unused sections of sensors.conf, no. Mark, you
are more familiar with that code than I am, can you please confirm?
This would be a possible optimization, and a valuable one with the
current fat default sensors.conf file we provide. But OTOH I think we
agree that we want to take a different road in the future, with
dedicated configuration files, so I'm not sure it's worth the effort.

If we really want to do this optimization, this wouldn't really be a
chicken-and-egg problem. We can sequence the actions the following way:
* Read the list of chips from sysfs.
* Parse sensors.conf, skipping unused sections.
* For each chip, read the list of features from sysfs, skipping ignored
  ones.
This would mean reorganizing the code a bit, but nothing impossible.

Anyway, I don't really care about the performance at this point, only
about the library API. I'll move the handling of ignore statements
wherever it is easier now, so that applications no longer have to care
about this. We can always optimize later.

Thanks,
-- 
Jean Delvare

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