search

eclipse-aspectj / aspectj

Other
291 stars 84 forks source link

Potential ThreadLocalMap.Entry accumulation in JoinPointImpl #302

Closed KimmingLau closed 5 months ago

KimmingLau commented 6 months ago

Our application is trying to upgrade aspectjweaver from 1.9.5 to 1.9.21.2 recently. But after upgradtion we found that memory usage in the old generation grows faster and FGC became more frequently, by the way we use jdk 1.8 and CMS+ParNew.

So we had made a heapdump and then found that the size of ThreadLocalMap in our application thread was much larger than before. And I've figured out that the value of growing entry in ThreadLocalMap is from field org.aspectj.runtime.reflect.JoinPointImpl#arcIndex. That's why I tried to review change histroy in JoinPointImpl and issue the following comments https://github.com/eclipse-aspectj/aspectj/commit/43df701a10a80306dd98da8644a9bbfbf5d17089#r140486566

Sorry for that I cannot provide more information about problem from which our application is suffering, such as monitor graph or heapdump file. I am trying to make a reproducer.

kriegaex commented 6 months ago

JoinPointImpl.arcIndex is a ThreadLocal<Integer>. It should consume minimal memory. How can it cause a noticeable memory leak? It should definitely be much, much better than the inheritable thread-local stack of around closures we had before. Of course, the list of around closures still exists, but it also has to, as nobody can predict which thread might need any entries from it at any given time.

KimmingLau commented 6 months ago

Just as what I mentioned in code comments, ThreadLocalMap.Entry is WeakReference. When the JoinPointImpl object is collected by GC, the Entry object is still remain. If the adviced method were called frequently, it would result in accumulation of a lot of Entry object. In our own application, we found tens of thousands of Entry per Thread, which were almostly from JoinPointImpl.arcIndex.

The life cycle of ThreadLocal.Entry here should be consistent with the JoinPointImpl object, so I think we should proactively call ThreadLocal.remove() after using JoinPointImpl.arcIndex.

KimmingLau commented 6 months ago

Here is a of the mentioned entrys information in heapdump:

Snipaste_2024-04-03_11-29-03

kriegaex commented 6 months ago

The life cycle of ThreadLocal.Entry here should be consistent with the JoinPointImpl object, so I think we should proactively call ThreadLocal.remove() after using JoinPointImpl.arcIndex.

Sorry, I am working on something else at the moment, I am too busy to immerse myself into the details here at the moment. Please be more specific and suggest code changes. As for those weak references, should they not be collected as well after the join point impl was collected?

Edit: Please, post screenshots inline, not as zip files. I just fixed that in your comment.

kriegaex commented 6 months ago

The problem with releasing the thread-locals is that a live thread could in principle defer proceeding indefinitely or not proceed at all. AspectJ has no way of knowing. Maybe there is a way to at least remove the thread-locals in cases where the last proceed has happened already, i.e. the index is back down to zero. I have not inspected the code yet, I am totally out of context at the moment.