RFR: 8324241: Always record evol_method deps to avoid excessive method flushing [v2]

[Volker Simonis]

On Tue, 23 Jan 2024 03:31:59 GMT, Dean Long <dlong at openjdk.org> wrote:

>> Volker Simonis has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Guard the feature with a diagnostic option and update the comments in the code
>
> src/hotspot/share/runtime/init.cpp line 121:
> 
>> 119:   if (AlwaysRecordEvolDependencies) {
>> 120:     JvmtiExport::set_can_hotswap_or_post_breakpoint(true);
>> 121:     JvmtiExport::set_all_dependencies_are_recorded(true);
> 
> I think the check for AlwaysRecordEvolDependencies needs to be moved into set_can_hotswap_or_post_breakpoint and set_all_dependencies_are_recorded, otherwise don't we risk the value being accidentally reset to false when set_can_hotswap_or_post_breakpoint() is called again by JvmtiManageCapabilities::update()?

A good question, but after deep digging (it took me quite some time to figure this out myself :) I don't think that `can_hotswap_or_post_breakpoint`/`all_dependencies_are_recorded` can ever be reset. Here's how it works:

- there's a global set of `always_capabilities` which is initialized when the first JVMTI environment is created:

// capabilities which are always potentially available
jvmtiCapabilities JvmtiManageCapabilities::always_capabilities;

void JvmtiManageCapabilities::initialize() {
  _capabilities_lock = new Mutex(Mutex::nosafepoint, "Capabilities_lock");
  always_capabilities = init_always_capabilities();


- as the name implies, this set of capabilities contains all generally available capabilities (except for the `onload` and `solo` capabilites, which are maintained in separate sets).
- `JvmtiManageCapabilities::update()` only operates on this global sets of capabilites (and *not* on the concrete capabilities of a specific JVMTI environment):

void JvmtiManageCapabilities::update() {
  jvmtiCapabilities avail;
  // all capabilities
  either(&always_capabilities, &always_solo_capabilities, &avail);
...
  // If can_redefine_classes is enabled in the onload phase then we know that the
  // dependency information recorded by the compiler is complete.
  if ((avail.can_redefine_classes || avail.can_retransform_classes) &&
      JvmtiEnv::get_phase() == JVMTI_PHASE_ONLOAD) {
    JvmtiExport::set_all_dependencies_are_recorded(true);
  }
...
  JvmtiExport::set_can_hotswap_or_post_breakpoint(
    avail.can_generate_breakpoint_events ||
    avail.can_redefine_classes ||
    avail.can_retransform_classes);

- This means that `JvmtiManageCapabilities::update()` is always conservative regarding its exports to `JvmtiExport` as it always exports *all* potentially available capabilites once a JVMTI environment is created and not only the specific capabilities requested by concrete JVMTI environment. This means that even if we create a JVMTI agent with an empty set of capabilities, `can_hotswap_or_post_breakpoint`/`all_dependencies_are_recorded` will still be set in `JvmtiExport`.
- There's no code path (at least I couldn't find one) which takes capabilities away from the `always_capabilities` set. Only if an environment requests `on_load` capabilities, they will be transferred from the global `onload_capabilities` to the `always_capabilities` set:

jvmtiError JvmtiManageCapabilities::add_capabilities(const jvmtiCapabilities *current,
                                                     const jvmtiCapabilities *prohibited,
                                                     const jvmtiCapabilities *desired,
                                                     jvmtiCapabilities *result) {
...
  // onload capabilities that got added are now permanent - so, also remove from onload
  both(&onload_capabilities, desired, &temp);
  either(&always_capabilities, &temp, &always_capabilities);
  exclude(&onload_capabilities, &temp, &onload_capabilities);


If you like I could put an assertion into `JvmtiExport::set_can_hotswap_or_post_breakpoint()` which verifies that `can_hotswap_or_post_breakpoint` never gets reset once it was set to `true`. What do you think?

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/17509#discussion_r1463584725