Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid having a ton of empty HashMaps in memory #404

Merged
merged 1 commit into from
Jun 26, 2024

Conversation

gsmet
Copy link
Contributor

@gsmet gsmet commented Jun 24, 2024

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
Copy link
Contributor Author

gsmet commented Jun 24, 2024

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
Copy link
Collaborator

Ladicek commented Jun 26, 2024

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.

@Ladicek Ladicek added this to the 3.2.1 milestone Jun 26, 2024
@gsmet
Copy link
Contributor Author

gsmet commented Jun 26, 2024

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
Copy link
Collaborator

Ladicek commented Jun 26, 2024

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
Copy link
Contributor Author

gsmet commented Jun 26, 2024

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 :).

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.
@Ladicek
Copy link
Collaborator

Ladicek commented Jun 26, 2024

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
Copy link
Contributor Author

gsmet commented Jun 26, 2024

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

Thanks!

@Ladicek Ladicek merged commit 1352479 into smallrye:main Jun 26, 2024
35 checks passed
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants