On Thu, 23 Nov 2023 14:15:54 GMT, Thomas Stuefe <stu...@openjdk.org> wrote:
>> @dholmes-ora >>> Can't we just check method->method_holder() for null rather than passing in >>> a parameter like this? >> >> I have removed the argument and I am now performing the check for >> `method_holder() != nullptr` as recommended. The code is a bit simpler and >> the cost of resolving the method holder for each method is probably quite >> low so we should be ok here. > > @jbachorik You are aware that this fix only works for some uncommon corner > cases, right? > > It only works if the Method is explicitly deallocated. The vast bulk of > Method aren't. Method, as a Metaspace object, is released in bulk when the > class is unloaded. The `::deallocate` path you fixed up - that eventually > ends up in `MetaspaceArena::deallocate()` - is a rare case that only gets > invoked if > - a class cannot be loaded but parts of it have already been loaded into > Metaspace. > - a class gets transformed > > In case the class gets unloaded via conventional means, your fix won't get > invoked (nor should it; releasing in bulk without having to care for > individual allocations is the point of Metaspace). > @tstuefe > > > In case the class gets unloaded via conventional means, your fix won't get > > invoked (nor should it; releasing in bulk without having to care for > > individual allocations is the point of Metaspace). > > Yes. This is intended. The deallocation path via Metaspace is fine. It is > just the code that is purging previous versions of a redefined class that has > this bug. I see it now. On class unloading, we reset the jmethodID "master table" linked list nodes, set all slots to null, but don't delete them to keep outside users from crashing. And we release the IK itself and before we do that free the methodID cache in IK. So we are covered. I thought this was about class unloading, sorry for not reading the description carefully. ------------- PR Comment: https://git.openjdk.org/jdk/pull/16662#issuecomment-1824662450
