On Thu, 23 Nov 2023 16:42:35 GMT, Jaroslav Bachorik <jbacho...@openjdk.org> wrote:
>> Please, review this fix for a corner case handling of `jmethodID` values. >> >> The issue is related to the interplay between `jmethodID` values and method >> redefinitions. Each `jmethodID` value is effectively a pointer to a `Method` >> instance. Once that method gets redefined, the `jmethodID` is updated to >> point to the last `Method` version. >> Unless the method is still on stack/running, in which case the original >> `jmethodID` will be redirected to the latest `Method` version and at the >> same time the 'previous' `Method` version will receive a new `jmethodID` >> pointing to that previous version. >> >> If we happen to capture stacktrace via `GetStackTrace` or >> `GetAllStackTraces` JVMTI calls while this previous `Method` version is >> still on stack we will have the corresponding frame identified by a >> `jmethodID` pointing to that version. >> However, sooner or later the 'previous' class version becomes eligible for >> cleanup at what time all contained `Method` instances. The cleanup process >> will not perform the `jmethodID` pointer maintenance and we will end up with >> pointers to deallocated memory. >> This is caused by the fact that the `jmethodID` lifecycle is bound to >> `ClassLoaderData` instance and all relevant `jmethodID`s will get >> batch-updated when the class loader is being released and all its classes >> are getting unloaded. >> >> This means that we need to make sure that if a `Method` instance is being >> deallocate the associated `jmethodID` (if any) must not point to the >> deallocated instance once we are finished. Unfortunately, we can not just >> update the `jmethodID` values in bulk when purging an old class version - >> the per `InstanceKlass` jmethodID cache is present only for the main class >> version and contains `jmethodID` values for both the old and current method >> versions. >> >> Therefore we need to perform `jmethodID` lookup when we are about to >> deallocate a `Method` instance and clean up the pointer only if that >> `jmethodID` is pointing to the `Method` instance which is being deallocated. >> >> _(For anyone interested, a much lengthier writeup is available in [my >> blog](https://jbachorik.github.io/posts/mysterious-jmethodid))_ > > Jaroslav Bachorik has updated the pull request incrementally with one > additional commit since the last revision: > > Reinstate mistakenly deleted comment src/hotspot/share/oops/instanceKlass.hpp line 1087: > 1085: // - We can not use the jmethodID cache associated with klass > directly because the 'previous' versions > 1086: // do not have the jmethodID cache filled in. Instead, we need to > lookup jmethodID for each method and this > 1087: // is expensive - O(n) for one jmethodID lookup. For all > contained methods it is O(n^2). The comment is helpful, but its an implementation comment, and I would move this part to the implementation. Here, what matters to know is that this function nulls out jmethodIDs for all methods in this IK. The comment also refers to class transformation specifically, I'd mention that so that readers get a context. Do I understand correctly that this comment tries to explain why we - instead of just iterating IK->_methods_jmethod_ids directly - we iterate all methods of this IK and then lookup the associated jmethodID in IK->_methods_jmethod_ids, right? I wondered about that but IIUC the pointers inside IK->_methods_jmethod_ids may refer to jmethodID slots that have been reused for different methods? ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/16662#discussion_r1403995913
