Improve glue loading performance

[jkronegg]

🤔 What's the problem you're trying to solve?

The JavaBackend class load the Cucumber glue (e.g. steps, hooks) by checking all annotated methods in all classes that on the classpath or in the packages defined in cucumber.glue property. Depending on the configuration, it can take a lot of time.

If no cucumber.glue property is specified, then the full classpath is scanned (thousands of classes) and some classes may raise NoClassDefFoundException that slow the process. On some projects, I noticed that glue loading is taking about 5 seconds. This can be because the glue package is badly configured or the user put too many things in the configured glue package.

The filtering in MethodScanner can also be improved. Currently, it scans all classes in the glue package, including the inner classes (static, anonymous, lambda). I don't see why we are looking for Cucumber annotations in anonymous classes. These classes are usually used for a single instance and not instanciable in the sense of MethodScanner.isInstanciable.

Well, everything works, but the user may not be aware that it's Cucumber is slow because of misconfiguration.

✨ What's your proposed solution?

In order to improve the situation, I propose to:

• avoid checking the Cucumber annotations on anonymous classes
• notify the user when glue loading can be improved by changing the configuration or the content of the glue package

Avoid checking anonymous classes is easy (modify MethodScanner.isInstanciable by adding && !clazz.isAnonymousClass()). I made a proof-of-concept on that, but I still need to determine what is the performance improvement. I can provide a PR for that.

Notifying the user is feasible, I made a proof-of-concept in JavaBackend:

  @Override
    public void loadGlue(Glue glue, List<URI> gluePaths) {
        GlueAdaptor glueAdaptor = new GlueAdaptor(lookup, glue);

        **final Set<Class<?>> containerClasses = new HashSet<>(); // new
        final Set<Class<?>> glueClasses = new HashSet<>(); // new
        long t0 = System.currentTimeMillis();// new**
        gluePaths.stream()
                .filter(gluePath -> CLASSPATH_SCHEME.equals(gluePath.getScheme()))
                .map(ClasspathSupport::packageName)
                .map(classFinder::scanForClassesInPackage)
                .flatMap(Collection::stream)
                .distinct()
                .forEach(aGlueClass -> {
                    **glueClasses.add(aGlueClass); // new**
                    scan(aGlueClass, (method, annotation) -> {
                        **containerClasses.add(method.getDeclaringClass()); // new**
                        container.addClass(method.getDeclaringClass());
                        glueAdaptor.addDefinition(method, annotation);
                    });
                });

        **// new code to notify the user
        long t1 = System.currentTimeMillis();
        int glueClassCount = glueClasses.size();
        if (glueClassCount > 0) {
            long duration = t1 - t0;
            int containerClassCount = containerClasses.size();
            long expectedGain = duration - duration * containerClassCount / glueClassCount;
            if (expectedGain > 100) {
                System.out.println("Scanning the glue packages took " + duration + " ms for " + glueClassCount
                        + " classes, but only " + containerClassCount
                        + " of them are Cucumber glue items. You could gain about " + expectedGain
                        + " ms by removing the classes from the glue directory that do not contain cucumber glue items (e.g. static/anonymous/lambda inner classes).");
            }
        }**
    }

I can provide a PR for that, but need some advice to replace the System.out.println by something more robust, like passing the DiscoveryIssueReporter to the JavaBackend (the issue reporter is in the Runner.runnerOptions but without a proper interface to access it).

⛏ Have you considered any alternatives or workarounds?

No response

📚 Any additional context?

Relates with #3137

[mpkorstanje]

avoid checking the Cucumber annotations on anonymous classes

Makes sense. Please do provide a PR separate from the user notification.

Notifying the user is feasible, I made a proof-of-concept in JavaBackend:
I can provide a PR for that, but need some advice to replace the System.out.println by something more robust, like passing the DiscoveryIssueReporter to the JavaBackend (the issue reporter is in the Runner.runnerOptions but without a proper interface to access it).

Looks good! For logging you can use the io.cucumber.core.logging.Logger. See existing code for examples. You can't use the DiscoveryIssueReporter because glue discovery happens at run time.

I think it would make sense to add a configuration parameter to disable the warning and one to set the threshold. That way users can turn of the warning if they don't care or can't solve the problem.

[mpkorstanje]

@jkronegg #3150 breaks Cucumber Scala. So any sort of class name filtering would have to be provided as parameter to the classpath scanner.