> On Sep 19, 2020, at 3:58 PM, John Naylor <john.nay...@2ndquadrant.com> wrote:
>
> On Sat, Sep 19, 2020 at 1:46 PM Mark Dilger
> <mark.dil...@enterprisedb.com> wrote:
>
>> 0002 and 0003 look good to me. I like the way you cleaned up a bit with the
>> unicode_norm_props struct, which makes the code a bit more tidy, and on my
>> compiler under -O2 it does not generate any extra runtime dereferences, as
>> the compiler can see through the struct just fine. My only concern would be
>> if some other compilers might not see through the struct, resulting in a
>> runtime performance cost? I wouldn't even ask, except that qc_hash_lookup
>> is called in a fairly tight loop.
>
> (I assume you mean unicode_norm_info) Yeah, that usage was copied from
> the keyword list code. I believe it was done for the convenience of
> the callers. That is worth something, and so is consistency. That
> said, I'd be curious if there is a measurable impact for some
> platforms.
Right, unicode_norm_info. I'm not sure the convenience of the callers matters
here, since the usage is restricted to just one file, but I also don't have a
problem with the code as you have it.
>> In your commit message for 0001 you mentioned testing on a multiplicity of
>> compilers. Did you do that for 0002 and 0003 as well?
>
> For that, I was simply using godbolt.org to test compiling the
> multiplications down to shift-and-adds. Very widespread, I only
> remember MSVC as not doing it. I'm not sure a few extra cycles would
> have been noticeable here, but it can't hurt to have that guarantee.
I am marking this ready for committer. I didn't object to the whitespace
weirdness in your patch (about which `git apply` grumbles) since you seem to
have done that intentionally. I have no further comments on the performance
issue, since I don't have any other platforms at hand to test it on. Whichever
committer picks this up can decide if the issue matters to them enough to punt
it back for further performance testing.
—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
