On Fri, Dec 08, 2023 at 05:56:20PM -0500, Tom Lane wrote:
> Nathan Bossart writes:
>> Here are a couple more easy micro-optimizations in nearby code. I've split
>> them into individual patches for review, but I'll probably just combine
>> them into one patch before committing.
>
> LGTM
Committe
Nathan Bossart writes:
> Here are a couple more easy micro-optimizations in nearby code. I've split
> them into individual patches for review, but I'll probably just combine
> them into one patch before committing.
LGTM
regards, tom lane
Here are a couple more easy micro-optimizations in nearby code. I've split
them into individual patches for review, but I'll probably just combine
them into one patch before committing.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
>From 79727aab47c7d7cd733ce21c8241051de6c9ae1e M
On Fri, Dec 08, 2023 at 11:51:15AM +0700, John Naylor wrote:
> This is less verbose and still compiles with constants:
>
> use_line_feeds ? strlen(",\n ") : strlen(",");
This one worked on my machine. I've committed the patch with that change.
Thanks everyone for the reviews!
--
Nathan Bossart
On Fri, Dec 8, 2023 at 10:32 AM Nathan Bossart wrote:
>
> On Fri, Dec 08, 2023 at 04:11:52PM +1300, David Rowley wrote:
> > + seplen = use_line_feeds ? sizeof(",\n ") - 1 : sizeof(",") - 1;
> >
> > Most modern compilers will be fine with just:
> >
> > seplen = strlen(sep);
> >
> > I had to go back
Nathan Bossart writes:
> On Thu, Dec 07, 2023 at 10:28:50PM -0500, Tom Lane wrote:
>> Yeah, I thought about that too after sending my message. This version
>> LGTM, although maybe the comment could be slightly more verbose with
>> explicit reference to Inf/NaN as being the cases we need to quote.
On Thu, Dec 07, 2023 at 10:28:50PM -0500, Tom Lane wrote:
> Nathan Bossart writes:
>> I did both of these in v2, although I opted to test that the first
>> character after the optional '-' was a digit instead of testing that it was
>> _not_ an 'I' or 'N'.
>
> Yeah, I thought about that too after
On Fri, Dec 08, 2023 at 04:11:52PM +1300, David Rowley wrote:
> + seplen = use_line_feeds ? sizeof(",\n ") - 1 : sizeof(",") - 1;
>
> Most modern compilers will be fine with just:
>
> seplen = strlen(sep);
>
> I had to go back to clang 3.4.1 and GCC 4.1.2 to see the strlen() call
> with that cod
Nathan Bossart writes:
> I did both of these in v2, although I opted to test that the first
> character after the optional '-' was a digit instead of testing that it was
> _not_ an 'I' or 'N'.
Yeah, I thought about that too after sending my message. This version
LGTM, although maybe the comment
On Thu, Dec 07, 2023 at 07:40:30PM -0500, Tom Lane wrote:
> Hmm ... I think that might not be the way to think about it. What
> I'm wondering is why we need a test as expensive as IsValidJsonNumber
> in the first place, given that we know this is a numeric data type's
> output. ISTM we only need
David Rowley writes:
> I might be neater to get rid of the if condition and have:
> [ calls of appendBinaryStringInfo with len 0 ]
Hmm, if we are trying to micro-optimize, I seriously doubt that
that's an improvement.
regards, tom lane
On Fri, 8 Dec 2023 at 12:13, Nathan Bossart wrote:
> Here's a patch that removes a couple of strlen() calls that showed up
> prominently in perf for a COPY TO (FORMAT json) on 110M integers. On my
> laptop, I see a 20% speedup from ~23.6s to ~18.9s for this test.
+ seplen = use_line_feeds ? size
Jeff Davis writes:
> Nice improvement. The use of (len = ...) in a conditional is slightly
> out of the ordinary, but it makes the conditionals a bit simpler and
> you have a comment, so it's fine with me.
Yeah. It's a little not per project style, but I don't see a nice
way to do it differently
On Thu, 2023-12-07 at 17:12 -0600, Nathan Bossart wrote:
> Here's a patch that removes a couple of strlen() calls that showed up
> prominently in perf for a COPY TO (FORMAT json) on 110M integers. On
> my
> laptop, I see a 20% speedup from ~23.6s to ~18.9s for this test.
Nice improvement. The use
14 matches
Mail list logo
