Reduce pystring usages

[meimchu]

First step of pystring removal by improving StringUtils (#2256)
StringUtils have been updated to include some pystring functionalities. Focused on replacing:

• pystring::split()
• pystring::strip()
• pystring::replace()
• pystring::mul()
• pystring::startswith()
• pystring::endswith()

These usages have also been replaced throughout the modules that use these as well. Only pystring::os usages should be left now.

Additional changes of note:

• Temporarily changed ci_workflow.yml. Additional explanation below.
• Added an additional include path for OpenFX build.

@KevinJW please take a look when you have a moment! Thank you.

[meimchu]

I have temporarily removed a couple of combinations of our CICD Linux tests here to at least allow more tests to run. Since one tests from the OS category can stop the other jobs from running.

There is an on-going discussions in Slack regarding some Linux combinations and the recent aswf/ci-ocio:2023 docker image update. That is not the focus of this PR so I have temporarily removed it as a distraction.

I will not merge this request without a conclusion is reached in that regards.

The initial message from Jean-Francois Panisset:

See my comments in #wg-ci, it does seem like the update of the aswf/ci-ocio:2023 image from 2023.2 to 2023.3 has "changed something", and now the self build of OpenEXR 3.4 (the version from 2025) is breaking since that makes use of C++ 20 features.

Quick workaround would be to pin this build to use the aswf/ci-ocio:2023.2 image which should restore the previous behavior, but more generally it would be interesting to figure out "what does building latest OCIO with VFX 2025 OpenEXR in a VFX 2023 environment" really mean, and is that something we need / want to support.

[KevinJW]

I wonder if something like the following might be 'better'/faster

std::string repeat_fast(const std::string& input, std::size_t n) {
    if (n == 0) {
        return {};
    }
    if (n == 1) {
        return input;
    }
    std::string result;
    const auto input_size = input.size();
    result.reserve(input_size * n);
    result = input;
    std::size_t current = 1;
    while (current * 2 <= n) {
        result += result;
        current *= 2;
    }
    const auto remaining = n - current;
    if (remaining > 0) {
        result += std::string_view(result.data(), input_size * remaining);
    }
    return result;
}

Edited because I nerd sniped myself and ran some performance tests
And again avoid an extra allocation

[KevinJW]

might want to call the function Repeat or Repeat_n rather than Multiply.

Note that reserve() can raise an exception if we would go over std::string::max_size() but likely we would run out of memory before that happens.

[meimchu]

What a wonderful suggestion. I just want to speak briefly on what I've done so far to get your eyes on it.

1. Implemented your function above as Repeat(). Made some small changes for syntax consistency.
2. I attempted to put a max_limit check as you've pointed out prior to reserve(). I chose not to throw an exception but I'm open to suggestions. It is a new behaviour compared to the old pystring::mul.
3. I have replaced the single StringUtils::Multiply() usage found in the code base (which was using pystring::mul() before).
4. I did do a quick benchmark testing between Multiply() and Repeat(). I got a string of 23 chars in length and repeated it 1000 times. The difference is significant and I like the pointer arithmetic in this implementation. I'm happy to remove Multiply() completely in favour of Repeat() if that is the desired decision.

Multiply() Total Time: 388.173 ms
Multiply() Avg Time:   0.0388174 ms/iter

Repeat() Total Time:   4.99192 ms
Repeat() Avg Time:     0.000499192 ms/iter

[KevinJW]

count should possibly be a size_t ? if we really need signed values then C++20 has std::ssize_t or we could use std::ptrdiff_t

[meimchu]

Thank you for this suggestion. I've replaced it with size_t as you've suggested to make it consistent with the other size_t usages. As well, I've removed a signed integer comparison ReplaceInPlace() accordingly.

[KevinJW]

ditto

[meimchu]

More size_t replacement as suggested!

[KevinJW]

and again?

[meimchu]

More size_t replacement as suggested!

[KevinJW]

Ah I didn't mean you needed to detect this case, I don't think you need this. if you remove it then the reserve() call below will deal with it. With this check you could think of it as silently "failing".

[meimchu]

Removed and let it YOLO! When I was testing this, I did mistakenly let it ran and it filled up my swap pretty fast. Also of note is that this is a private function only used in Op's std::string SerializeOpVec(const OpRcPtrVec & ops, int indent=0);. I have reservations about that function's indent parameter as int. As SerializeOpVec is internal and infrequently used, I have switched this to size_t to remove negative integer possibility. If a contributor decided to use Repeat() or SerializeOpVec() with a negative number though, then they're going to have a bad time!

[KevinJW]

I've not benchmarked it but it is possible that under some circumstances we might want to check || str.empty() to catch the case of appending an empty string many times. though the extra test probably slows down the function when n is small.

[meimchu]

I agree with you that doing an early check on str.empty() for early return makes sense. I ran a benchmark on both Multiply() and Repeat() with an empty string input and 10,000 repeats. Both Multiply() and Repeat() will return itself and Repeat() just barely beat out Multiply(). I think at this point, I feel pretty good about just removing Multiply() in favour of Repeat() if you agree with it.

Multiply() Total Time: 0.129125 ms
Multiply() Avg Time:   1.29125e-05 ms/iter

Repeat() Total Time:   0.119125 ms
Repeat() Avg Time:     1.19125e-05 ms/iter