Re: Optimization for lower(), upper(), casefold() functions.

Hi Jeff,

12.03.2025 07:05, Jeff Davis wrote:
> On Sun, 2025-03-02 at 23:33 +0300, Alexander Borisov wrote:

[...]

> I have refactored unicode_case.c a bit (v3j-0001) and rebased your v3
> work on top of that (v3j-0002).
>
> The refactoring is so that the optimizations do not need to modify
> convert_case, which is already complex and I'd like to avoid adding
> more to that function. Instead, I created a casemap() function, which
> maps a single chracter, and convert_case() calls that.
>
> I didn't test the refactoring for performance, but it looks as
> optimizable as what was there before.

I like the proposed changes.
Here are some thoughts and small improvements:

I've modified patch v3j-0001 a bit.
Specifically:
1. Added static for casemap() function. Otherwise the compiler could not
optimize the code and the performance dropped significantly.
2. Added a fast path for codepoint < 0x80.

v3j-0002:
In the fast path for codepoints < 0x80, I added a premature return.
This avoided additional insertions, which increased performance.

> A couple questions:
>
> * Is there a reason the fast-path for codepoints < 0x80 is in
> unicode_case.c rather than unicode_case_func.h?

Yes, this is an important optimization, below are benchmarks that
confirm it (I mean the lack of fast path in patch v3j-0001). Also we
share logic (0) case conversion (1) get/find value from table.

> * Is there a reason you defined case_index() as static rather than
> static inline?

There is no reason, but we can make it static inline, but that won't do
anything, the compiler will optimize it anyway. Perhaps for general
beauty it should be made static inline, I don't have a rigid position
here. Benchmark showed that there is no difference in performance.

> * Is there a reason to have a new file unicode_case_func.h rather than
> just add it to unicode_case_table.h?

I was purely based on existing approaches in Postgres, the
Normalization Forms have them separated into different headers. Just
trying to be consistent with existing approaches.

> I'm looking at a few more details, but this is a low-risk change
> because there are exhaustive tests, so I intend to commit something
> like this soon.

Thanks for the patch review!

Regards,
Alexander Borisov