Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix performance regression on big gherkin::Features (#331) #352

Merged
merged 18 commits into from
Jan 7, 2025

Conversation

tyranron
Copy link
Member

@tyranron tyranron commented Jan 6, 2025

Resolves #331

Synopsis

See #331 (comment):

We maintain a large collection of feature files that feed nightly regression tests, the runtime of which has grown significantly recently. These are generally maintained in logically separated feature files, and leverage Scenario Outline tables, sometimes with 100-200 scenarios per feature file.

In experimenting with optimizations on a single tag, I tried splitting a feature file into 4 and observed a massive performance gain. Here is my baseline:

[Summary]
1 feature
257 scenarios (226 passed, 31 failed)
1663 steps (1632 passed, 31 failed)
Tests completed in 266 sec

Here are the same exact tests divided over 4 feature files:

[Summary]
4 features
257 scenarios (226 passed, 31 failed)
1663 steps (1632 passed, 31 failed)
Tests completed in 99 sec

Cause

See #331 (comment):

I've tracked down the cause of this issue. The bigger .feature file is - the bigger gherkin::Feature structure is parsed out of it. Once we have it quite big, the usual Clone operations become quite expensive. We've already used Arc through the codebase for passing them around, but there were still some places doing so unwise. The one I tracked down instanly and fixed in 562e2ba.

However, this is only a part of the problem. Since gherkin types use naive #[derive(Hash, PartialEq)] implementations, they become too expensive for large structures. Frankly, the cargo-flamegraph shows on the repro that <gherkin::Table as Hash>::hash() ate 42% CPU time and <gherkin::Feature as PartialEq>::eq() ate 22% CPU time 😱

test-monolith

Solution

Replace Arc usages on gherkin types with Source, where Source is a transparent wrapper over Arc, implementing PartialEq and Hash based on the pointer value.

impl<T> PartialEq for Source<T> {
    fn eq(&self, other: &Self) -> bool {
        Arc::ptr_eq(&self.0, &other.0)
    }
}

impl<T> Hash for Source<T> {
    fn hash<H: Hasher>(&self, state: &mut H) {
        Arc::as_ptr(&self.0).hash(state);
    }
}

@tyranron tyranron added enhancement Improvement of existing features or bugfix semver::breaking Represents breaking changes k::refactor Refactoring, technical debt elimination and other improvements of existing code base k::performance Related to performance of library labels Jan 6, 2025
@tyranron tyranron added this to the 0.22.0 milestone Jan 6, 2025
@tyranron tyranron self-assigned this Jan 6, 2025
@tyranron tyranron marked this pull request as ready for review January 7, 2025 11:23
@tyranron tyranron merged commit 862df28 into main Jan 7, 2025
43 checks passed
@tyranron tyranron deleted the 331-fix-performance branch January 7, 2025 11:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvement of existing features or bugfix k::performance Related to performance of library k::refactor Refactoring, technical debt elimination and other improvements of existing code base semver::breaking Represents breaking changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Runtimes slow down dramatically proportionally with feature file size
1 participant