Memoize configuration parameters that are costly to create

[henri-tremblay]

🤔 What's changed?

Remember the value of a configuration parameter to return the same result next time.

⚡️ What's your motivation?

Configuration parameters like Pattern are really costly to create. And they are accessed repetitively for each scenario. In benchmarks generating tests to run without running them, it's more than 50% of the time.

🏷️ What kind of change is this?

• 🐛 Bug fix (non-breaking change which fixes a defect)

If possible, I would like to see it included in the 7.x cycle but didn't know which branch to target.

♻️ Anything particular you want feedback on?

I haven't tested memoization everywhere I used it because there's often more manipulation afterwards. If we go crazy, we could always keep the final returned result instead of the one after the compute. But I think that's a good start.

📋 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]

The CucumberConfiguration class is used when the Cucumber framework is used with the JUnit platform @Suite annotation. The tagFilter and nameFilter methods are run one time per scenario, so yes it makes sense to improve the situation. When profiling two different projects, the CucumberConfiguration takes about 55 ms, but none refers to the tagFilter and nameFilter:

Your use of the memoize method implies that you do a Map lookup. You could get even better performance by just memorizing the value, e.g.:

    @SuppressWarnings("OptionalUsedAsFieldOrParameterType")
    private Optional<Pattern> nameFilterPattern;
    @SuppressWarnings("OptionalAssignedToNull")
    Optional<Pattern> nameFilter() {
        if (nameFilterPattern == null) {
            nameFilterPattern = configurationParameters.get(FILTER_NAME_PROPERTY_NAME, Pattern::compile);
        }
        return nameFilterPattern;
    }

That was on my list of optimizations to add to Cucumber, but I let you do it.
As the nameFilter and tagFilter methods are not on the profiler flame graph, I would be very happy to see some benchmarking that show the 50% improvement.

[mpkorstanje]

If there is a benefit to this then I'd prefer we copy JUnits design where there is CachingJupiterConfiguration and DefaultJupiterConfiguration which both implement the same interface.

[mpkorstanje]

If possible, I would like to see it included in the 7.x cycle but didn't know which branch to target.

I was hoping to limit the breaking changes in v8 to raising the baseline to Java 17 without too many breaking other changes. Would that pose a problem for you?

[jkronegg]

When running Cucumber from IntelliJ directly from the feature file (right click, Run), then the properties are parsed by CucumberPropertiesParser (through Cucumber CLI) and the properties are parsed only once (not one time per scenario).

@mpkorstanje would it be feasible to have the same configuration logic between the junit platform and the Cucumber CLI execution modes ? (that would be probably much more work than just caching)

[mpkorstanje]

@mpkorstanje would it be feasible to have the same configuration logic between the junit platform and the Cucumber CLI execution modes ? (that would be probably much more work than just caching)

No. The CucumberTestEngine supports a different sets of properties and options from the Runtime class.

[henri-tremblay]

I'm trying to give you a benchmark. I had one for JUnit 4. Let's see for 5.

[mars1589]

In plugins(), we now cache the parsed plugin list, then append the publish/no-publish plugin to that list. Since the base list is memoized, if plugins() is called more than once on the same CucumberConfiguration, we can append again to the same list and end up with duplicate plugin entries. So plugins() stops being idempotent.

Could we keep memoization for the expensive parse part, but still return a fresh mutable list per call before adding publish plugin? For example, copy the memoized base list into a new ArrayList and then append publish plugin to that new list.

Also, can we add a regression test that calls plugins() twice on the same config instance (with PLUGIN_PROPERTY_NAME and publish token enabled) and asserts the second call does not increase count or duplicate entries?