On Thu, 17 Feb 2022 05:57:24 GMT, Chris Plummer <cjplum...@openjdk.org> wrote:
> processUnloads is continuously called on every event. It's purpose is to > process pending CLASS_UNLOAD events that have accumulated since the last > JVMTI event. This is a rather hacky solution. JVMTI does not actually send a > CLASS_UNLOAD event. It doesn't support them. What it does support is > OBJECT_FREE events. So when the debugger has enabled CLASS_UNLOAD events, the > debug agent enables OBJECT_FEE events. It tags each j.l.Class instance so it > will get the OBJECT_FREE event for them, and when the event comes in, the > Class gets added to deletedSignatures so the agent can later notify the > debugger about it. > > As a side note, this OBJECT_FREE solution is somewhat new. The old solution > was to always do an "allClasses" during initialization, and then diff that > with "allClasses" every time there was a full GC to see which classes > unloaded. I wonder now if there isn't a reason we just don't send the > CLASS_UNLOAD when the OBJECT_FREE is received. It would probably greatly > simplify classTrack.c, and will fix an outstanding bug we have where > sometimes there is a lengthy delay before CLASS_UNLOAD events are sent. This > is because once there is a GC, they events are not sent until the next JVMTI > event. It's actually possible there would never be one if the only thing the > debugger is requesting is CLASS_UNLOAD events. > > Anyway, back to processUnloads synchronization. The reason for > synchronization between processUnload and activate is because one thread > could be handling some random JVMTI event (maybe a breakpoint) and is calling > processUnloads, and at the same time another thread is handling a > CLASS_UNLOAD event request from the debugger, and calling activate. But now > that I think of it, this should be harmless even without synchronization. The > processUnloads thread will see the allocated deletedSignatures if activate > has gotten that far, otherwise it won't. It should behave correctly in either > case. I'm not convince that activate even needs synchronization. The purpose > of the classTrack synchronization is to prevent one thread in processUnloads > from grabbing and using deletedSignatures, only to have another thread in > processUnloads do the same, and delete deletedSignatures in the process, > leaving the 1st thread holding onto a deallocated pointer. However, even this > should not be possible because all callers to processUnloads have already grabbed the handlerLock. > > What I'm tempted to say here is just have this PR keep all the > synchronization and I'll file a CR to possibly do a lot of cleanup here. > Either remove (maybe all of) the synchronization, or maybe even implement > sending the CLASS_UNLOAD when the OBJECT_FREE comes in, which will make the > synchronization question moot, and also fix that other bug I mentioned. > > BTW, one other bit of nonsense. classTrack_activate() does: > > ``` > deletedSignatures = bagCreateBag(sizeof(char*), 1000); > ``` > > This will be deleted the very first time an event comes in, even if no > classes were unloaded, and processUnloads replaces it with: > > ``` > deletedSignatures = bagCreateBag(sizeof(char*), 10); > ``` I believe checking `bagSize() == 0` is an optimization. I think just taking `handlerLock` lock before calling `classTrack_reset()` ensures no race can happen and seems harmless. ------------- PR: https://git.openjdk.java.net/jdk/pull/7461
