[Java] Improve cucumber expression creation performance

[jkronegg]

🤔 What's changed?

Improved CucumberExpression.escapeRegex(String) performance as proposed in #200. Calls to escapeRegex are about 7 times faster and the overall performance improvement on CucumberExpression new instance is about 7%.

⚡️ What's your motivation?

Fixes #200

🏷️ What kind of change is this?

• 🏦 Refactoring/debt/DX (improvement to code design, tooling, documentation etc. without changing behaviour)

♻️ Anything particular you want feedback on?

Unit tests have been executed on the previous implementation and proposed implementation to prevent regressions.

📋 Checklist:

• I agree to respect and uphold the Cucumber Community Code of Conduct
• I've changed the behaviour of the code
  • I have added/updated tests to cover my changes.
• My change requires a change to the documentation.
  • I have updated the documentation accordingly.
• Users should know about my change
  • I have added an entry to the "Unreleased" section of the CHANGELOG, linking to this pull request.

[jkronegg]

Hi @mpkorstanje, is anything missing in the PR ? Should I provide more information about the algorithm ?

[mpkorstanje]

No I'm just backlogged for a bit.

[mpkorstanje]

I think that in practice expressions with characters that need to be escaped are rare. Would it make sense to check the entire string for characters that need to be replaced first? It would save an array copy.

[jkronegg]

If we rarely escape in practice, the situation is a bit different. We need to benchmark the variants based on the escaping frequency, not only on pure escaping performance.

I've implemented two other variants, so to summarize:

Variant	Description
escapeRegex0	Pattern (cucumber 7.11.0)
escapeRegex1	append(char) with 1 array allocation and 1 array copy
escapeRegex2	append(String) with 1 array allocation and 1 array copy
escapeRegex3	append(char) with 1 array allocation and 0..1 array copy
escapeRegex4	append(String) with 0..1 array allocation and 0..1 array copy

I benchmarked these variants with JMH for varying escaping frequencies:

The variant escapeRegex4 outperforms the other variants for all escaping frequencies. Depending on escaping frequency, this variant is 1.2 to 3 times faster the cucumber 7.11.0 implementation. The code is a bit more complex, but still understandable, so I think we can use this variant:

public static String escapeRegex4(String text) {
    int length = text.length();
    StringBuilder sb = null;
    int blocStart=0;
    int maxChar = CHAR_TO_ESCAPE.length;
    for (int i = 0; i < length; i++) {
        char currentChar = text.charAt(i);
        if (currentChar < maxChar && CHAR_TO_ESCAPE[currentChar]) {
            if (sb == null) {
                sb = new StringBuilder(length * 2);
            }
            if (i > blocStart) {
                // flush previous block
                sb.append(text, blocStart, i);
            }
            sb.append('\\');
            sb.append(currentChar);
            blocStart=i+1;
        }
    }
    if (sb != null) {
        if (length > blocStart) {
            // flush remaining characters
            sb.append(text, blocStart, length);
        }
        return sb.toString();
    }
    return text;
}

In real conditions, on my project with about ~400 test scenarios runs in about 8.5 seconds. The InteliJ profiler says escapeRegex0 takes 3.37% of the total CPU time (=280 ms), while escapeRegex4 takes 0.97% of the total (=80 ms). That's a 200 ms improvement (2.3%): not a lot on one build (I was hoping for more), but worth the effort when considering the number of builds on all cucumber users.

For completeness, the escapeRegex3 variant is:

public static String escapeRegex3(String text) {
    int length = text.length();
    StringBuilder sb = new StringBuilder(length * 2); // prevent resizes
    int maxChar = CHAR_TO_ESCAPE.length;
    boolean escaped = false;
    for (int i = 0; i < length; i++) {
        char currentChar = text.charAt(i);
        if (currentChar < maxChar && CHAR_TO_ESCAPE[currentChar]) {
            sb.append('\\');
            escaped = true;
        }
        sb.append(currentChar);
    }
    return escaped ? sb.toString() : text;
}

[mpkorstanje]

Looks good, just some nitpicks!

[mpkorstanje]

@jkronegg would you be interested in helping out with cucumber/cucumber-jvm#2542. It will eventually enable us to discover all step definitions once, and because of that build all cucumber expressions once (per language) which should significantly improve performance. Though I can imagine that a larger contribution with a longer commitment isn't quite what you are looking for, so if you'd decline that is perfectly understandable.

[mpkorstanje]

I'll merge and release this soonish.

[jkronegg]

@jkronegg would you be interested in helping out with cucumber/cucumber-jvm#2542. It will eventually enable us to discover all step definitions once, and because of that build all cucumber expressions once (per language) which should significantly improve performance. Though I can imagine that a larger contribution with a longer commitment isn't quite what you are looking for, so if you'd decline that is perfectly understandable.

@mpkorstanje sorry for the long response delay. My employer (https://www.mobiliere.ch/) allows me to work up to 10% of my labor time for the Cucumber framework. I still need to manage the details, but I will be happy to help the Cucumber community.

[mpkorstanje]

Cheers. I'll see if I can write something up to help you get started. I'm unfortunately a bit short on time at the moment.