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

[Pico] Fixed missing calls to start, stop and dispose handles #2772

Merged
merged 2 commits into from
Jul 2, 2023

Conversation

jkronegg
Copy link
Contributor

@jkronegg jkronegg commented Jun 28, 2023

🤔 What's changed?

Corrected the pico-container lifecycle: the start, stop and dispose methods are now called for every test scenarios.

The code is the same as before #2725 (so it's safer in terms of lifecycle), except that the pico container is recycled in the start() method when it already exists.

When a second call to start() is done (e.g. for the 2nd scenario), the container component instances are all in disposed state and the container lifecycle is in disposed state. The recycling operation consists in removing all container component instances (i.e. flushing the cache) and resetting the container lifecycle.

⚡️ What's your motivation?

Fixes #2771

🏷️ What kind of change is this?

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

♻️ Anything particular you want feedback on?

I tested the new version on a project with ~480 test scenarios and saw no performance difference when compared to Cucumber 7.12.0.

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

@codecov
Copy link

codecov bot commented Jun 28, 2023

Codecov Report

Merging #2772 (b479614) into main (45c524d) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##               main    #2772   +/-   ##
=========================================
  Coverage     84.98%   84.99%           
- Complexity     2725     2727    +2     
=========================================
  Files           331      331           
  Lines          9537     9542    +5     
  Branches        914      915    +1     
=========================================
+ Hits           8105     8110    +5     
  Misses         1108     1108           
  Partials        324      324           
Impacted Files Coverage Δ
...in/java/io/cucumber/picocontainer/PicoFactory.java 96.42% <100.00%> (+0.77%) ⬆️

pico.getComponentAdapters()
.forEach(cached -> ((Cached<?>) cached).flush());
}
pico.start();
Copy link
Contributor

Choose a reason for hiding this comment

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

Performance wise, how do the different branches compare? If there still an appreciable performance gain?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, performance is still good. JMH micro-benchmark shows:

Benchmark Mode Cnt Score Error Units
PicoFactoryBenchmark.v7_11_1 thrpt 25 24015,595 ± 1681,477 ops/s
PicoFactoryBenchmark.v7_12_1 thrpt 25 769958,897 ± 65377,491 ops/s
PicoFactoryBenchmark.v7_13 (this PR) thrpt 25 709328,227 ± 12756,281 ops/s

The benchmark consists in instanciating a new PicoFactory ouside the benchmark, then adding 7 classes (containing 183 step defs in total), then start the factory, get one instance and stop the factory:

public class PicoFactoryBenchmark {
  @Benchmark
  public List<Object> v7_11_1(PicoFactoryPlan plan) {
      return benchmark(plan.picoFactory7_11_1);
  }

  @Benchmark
  public List<Object> v7_12_1(PicoFactoryPlan plan) {
      return benchmark(plan.picoFactory7_12_1);
  }
  @Benchmark
  public List<Object> v7_13(PicoFactoryPlan plan) {
      return benchmark(plan.picoFactory7_13);
  }
  private static List<Object> benchmark(ObjectFactory objectFactory) {
      List<Object> instances = new ArrayList<>();
      objectFactory.addClass(A.class);
      objectFactory.addClass(B.class);
      objectFactory.addClass(C.class);
      objectFactory.addClass(D.class);
      objectFactory.addClass(F.class);
      objectFactory.addClass(F.class);
      objectFactory.addClass(G.class);

      objectFactory.start();
      instances.add(objectFactory.getInstance(A.class));
      objectFactory.stop();

      return instances;
  }

}

I think the version from Cucumber 7.12.1 is about 10% faster than the new code for 7.13 because the pico-container is started in the constructor (that is, outside the benchmark). But this code for 7.13 is still 30 times faster than in Cucumber 7.11.1.

@mpkorstanje mpkorstanje changed the title Corrected pico container lifecycle [Pico] Fixed pico container lifecycle Jul 2, 2023
@mpkorstanje mpkorstanje changed the title [Pico] Fixed pico container lifecycle [Pico] ixed missing calls to start, stop and dispose handles Jul 2, 2023
@mpkorstanje mpkorstanje changed the title [Pico] ixed missing calls to start, stop and dispose handles [Pico] Fixed missing calls to start, stop and dispose handles Jul 2, 2023
@mpkorstanje mpkorstanje merged commit 2f6066d into main Jul 2, 2023
9 checks passed
@mpkorstanje mpkorstanje deleted the pico-container-lifecycle-issue branch July 2, 2023 18:08
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.

Pico container lifecycle changes
2 participants