On Sat, 25 Nov 2023 02:20:18 GMT, David Holmes <dhol...@openjdk.org> wrote:
>> This has gotten a lot more complicated. All I was suggesting was if this:
>>
>> if (with_method_holders) {
>> method->clear_jmethod_id();
>> }
>>
>> could be changed to
>>
>> if (method->method_holder() == nullptr) {
>> method->clear_jmethod_id();
>> }
>>
>> Now I'm not at all sure what you are doing.
>
>> @dholmes-ora Unfortunately, I can not just do `method->method_holder() ==
>> nullptr` as `method_holder()` is expanding to
>> `Method::constants()->pool_holder()` and `Method::constants()` is expanding
>> to `Method::constMethod()->constants()`. This can cause SIGSEGV if either
>> `Method::_constMethod` or `ConstMethod::_constants` is NULL.
>
> I'm getting a strange sense of deja-vu here. This API is flawed if you cannot
> even ask for the method holder without some intervening value causing a SEGV.
> I've lost sight of the big picture here in terms of the lifecycle of the
> Method we are querying, the methodID we may be clearing and the existence or
> not of a method_holder().
@dholmes-ora I removed the assert. It is not necessary any more as the call to
`Method::clear_jmethod_id()` was moved from the more generic
`Method::deallocate_contents()` to `InstanceKlass::clear_jmethod_ids()` which
is called if and only if the previous class versions are being purged. Because
the issue with the method holder is related to `ClassParser` and not fully
initialized classes only, the assert can safely be removed. The
`Method::clear_jmethod_id()` will never be called in a context when the link to
its method holder is broken.
> I've lost sight of the big picture here in terms of the lifecycle of the
> Method we are querying, the methodID we may be clearing and the existence or
> not of a method_holder().
I have updated the PR description to correspond to the actual state of affairs
- the change is that instead of doing the `jmethodID` pointer maintenance for
each `Method::deallocate_contents()` call it will be done only for methods
contained by the old class versions that are getting purged. This has two
benefits compared to the original proposal:
- we add the overhead of `jmethodID` lookup only to the problematic case of
purging old class versions
- method holder is always valid when purging old class versions so we don't
need to have checks/asserts for that
-------------
PR Comment: https://git.openjdk.org/jdk/pull/16662#issuecomment-1827409695
PR Comment: https://git.openjdk.org/jdk/pull/16662#issuecomment-1827430093
