On Wed, 15 Oct 2025 06:56:53 GMT, Tobias Hartmann <[email protected]> wrote:
>> src/hotspot/share/code/nmethod.hpp line 1066: >> >>> 1064: // Used for fast breakpoint support if only_calling_convention is >>> false; >>> 1065: // used for updating the calling convention if true. >>> 1066: bool is_dependent_on_method(Method* dependee, bool >>> only_calling_convention); >> >> I'm not really happy about this `bool only_calling_convention`. I'd rather >> like a `Dependencies::DepType` instead because it is only used in >> >> Dependencies::DepType dep_type = only_calling_convention ? >> Dependencies::mismatch_calling_convention : Dependencies::evol_method; >> >> >> The problem is that then I get a cyclic include between `nmethod.hpp` and >> `dependencies.hpp`. It's probably avoidable, but I need to refactor a bit >> too intensely than I feel comfortable in such a small fix. > > I guess an alternative would be to add separate methods, similar to what we > have for `CodeCache::mark_dependents_on_method_for_breakpoint` -> > `CodeCache::mark_dependents_on_method_for_mismatch` or something. That would > at least limit the `only_calling_convention` arg to `is_dependent_on_method`. > > Or what about always checking both dependencies in > `nmethod::is_dependent_on_method`? After all, both dependencies represent an > actual dependency: > - If the nmethod has a `evol_method` dependency, it's supposed to be deopted > anyway. It doesn't matter where this happens. > - If the nmethod has a `mismatch_calling_convention` dependency, in the worst > case we deopt it when we reach this code via > `CodeCache::mark_dependents_on_method_for_breakpoint` or > `WB_DeoptimizeMethod`, i.e. when we want to make sure that all compiled > versions (via inlining) of a method are deopted. So we would unnecessarily > deopt the caller of a mismatched method when we set a breakpoint in that > mismatched method (or deopt it via the WB API). I think that's fine. I started with separate methods, but that was unfortunate duplication. I went for the second option, since testing seems happy. I'm not sure about the new names I introduced tho (`method_types` and `has_method_dep`). If you have better idea, I'll be happy to change them. ------------- PR Review Comment: https://git.openjdk.org/valhalla/pull/1677#discussion_r2432489457
