Thread: Faster str to int conversion (was Table with large number of intcolumns, very slow COPY FROM)

Hi,


On 2017-12-08 10:17:34 -0800, Andres Freund wrote:
> the strtoll is libc functionality triggered by pg_atoi(), something I've
> seen show up in numerous profiles. I think it's probably time to have
> our own optimized version of it rather than relying on libcs.

Attached is a hand-rolled version. After quickly hacking up one from
scratch, I noticed we already kind of have one for int64 (scanint8), so
I changed the structure of this one to be relatively similar.

It's currently using the overflow logic from [1], but that's not
fundamentally required, we could rely on fwrapv for this one too.

This one improves performance of the submitted workload from 1223.950ms
to 1020.640ms (best of three). The profile's shape changes quite
noticeably:

master:
+   15.38%  postgres  libc-2.25.so      [.] __GI_____strtoll_l_internal
+   11.79%  postgres  postgres          [.] heap_fill_tuple
+    8.00%  postgres  postgres          [.] CopyFrom
+    7.40%  postgres  postgres          [.] CopyReadLine
+    6.79%  postgres  postgres          [.] ExecConstraints
+    6.68%  postgres  postgres          [.] NextCopyFromRawFields
+    6.36%  postgres  postgres          [.] heap_compute_data_size
+    6.02%  postgres  postgres          [.] pg_atoi
patch:
+   13.70%  postgres  postgres          [.] heap_fill_tuple
+   10.46%  postgres  postgres          [.] CopyFrom
+    9.31%  postgres  postgres          [.] pg_strto32
+    8.39%  postgres  postgres          [.] CopyReadLine
+    7.88%  postgres  postgres          [.] ExecConstraints
+    7.63%  postgres  postgres          [.] InputFunctionCall
+    7.41%  postgres  postgres          [.] heap_compute_data_size
+    7.21%  postgres  postgres          [.] pg_verify_mbstr
+    5.49%  postgres  postgres          [.] NextCopyFromRawFields


This probably isn't going to resolve Alex's performance concerns
meaningfully, but seems quite worthwhile to do anyway.

We probably should have int8/16/64 version coded just as use the 32bit
version, but I decided to leave that out for now. Primarily interested
in comments.  Wonder a bit whether it's worth providing an 'errorOk'
mode like scanint8 does, but surveying its callers suggests we should
rather change them to not need it...

Greetings,

Andres Freund

[1] http://archives.postgresql.org/message-id/20171030112751.mukkriz2rur2qkxc%40alap3.anarazel.de

Hi,

On 2017-12-08 13:44:37 -0800, Andres Freund wrote:
> On 2017-12-08 10:17:34 -0800, Andres Freund wrote:
> > the strtoll is libc functionality triggered by pg_atoi(), something I've
> > seen show up in numerous profiles. I think it's probably time to have
> > our own optimized version of it rather than relying on libcs.
> 
> Attached is a hand-rolled version. After quickly hacking up one from
> scratch, I noticed we already kind of have one for int64 (scanint8), so
> I changed the structure of this one to be relatively similar.
> 
> It's currently using the overflow logic from [1], but that's not
> fundamentally required, we could rely on fwrapv for this one too.
> 
> This one improves performance of the submitted workload from 1223.950ms
> to 1020.640ms (best of three). The profile's shape changes quite
> noticeably:

FWIW, here's a rebased version of this patch. Could probably be polished
further. One might argue that we should do a bit more wide ranging
changes, to convert scanint8 and pg_atoi to be also unified. But it
might also just be worthwhile to apply without those, given the
performance benefit.

Anybody have an opinion on that?

Greetings,

Andres Freund

Hi,

On 2017-12-08 13:44:37 -0800, Andres Freund wrote:
> On 2017-12-08 10:17:34 -0800, Andres Freund wrote:
> > the strtoll is libc functionality triggered by pg_atoi(), something I've
> > seen show up in numerous profiles. I think it's probably time to have
> > our own optimized version of it rather than relying on libcs.
> 
> Attached is a hand-rolled version. After quickly hacking up one from
> scratch, I noticed we already kind of have one for int64 (scanint8), so
> I changed the structure of this one to be relatively similar.
> 
> It's currently using the overflow logic from [1], but that's not
> fundamentally required, we could rely on fwrapv for this one too.
> 
> This one improves performance of the submitted workload from 1223.950ms
> to 1020.640ms (best of three). The profile's shape changes quite
> noticeably:

FWIW, here's a rebased version of this patch. Could probably be polished
further. One might argue that we should do a bit more wide ranging
changes, to convert scanint8 and pg_atoi to be also unified. But it
might also just be worthwhile to apply without those, given the
performance benefit.

Anybody have an opinion on that?

Greetings,

Andres Freund

On Sat, Jul 7, 2018 at 4:01 PM, Andres Freund <andres@anarazel.de> wrote:
> FWIW, here's a rebased version of this patch. Could probably be polished
> further. One might argue that we should do a bit more wide ranging
> changes, to convert scanint8 and pg_atoi to be also unified. But it
> might also just be worthwhile to apply without those, given the
> performance benefit.

Wouldn't hurt to do that one too, but might be OK to just do this
much.  Questions:

1. Why the error message changes?  If there's a good reason, it should
be done as a separate commit, or at least well-documented in the
commit message.

2. Does the likely/unlikely stuff make a noticeable difference?

3. If this is a drop-in replacement for pg_atoi, why not just recode
pg_atoi this way -- or have it call this -- and leave the callers
unchanged?

4. Are we sure this is faster on all platforms, or could it work out
the other way on, say, BSD?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Sat, Jul 7, 2018 at 4:01 PM, Andres Freund <andres@anarazel.de> wrote:
> FWIW, here's a rebased version of this patch. Could probably be polished
> further. One might argue that we should do a bit more wide ranging
> changes, to convert scanint8 and pg_atoi to be also unified. But it
> might also just be worthwhile to apply without those, given the
> performance benefit.

Wouldn't hurt to do that one too, but might be OK to just do this
much.  Questions:

1. Why the error message changes?  If there's a good reason, it should
be done as a separate commit, or at least well-documented in the
commit message.

2. Does the likely/unlikely stuff make a noticeable difference?

3. If this is a drop-in replacement for pg_atoi, why not just recode
pg_atoi this way -- or have it call this -- and leave the callers
unchanged?

4. Are we sure this is faster on all platforms, or could it work out
the other way on, say, BSD?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Hi,

On 2018-07-18 14:34:34 -0400, Robert Haas wrote:
> On Sat, Jul 7, 2018 at 4:01 PM, Andres Freund <andres@anarazel.de> wrote:
> > FWIW, here's a rebased version of this patch. Could probably be polished
> > further. One might argue that we should do a bit more wide ranging
> > changes, to convert scanint8 and pg_atoi to be also unified. But it
> > might also just be worthwhile to apply without those, given the
> > performance benefit.
> 
> Wouldn't hurt to do that one too, but might be OK to just do this
> much.  Questions:
> 
> 1. Why the error message changes?  If there's a good reason, it should
> be done as a separate commit, or at least well-documented in the
> commit message.

Because there's a lot of "invalid input syntax for type %s: \"%s\"",
error messages, and we shouldn't force translators to have separate
version that inlines the first %s.  But you're right, it'd be worthwhile
to point that out in the commit message.


> 2. Does the likely/unlikely stuff make a noticeable difference?

Yes. It's also largely a copy from existing code (scanint8), so I don't
really want to differ here.


> 3. If this is a drop-in replacement for pg_atoi, why not just recode
> pg_atoi this way -- or have it call this -- and leave the callers
> unchanged?

Because pg_atoi supports a variable 'terminator'. Supporting that would
create a bit slower code, without being particularly useful.  I think
there's only a single in-core caller left after the patch
(int2vectorin). There's a fair argument that that should just be
open-coded to handle the weird space parsing, but given there's probably
external pg_atoi() callers, I'm not sure it's worth doing so?

I don't think it's a good idea to continue to have pg_atoi as a wrapper
- it takes a size argument, which makes efficient code hard.


> 4. Are we sure this is faster on all platforms, or could it work out
> the other way on, say, BSD?

I'd be *VERY* surprised if any would be faster. It's not easy to write a
faster implmentation, than what I've proposed, and especially not so if
you use strtol() as the API (variable bases, a bit of locale support).

Greetings,

Andres Freund

Hi,

On 2018-07-18 14:34:34 -0400, Robert Haas wrote:
> On Sat, Jul 7, 2018 at 4:01 PM, Andres Freund <andres@anarazel.de> wrote:
> > FWIW, here's a rebased version of this patch. Could probably be polished
> > further. One might argue that we should do a bit more wide ranging
> > changes, to convert scanint8 and pg_atoi to be also unified. But it
> > might also just be worthwhile to apply without those, given the
> > performance benefit.
> 
> Wouldn't hurt to do that one too, but might be OK to just do this
> much.  Questions:
> 
> 1. Why the error message changes?  If there's a good reason, it should
> be done as a separate commit, or at least well-documented in the
> commit message.

Because there's a lot of "invalid input syntax for type %s: \"%s\"",
error messages, and we shouldn't force translators to have separate
version that inlines the first %s.  But you're right, it'd be worthwhile
to point that out in the commit message.


> 2. Does the likely/unlikely stuff make a noticeable difference?

Yes. It's also largely a copy from existing code (scanint8), so I don't
really want to differ here.


> 3. If this is a drop-in replacement for pg_atoi, why not just recode
> pg_atoi this way -- or have it call this -- and leave the callers
> unchanged?

Because pg_atoi supports a variable 'terminator'. Supporting that would
create a bit slower code, without being particularly useful.  I think
there's only a single in-core caller left after the patch
(int2vectorin). There's a fair argument that that should just be
open-coded to handle the weird space parsing, but given there's probably
external pg_atoi() callers, I'm not sure it's worth doing so?

I don't think it's a good idea to continue to have pg_atoi as a wrapper
- it takes a size argument, which makes efficient code hard.


> 4. Are we sure this is faster on all platforms, or could it work out
> the other way on, say, BSD?

I'd be *VERY* surprised if any would be faster. It's not easy to write a
faster implmentation, than what I've proposed, and especially not so if
you use strtol() as the API (variable bases, a bit of locale support).

Greetings,

Andres Freund

On Thu, Jul 19, 2018 at 4:32 PM, Andres Freund <andres@anarazel.de> wrote:
>> 1. Why the error message changes?  If there's a good reason, it should
>> be done as a separate commit, or at least well-documented in the
>> commit message.
>
> Because there's a lot of "invalid input syntax for type %s: \"%s\"",
> error messages, and we shouldn't force translators to have separate
> version that inlines the first %s.  But you're right, it'd be worthwhile
> to point that out in the commit message.

It just seems weird that they're bundled together in one commit like this.

>> 2. Does the likely/unlikely stuff make a noticeable difference?
>
> Yes. It's also largely a copy from existing code (scanint8), so I don't
> really want to differ here.

OK.

>> 3. If this is a drop-in replacement for pg_atoi, why not just recode
>> pg_atoi this way -- or have it call this -- and leave the callers
>> unchanged?
>
> Because pg_atoi supports a variable 'terminator'.

OK.

>> 4. Are we sure this is faster on all platforms, or could it work out
>> the other way on, say, BSD?
>
> I'd be *VERY* surprised if any would be faster. It's not easy to write a
> faster implmentation, than what I've proposed, and especially not so if
> you use strtol() as the API (variable bases, a bit of locale support).

OK.

Nothing else from me...

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Thu, Jul 19, 2018 at 4:32 PM, Andres Freund <andres@anarazel.de> wrote:
>> 1. Why the error message changes?  If there's a good reason, it should
>> be done as a separate commit, or at least well-documented in the
>> commit message.
>
> Because there's a lot of "invalid input syntax for type %s: \"%s\"",
> error messages, and we shouldn't force translators to have separate
> version that inlines the first %s.  But you're right, it'd be worthwhile
> to point that out in the commit message.

It just seems weird that they're bundled together in one commit like this.

>> 2. Does the likely/unlikely stuff make a noticeable difference?
>
> Yes. It's also largely a copy from existing code (scanint8), so I don't
> really want to differ here.

OK.

>> 3. If this is a drop-in replacement for pg_atoi, why not just recode
>> pg_atoi this way -- or have it call this -- and leave the callers
>> unchanged?
>
> Because pg_atoi supports a variable 'terminator'.

OK.

>> 4. Are we sure this is faster on all platforms, or could it work out
>> the other way on, say, BSD?
>
> I'd be *VERY* surprised if any would be faster. It's not easy to write a
> faster implmentation, than what I've proposed, and especially not so if
> you use strtol() as the API (variable bases, a bit of locale support).

OK.

Nothing else from me...

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Hi,

On 2018-07-20 08:27:34 -0400, Robert Haas wrote:
> On Thu, Jul 19, 2018 at 4:32 PM, Andres Freund <andres@anarazel.de> wrote:
> >> 1. Why the error message changes?  If there's a good reason, it should
> >> be done as a separate commit, or at least well-documented in the
> >> commit message.
> >
> > Because there's a lot of "invalid input syntax for type %s: \"%s\"",
> > error messages, and we shouldn't force translators to have separate
> > version that inlines the first %s.  But you're right, it'd be worthwhile
> > to point that out in the commit message.
> 
> It just seems weird that they're bundled together in one commit like this.

I'll push it separately.

> Nothing else from me...

Thanks for looking!

Greetings,

Andres Freund

Hi,

On 2018-07-20 08:27:34 -0400, Robert Haas wrote:
> On Thu, Jul 19, 2018 at 4:32 PM, Andres Freund <andres@anarazel.de> wrote:
> >> 1. Why the error message changes?  If there's a good reason, it should
> >> be done as a separate commit, or at least well-documented in the
> >> commit message.
> >
> > Because there's a lot of "invalid input syntax for type %s: \"%s\"",
> > error messages, and we shouldn't force translators to have separate
> > version that inlines the first %s.  But you're right, it'd be worthwhile
> > to point that out in the commit message.
> 
> It just seems weird that they're bundled together in one commit like this.

I'll push it separately.

> Nothing else from me...

Thanks for looking!

Greetings,

Andres Freund