[lprimak]

Fixes #3582.

Weld checked each class by asking small questions one at a time, and every question walked through the class again. Now the class is walked once and the questions are answered from that result. The results are the same.

Used in two places:

• RequiredAnnotationDiscovery: the annotation types found in a class (plus those on its annotations, one level deep, as before) are kept in a set per class, instead of searching the class again for each annotation of each @WithAnnotations observer. The set is dropped in cleanupAfterBoot().
• EnhancedAnnotatedTypeImpl#getOverriddenMethods: methods are grouped by declaring class once, in the same order as before, instead of going through all methods for every class in the hierarchy.

In a Payara 7 redeploy of a PrimeFaces application this took the time from about 550 ms to about 320-345 ms (details in #3582).

[manovotn]

I did leave some comments and I am still looking into this but there are few bigger areas that we should clear up first.

Firstly, Weld has a small repo with some benchmarks, it might be worth adding something there and measure it as that would be stripped of any influence of a whole EE server - https://github.com/weld/weld-core-benchmarks. While I appreciate your findings, I know from my own experience with WFLY that the server overhead can significantly temper with results

• We should also consider measuring how much more expensive your solution gets for some simple cases. While it improves large apps, I guess it will actually hinder smaller deployments by searching the whole deployment where previously we could get a quick answer from top level class.

Secondly, many of the tests you have here overlap with what Weld already has. I think it might be smarter to instead expand on existing test coverage (or add tests into those places).

• For annotation discovery, there is WithAnnotationsTest (note that there is SE variant and another in tests-arquillian) which I think should be compared to your coverage and extended.
• For overriden methods, this is a bit more convoluted - here I agree your test is probably better provided you remove the old algorithm and stick to asserting expected results.

Lastly, there is one case that your approach will fail at - current code uses a short-circuit approach where it returns the first possible result and hence can bypass what would otherwise result in an error. Here's the code:

class Base {
    public Base() {}
    private Base(Missing optionalDependency) {}
}

@Dependent
@Marker
class Child extends Base {}

public class VetoChildExtension implements Extension {
    void vetoChild(
            @Observes @WithAnnotations(Marker.class)
            ProcessAnnotatedType<Child> event) {
        event.veto();
    }
}

Missing is available during compilation but absent at runtime.
In current Weld code, this works and deploys whereas in your case, it will blow up as you attempt to read the whole complete picture and encounter Missing in there.

[manovotn]

@lprimak actually, seeing the PR, I was thinking I we could instead populate the cache incrementally while searching.
Now, this might be overly complex and not worth doing but in theory or would solve the short-circuit case while helping repeated searches.

We should consider:

• If it even makes sense perf. wise - i.e. the benchmarks for smaller apps I mentioned might justify such change
• Is the short-circuit approach worth keeping? I would say in many cases it might well be sufficient.
• If it's doable - does this part of bootstrap happen synchronously or in parallel? If it's the latter, then we couldn't do it for sure.

However, a complete incrementally populated cache might be an overkill.
Alternatively, we could also consider partial caching:

1. Keep the original short-circuit search.
2. Collect encountered annotation types while searching.
3. If a match is found early, return immediately.
4. If the search reaches the end, publish the complete collected set for subsequent queries.

That avoids storing resumable traversal state. One negative query produces a complete cache, benefiting later queries. Classes that repeatedly match early retain their existing cheap behavior. The limitation is that repeated successful searches near the end may still repeat work until a search completes.

Thoughts?
Also, let's see if we could summon @mkouba here and get his 0.02$ on this :)

[manovotn]

However, a complete incrementally populated cache might be an overkill.
Alternatively, we could also consider partial caching:

FTR, I am trying to get AI to create some first draft of those to see if it's even sensible; will update this PR once I get some form of result which may or may not be tomorrow based on my other work.

[lprimak]

Hi, Matej, thank you for the review, as this is a big performance improvement, I appreciate your efforts helping to get this merged.

I wanted to make this change as simple and DRY as possible, to minimize regressions, thus came up with the simplest solution to the problem that profiler was showing.
I think that incremental cache is indeed an overkill here.

Smaller apps: While I agree with your assessment here, does this matter holistically? This kind of deploy performance is measured in wall clock, so for small apps, even if this optimization is less efficient, would it not actually matter?

FYI: I had AI write the tests and didn't pay too much attention to them, so they may very well be mostly AI slop and need improvement.
Now, I paid a bit more attention to the tests :)

[lprimak]

@manovotn Implemented short-circuit, was not a huge change

[manovotn]

Added few more small remarks.

I have yet to re-review those tests (and likely won't be able to until early next week) but otherwise I think the optimization like this makes sense. The short-circuit is IMO worth keeping.

[lprimak]

All your review comments make sense and have been applied - thank you!

[manovotn]

@lprimak please squash the commits into a single one with some reasonable message and do so on both branches.
Other than that, I think it's good to go.

[lprimak]

please squash the commits

Done for both branches.
FYI: GitHub "Squash and Merge" can be used to do the same thing, without forcing force-pushes, which could be destructive if not careful.

[manovotn]

LGTM

Thank you for contributing @lprimak!

I assume we could have a release by the end of the month (for both versions). In case the date is nearing and I still haven't done it, don't hesitate to poke me here 😅

[lprimak]

Awesome. Glad to contribute again, 10 years later :)