Consider enabling major optimization in AspectJ 1.9.8

[LifeIsStrange]

eclipse-aspectj/aspectj#9

In our project we found out that during the build up of the spring context
the class loading takes a very long time.
Root cause is the huge amount of file I/O during pointcut class loading.
We are taking about ~250k file loads.

With these changes we managed to cut down the starting time by around 50%.

What we found out is that IMHO - the clear method of the ClassLoaderRepository
is called far too often -> in our settings this resulted in not a single cache
hit as the cache got cleared permanently.
Therefore we de-actived the cache clear calls inside the ClassLoaderRepository.

Secondly we changed the Java15AnnotationFinder in a way to not always create
new objects for the ClassLoaderRepository but re-use one static instance.
Otherwise we experienced >100k objects being created.

Last but not least we introduced a cache for unavailable classes so that
they do not have to be looked up using file I/O over and over again.

Therefore, if that performance profile is common, then enabling this optimization with a flag on the next AspectJ release has the potential:

• To cut in half Spring startup time!
• might affect overall throughput positively (including IO)
• might affect memory consumption positively (better cache hits, etc)

While this PR seems extremely interesting, it lacks extensive testing so it would be a nice, high impact, low hanging fruit for the Spring team, to test it, to measure the effects on metrics (startup time, throughput, resource use (io, cpu) and memory use. And finally to enable it by default after some testing :)

[bclozel]

As far as I understand, this optimization has not been enabled by default. This requires setting the "org.aspectj.apache.bcel.ignoreCacheClearRequests" System property. I don't think Spring Framework usually does that. Shouldn't we align with AspectJ defaults and let the community opt-in to test things out?

[jhoeller]

At the core framework level, we don't really deal with setting system properties indeed. We don't even have a proper place to initialize such a system property, just possibly in a Servlet initializer.

From that perspective, enabling this optimization by default seems more sensible at the Spring Boot level?

[LifeIsStrange]

The community will largely ignore even the existence of this option. Since this sounds like a free performance win with no downsides I'd vote for enabling it by default. Though maybe this is spring boot job?

[bclozel]

I'm not so sure this has no downside at all - the AspectJ team would have enabled it by default if it was that clear cut.

I'm moving this issue for consideration to Spring Boot as we can't act on it in Spring Framework anyway.

[wilkinsona]

I don't think we should do this. From the end of @aclement's comment that @bclozel linked to above:

So I think it is a reasonable call to make these opt-in for now, make sure people know they can try them and we are looking for feedback.

FWIW, I'm not sure that we're in a position to set an AspectJ system property either as it may conflict with something else running in the same JVM, particularly in a war deployment to a standalone container.

[aclement]

I was hoping I'd get some feedback relatively quickly and we could look to turn these on even in 1.9.9 - I agree probably not up to Spring to do it. I suppose running all the Spring tests with the options turned on could be something to check (I haven't tried that) but that is basic testing that verifies nothing breaks, it wouldn't be revealing issues like memory bloat over time due to never clearing/caches or the shared static state causing problems.

But I know, I probably won't see the real error reports until it is turned on by default. (Feel free to cc: me on any feedback you get for anyone using these under Spring)

Maybe I could do a Spring blog to spread the word and try to drive feedback.

[LifeIsStrange]

@aclement making an announcement/linking the blog on the java subreddit could gather some feedback too

[bclozel]

FYI I've run the entire Spring Framework test suite and got no failure, but cannot vouch for any performance issue with tests. @LifeIsStrange do you have an application where this new feature could have an impact? If so, we'd be interested in your findings.

[LifeIsStrange]

No unfortunately I have no web application at hand to test

[philwebb]

I'm going to close this one since I don't think Spring Boot should be changing the default. I think the best plan is to try and have AspectJ users test their applications with the property set and for the default to be switched in due course.

[LifeIsStrange]

@aclement Out of curiosity, has this optimization been turned on by default since then?

[aclement]

@LifeIsStrange I don't believe so.

https://github.com/eclipse-aspectj/aspectj/blob/2b0e11d8633c84e8b0827e0eb77b4639fe0914e4/weaver/src/main/java/org/aspectj/weaver/reflect/Java15AnnotationFinder.java#L54