On Wed, Sep 04, 2019 at 02:08:39AM -0700, Andres Freund wrote:
>> +static bool
>> +str2int64(const char *str, int64 *val)
>> +{
>> + pg_strtoint_status stat = pg_strtoint64(str, val);
>> +
>
> I find it weird to have a wrapper that's named 'str2...' that then calls
> 'strto' to implement itself.
It happens that this wrapper in pgbench.c is not actually needed.
> Hm. If we're concerned about the cost of isdigit etc - and I think
> that's reasonable, after looking at their implementation in glibc (it
> performs a lookup in a global table to handle potential locale changes)
> - I think we ought to just not use the provided isdigit, and probably
> not isspace either. This code effectively relies on the input being
> ascii anyway, so we don't need any locale specific behaviour afaict
> (we'd e.g. get wrong results if isdigit() returned true for something
> other than the ascii chars).
Yeah. It seems to me that we have more optimizations that could come
in line here, and actually we have perhaps more refactoring at hand
with each one of the 6 functions we'd like to add at the end. I had
in mind about first shaping the refactoring patch, consolidating all
the interfaces, and then evaluate the improvements we can come up with
as after the refactoring we'd need to update only common/string.c.
> I've not benchmarked that, but I'd be surprised if it didn't improve
> matters.
I think that you are right here, there is something to gain. Looking
at their stuff this makes use of __isctype as told by ctype/ctype.h.
--
Michael