Skip to content

Conversation

jkronegg
Copy link
Contributor

@jkronegg jkronegg commented Dec 6, 2022

🤔 What's changed?

I rewrote a regular expression to improve CucumberExpression creation performance.

⚡️ What's your motivation?

My motivation is to have a better performance for the Cucumber framework.

I made a benchmark with JMH which shows that the modified method is about 15% faster (see escapeRegex2 below) :

Benchmark Mode Cnt Score Error Units
CucumberExpressionBenchmark.escapeRegex0 thrpt 25 726563,091 ± 15558,020 ops/s
CucumberExpressionBenchmark.escapeRegex2 thrpt 25 833512,235 ± 25088,968 ops/s

🏷️ 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?

Nothing.

📋 Checklist:

I made a benchmark with JMH which shows that the modified method is about 15% faster (see escapeRegex2 below) :

Benchmark                                  Mode  Cnt       Score       Error  Units
CucumberExpressionBenchmark.escapeRegex0  thrpt   25  726563,091 ± 15558,020  ops/s
CucumberExpressionBenchmark.escapeRegex1  thrpt   25  782046,026 ± 10993,804  ops/s
CucumberExpressionBenchmark.escapeRegex2  thrpt   25  833512,235 ± 25088,968  ops/s

The `CucumberExpression` class looks extensively tested, thus I didn't added more test cases.

Note: for completeness, I tested another variant (escapeRegex1) which replaces the `ESCAPE_PATTERN.matcher(text).replaceAll(String)` with multiple `String.replace(String,String)` : the performance is not that good (only 8% faster than the original method) and code readability/maintenance is bad. But it was interesting to have the numbers to compare...
@mpkorstanje mpkorstanje self-assigned this Dec 6, 2022
@mpkorstanje
Copy link
Contributor

Merged with 2ab6bad

@mpkorstanje mpkorstanje closed this Dec 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants