smallrye / jandex

Java Annotation Indexer
Apache License 2.0
392 stars 94 forks source link

Avoid having a ton of empty HashMaps in memory #404

Closed gsmet closed 2 months ago

gsmet commented 2 months ago

I'm working on a heap dump and Eclipse MAT complained that there was a lot of empty HashMaps around. I had a closer look and a lot of them were due to the Map kept in ClassInfo and this one looked like the most plausible culprit. Now, in Quarkus, the Jandex index is just around for building the application but it looks like an easy win. I also handled the size 1 case but I'm not sure it's worth the hassle as long as we stick to Java 8.

@Ladicek this is not to merge as is, it was more to get your feedback with a concrete explanation.

The following screenshot is a query of the empty HashMaps in the heap dump. Note that it's the same in the KeySet entry that is at the top.

Screenshot from 2024-06-24 15-02-36

gsmet commented 2 months ago

Also, I wonder if we should make the map immutable there rather than in the getter.

And I was wondering the reason why we are still supporting Java 8 for modern versions of Jandex?

Ladicek commented 2 months ago

There's multiple org.jboss.jandex.ClassInfo entries in the list, so it must be more than just the annotations map. I wonder if you have a more detailed information, or perhaps the heap dump itself that you could share?

Otherwise, the PR looks good.

gsmet commented 2 months ago

Yeah, so this might be due to the class loader leaks I'm trying to solve :). I often have classes loaded from various CLs. Now I'm not entirely sure here as I wouldn't expect to have several indexes opened.

I checked and unfortunately I don't have the memory dump handy anymore, I took so many of them these past few days that I had to do some cleanup yesterday.

I think I got this particular heap dump when running the extensions/opentelemetry/deployment tests (as that's my current target) but no idea which test triggered it.

I'll ping you if I see it again in my CL leak journey.

BTW, I had a look around in ClassInfo before creating the PR and I didn't see any other potential culprits. But you might be more lucky than I am.

Also did you see my question about Java 8 support?

Ladicek commented 2 months ago

OK, thanks. I'm busy with ArC these days, but once that is over, I might take a look on this.

When it comes to Java 8 support, I'm trying to be more conservative than usual with Jandex, because it's apparently used a lot outside of Quarkus / WildFly. I don't think there's anything that we would gain by bumping to 11 or even 17. Adding a simple implementation of Map for single entry shouldn't be a big deal.

gsmet commented 2 months ago

Yeah I was trying to address this without too much hassle but feel free to be smarter :).

It's definitely not a big priority so take your time :).

Ladicek commented 2 months ago

I actually realized there's Collections.singletonMap(), for the single-entry case. Also, you're right, there's no other Map in ClassInfo (not sure why the picture in the description of this PR includes org.jboss.jandex.ClassInfo multiple times then, but whatever). Updated this PR with that, and will merge.

gsmet commented 2 months ago

Ah yes, doh, forgot about singletonMap(). That's what you get for switching to Map.of() in your daily life.

Thanks!