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

[Core] Add faster UUID generator selectable through SPI #2703

Merged
merged 10 commits into from
Apr 6, 2023

Conversation

jkronegg
Copy link
Contributor

@jkronegg jkronegg commented Mar 14, 2023

🤔 What's changed?

The cucumber event bus UUID generator can now be configured through a SPI using a properties from:

  • command-line
  • environment variables
  • system properties
  • cucumber.properties
  • junit-platform.properties
  • @CucumberOptions

Two UUID generators are provided:

An event has a UUID. The UUID generator can be configured using the cucumber.uuid-generator property:

UUID generator Features Performance [Millions UUID/second] Typical usage example
io.cucumber.core.eventbus.RandomUuidGenerator Thread-safe, collision-free, multi-jvm ~1 Reports may be generated on different JVMs at the same time. A typical example would be one suite that tests against Firefox and another against Safari. The exact browser is configured through a property. These are then executed concurrently on different Gitlab runners.
io.cucumber.core.eventbus.IncrementingUuidGenerator Thread-safe, collision-free, single-jvm ~130 Reports are generated on a single JVM

Performance on real projects depends on the size (e.g. on a project with 3095 Given, 445 When, 1177 Then, 445 Scenarios, 48 Rules, the IncrementingUuidGenerator improves the global performance by about 3-5% and the feature file parsing performance of about 37%).

The IncrementingUuidGenerator is a very simple UUID generator based on AtomicLong counters. It is thread-safe and collision-free within a single JVM.

The RandomUuidGenerator is a usual UUID.randomUUID() generator.

⚡️ What's your motivation?

Fixes #2698

🏷️ What kind of change is this?

  • ⚡ New feature (non-breaking change which adds new behaviour)

♻️ Anything particular you want feedback on?

Files of particular interest are:

  • IncrementingUuidGenerator
  • RandomUuidGenerator
  • README.md
  • UuidGeneratorServiceLoader

After this PR, I'll add a description on https://cucumber.io/docs/cucumber/state/?lang=java#the-cucumber-object-factory

📋 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 jkronegg requested a review from mpkorstanje March 14, 2023 07:30
@codecov
Copy link

codecov bot commented Mar 14, 2023

Codecov Report

Merging #2703 (4c4c6a0) into main (c7712fc) will increase coverage by 0.28%.
The diff coverage is 98.01%.

@@             Coverage Diff              @@
##               main    #2703      +/-   ##
============================================
+ Coverage     84.69%   84.98%   +0.28%     
- Complexity     2685     2723      +38     
============================================
  Files           323      330       +7     
  Lines          9456     9556     +100     
  Branches        899      916      +17     
============================================
+ Hits           8009     8121     +112     
+ Misses         1120     1111       -9     
+ Partials        327      324       -3     
Impacted Files Coverage Δ
...c/main/java/io/cucumber/junit/CucumberOptions.java 100.00% <ø> (ø)
...c/main/java/io/cucumber/junit/NoUuidGenerator.java 0.00% <0.00%> (ø)
.../main/java/io/cucumber/testng/CucumberOptions.java 100.00% <ø> (ø)
.../main/java/io/cucumber/testng/NoUuidGenerator.java 0.00% <0.00%> (ø)
...umber/core/eventbus/IncrementingUuidGenerator.java 100.00% <100.00%> (ø)
...io/cucumber/core/eventbus/RandomUuidGenerator.java 100.00% <100.00%> (ø)
.../java/io/cucumber/core/eventbus/UuidGenerator.java 100.00% <100.00%> (ø)
...ucumber/core/options/CommandlineOptionsParser.java 81.81% <100.00%> (+0.31%) ⬆️
.../core/options/CucumberOptionsAnnotationParser.java 85.10% <100.00%> (+0.66%) ⬆️
...ucumber/core/options/CucumberPropertiesParser.java 96.10% <100.00%> (+0.10%) ⬆️
... and 8 more

... and 1 file with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@jkronegg
Copy link
Contributor Author

How to solve the Semver build error? Should I just change the version to 7.12.1-SNAPSHOT ? Most probably it is caused by

[ERROR] java.method.addedToInterface: method java.lang.Class<? extends io.cucumber.core.eventbus.UuidGenerator> io.cucumber.core.options.CucumberOptionsAnnotationParser.CucumberOptions::uuidGenerator(): Method was added to an interface. https://revapi.org/revapi-java/differences.html#java.method.addedToInterface

Also I saw that the test coverage is not so good, I'll work on that...

@mpkorstanje
Copy link
Contributor

Incrementing the minor version should make the semver check pass. You can use mvn versions:set for this.

Copy link
Contributor

@mpkorstanje mpkorstanje left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't look at everything yet but these stood out immediately.

@jkronegg
Copy link
Contributor Author

Incrementing to 7.12.0-SNAPSHOT solved some semver issues, but I still have:

[ERROR] Failed to execute goal org.revapi:revapi-maven-plugin:0.15.0:check (check) on project cucumber-core: The following API problems caused the build to fail:
[ERROR] java.method.addedToInterface: method java.lang.Class<? extends io.cucumber.core.eventbus.UuidGenerator> io.cucumber.core.options.CucumberOptionsAnnotationParser.CucumberOptions::uuidGenerator(): Method was added to an interface. https://revapi.org/revapi-java/differences.html#java.method.addedToInterface
[ERROR] java.method.addedToInterface: method java.lang.Class<? extends io.cucumber.core.eventbus.UuidGenerator> io.cucumber.core.runner.Options::getUuidGeneratorClass(): Method was added to an interface. https://revapi.org/revapi-java/differences.html#java.method.addedToInterface
[ERROR] 

As the CucumberOptionsAnnotationParser and the Options are internal Cucumber classes, is it a false positive ?

@mpkorstanje
Copy link
Contributor

We can consider them false positives. You can add it to the internal section in this file:

https://github.com/cucumber/cucumber-jvm/blob/main/.revapi/api-changes.json

@jkronegg
Copy link
Contributor Author

jkronegg commented Mar 17, 2023

@mpkorstanje I've finished the work I planned to do on this PR. Do you see anything to change ?

p.s. : I added documentation on cucumber/docs#850

Copy link
Contributor

@mpkorstanje mpkorstanje left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed this from my phone. Looks good overal. A few nitpicks and some minor stuff. I didn't see anything major.

I also left a few items for myself, please leave those open.


@Override
public UUID get() {
return null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to self: Write an issue with the considerations of using a null-safe API, returning null only on methods annotated with nullable. Specifically investigate automated checks.

Julien Kronegg and others added 7 commits March 22, 2023 11:53
# Conflicts:
#	CHANGELOG.md
#	cucumber-core/src/main/java/io/cucumber/core/eventbus/IncrementingUuidGenerator.java
#	cucumber-core/src/main/java/io/cucumber/core/eventbus/RandomUuidGenerator.java
#	cucumber-core/src/main/java/io/cucumber/core/eventbus/UuidGenerator.java
#	cucumber-core/src/test/java/io/cucumber/core/eventbus/IncrementingUuidGeneratorTest.java
#	cucumber-core/src/test/java/io/cucumber/core/eventbus/RandomUuidGeneratorTest.java
#	cucumber-core/src/test/java/io/cucumber/core/options/NoUuidGenerator.java
#	cucumber-core/src/test/java/io/cucumber/core/runtime/UuidGeneratorServiceLoaderTest.java
#	cucumber-junit/src/main/java/io/cucumber/junit/NoUuidGenerator.java
#	cucumber-testng/src/main/java/io/cucumber/testng/NoUuidGenerator.java
#	pom.xml
@jkronegg jkronegg requested a review from mpkorstanje March 28, 2023 14:25
Copy link
Contributor

@mpkorstanje mpkorstanje left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. I'll take care of the last few comments and merge this tomorrow in all likelihood.

@mpkorstanje mpkorstanje changed the title Improved event bus performance using UUID generator selectable through SPI [Core] Add faster UUID generator selectable through SPI Apr 6, 2023
@mpkorstanje mpkorstanje merged commit 5691d30 into main Apr 6, 2023
@mpkorstanje mpkorstanje deleted the uuid-generator branch April 6, 2023 19:18
mpkorstanje added a commit that referenced this pull request Sep 21, 2024
With #2703 a faster UUID generator was introduced. And while the
configuration options were added, they were not actually used by
`cucumber-junit`, `cucumber-junit-platform-engine` and
`cucumber-testng`.
mpkorstanje added a commit that referenced this pull request Sep 21, 2024
With #2703 a faster UUID generator was introduced. And while the
configuration options were added, they were not actually used by
`cucumber-junit`, `cucumber-junit-platform-engine` and
`cucumber-testng`.
mpkorstanje added a commit that referenced this pull request Sep 21, 2024
With #2703 a faster UUID generator was introduced. And while the
configuration options were added, they were not actually used by
`cucumber-junit`, `cucumber-junit-platform-engine` and
`cucumber-testng`.
mpkorstanje added a commit that referenced this pull request Sep 26, 2024
With #2703 a faster UUID generator was introduced. And while the
configuration options were added, they were not actually used by
`cucumber-junit`, `cucumber-junit-platform-engine` and
`cucumber-testng`.
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.

Improve feature loading performance by changing UUID generator
2 participants