[Pico] Improved performance

[jkronegg]

🤔 What's changed?

Improved picontainer performance by reusing the library initialization (basically: clear the cache on stop()).

⚡️ What's your motivation?

Solves #2724 .

🏷️ What kind of change is this?

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

♻️ Anything particular you want feedback on?

The issue cannot be resolved in the picocontainer library itself (picocontainer/picocontainer#11) because the library seems not maintained anymore.

📋 Checklist:

• I agree to respect and uphold the Cucumber Community Code of Conduct
• I've changed the behaviour of the code
• Users should know about my change
  • I have added an entry to the "Unreleased" section of the CHANGELOG, linking to this pull request.

[codecov]

Codecov Report

Merging #2725 (b8ca737) into main (5691d30) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head b8ca737 differs from pull request most recent head 681cc8b. Consider uploading reports for the commit 681cc8b to get more accurate results

@@            Coverage Diff            @@
##               main    #2725   +/-   ##
=========================================
  Coverage     84.98%   84.99%           
- Complexity     2723     2724    +1     
=========================================
  Files           330      330           
  Lines          9556     9555    -1     
  Branches        916      915    -1     
=========================================
  Hits           8121     8121           
  Misses         1111     1111           
+ Partials        324      323    -1     

Impacted Files	Coverage Δ
...in/java/io/cucumber/picocontainer/PicoFactory.java	95.65% <100.00%> (+3.98%)	⬆️

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

[mpkorstanje]

@jkronegg I can't find flaw this with MR. But it bothers me that we can simply skip invoking the life cycle methods on pico container and still see the same effect. Could you provide a brief rationale why this works? Or if brief takes up too much time, a slightly longer explanation is fine too.

[jkronegg]

Could you provide a brief rationale why this works?

I tried to use the stop() and dispose() methods, with the hope that it will delete the instances but keep the class metadata: not only the instances are kept, but the the picocontainer stays in "stopped" or "disposed" mode and I was not able to restart it. I think this is because the pico lifecycle manager is only dispatching stop() and dispose() commands, but as they are not implemented, there is no effect. It's only on the container itself (on which the handlers are defined) that an effect can be seen.

As the picocontainer has been created "withCaching", my second trial was to look for a cache eviction method, but I could not find one.

My third approach (in this MR) is to remove the instances held by the Cached pico component adapters, in order to make the adapter think that the instances are not yet created. Fortunately, I found the Cached.flush() method and by chance it removes the instance. The DefaultPicoContainer has several collections that hold the pico adapters, so I empirically use the getComponentAdapters() method because it avoids casting. The method returns a pseudo immutable collection (the collection cannot be modified but the Cached objects are not themselves immutable), which explain why the component adapter cache can be flushed.

Then I ran the PicoFactoryTest and was happy to see that everything was already tested. Then I ran it on my usual project (which has a TestContext data object which is shared between 4-5 class files which define the cucumber steps) : as everything ran correctly, I supposed that the trick works properly.

I must admit : there is a part of empirical/brute force in the approach, I'm no picocontainer expert, and I was also surprised that it worked without much effort (it's only about 1 hour of work)😅.

[mpkorstanje]

Mmh. I suppose because we are not using nested containers and only components, we can get away with skipping the lifecycle for the containers.

But I'm going to let this MR sit until I have some more time to look into Pico Containers features. Pico Container is one of the more common DI containers for Cucumber and has been around for a long time. There is a fair chance it is subject to Hyrum's Law.

[mpkorstanje]

Okay. I think this is mostly fine because

1. All added classes and their constructor dependencies are made into components
2. All components go through their life cycle hooks.
3. The internal state of the PicoFactory before and after each stop/start is the same.

One case where Hyrum might come and bite us is the order in which the components are shut down.Pico Container does this in a specific order:

    private void stopAdapters() {
        for (int i = getOrderedComponentAdapters().size() - 1; 0 <= i; i--) {
            ComponentAdapter<?> adapter = getOrderedComponentAdapters().get(i);
            if (adapter instanceof ComponentLifecycle) {
                ComponentLifecycle<?> componentLifecycle = (ComponentLifecycle<?>)adapter;
                if (componentLifecycle.componentHasLifecycle() && componentLifecycle.isStarted()) {
                    componentLifecycle.stop(DefaultPicoContainer.this);
                }
            }
        }
    }

This order appears to be the reverse order in which components are defined. But because we define from a unordered collection this should be fine there too.

cucumber-jvm/cucumber-picocontainer/src/main/java/io/cucumber/picocontainer/PicoFactory.java

Lines 16 to 34 in b182cf8

	private final Set<Class<?>> classes = new HashSet<>();
	private MutablePicoContainer pico;
	private static boolean isInstantiable(Class<?> clazz) {
	boolean isNonStaticInnerClass = !Modifier.isStatic(clazz.getModifiers()) && clazz.getEnclosingClass() != null;
	return Modifier.isPublic(clazz.getModifiers()) && !Modifier.isAbstract(clazz.getModifiers())
	&& !isNonStaticInnerClass;
	}
	public void start() {
	pico = new PicoBuilder()
	.withCaching()
	.withLifecycle()
	.build();
	for (Class<?> clazz : classes) {
	pico.addComponent(clazz);
	}
	pico.start();
	}