On Wed, 24 Jan 2024 14:48:52 GMT, Volker Simonis <simo...@openjdk.org> wrote:
>> Currently we don't record dependencies on redefined methods (i.e. >> `evol_method` dependencies) in JIT compiled methods if none of the >> `can_redefine_classes`, `can_retransform_classes` or >> `can_generate_breakpoint_events` JVMTI capabalities is set. This means that >> if a JVMTI agent which requests one of these capabilities is dynamically >> attached, all the methods which have been JIT compiled until that point, >> will be marked for deoptimization and flushed from the code cache. For >> large, warmed-up applications this mean deoptimization and instant >> recompilation of thousands if not then-thousands of methods, which can lead >> to dramatic performance/latency drop-downs for several minutes. >> >> One could argue that dynamic agent attach is now deprecated anyway (see [JEP >> 451: Prepare to Disallow the Dynamic Loading of >> Agents](https://openjdk.org/jeps/451)) and this problem could be solved by >> making the recording of `evol_method` dependencies dependent on the new >> `-XX:+EnableDynamicAgentLoading` flag isntead of the concrete JVMTI >> capabilities (because the presence of the flag indicates that an agent will >> be loaded eventually). >> >> But there a single, however important exception to this rule and that's JFR. >> JFR is advertised as low overhead profiler which can be enabled in >> production at any time. However, when JFR is started dynamically (e.g. >> through JCMD or JMX) it will silently load a HotSpot internl JVMTI agent >> which requests the `can_retransform_classes` and retransforms some classes. >> This will inevitably trigger the deoptimization of all compiled methods as >> described above. >> >> I'd therefor like to propose to *always* and unconditionally record >> `evol_method` dependencies in JIT compiled code by exporting the relevant >> properties right at startup in `init_globals()`: >> ```c++ >> jint init_globals() { >> management_init(); >> JvmtiExport::initialize_oop_storage(); >> +#if INCLUDE_JVMTI >> + JvmtiExport::set_can_hotswap_or_post_breakpoint(true); >> + JvmtiExport::set_all_dependencies_are_recorded(true); >> +#endif >> >> >> My measurements indicate that the overhead of doing so is minimal (around 1% >> increase of nmethod size) and justifies the benefit. E.g. a Spring Petclinic >> application started with `-Xbatch -XX:+UnlockDiagnosticVMOptions >> -XX:+LogCompilation` compiles about ~11500 methods (~9000 with C1 and ~2500 >> with C2) resulting in an aggregated nmethod size of around ~40bm. >> Additionally recording `evol_method` dependencies only increases this size >> be about 400kb.... > > Volker Simonis has updated the pull request incrementally with one additional > commit since the last revision: > > Made the flag experimental and added an assertion to > set_can_hotswap_or_post_breakpoint() I am good with these changes, only a few stylistic nits. I am on the fence where the default for this flag should stand. The 1% loss in nmethod size is probably okay, given that we gained as much with denser code improvements like [JDK-8319406](https://bugs.openjdk.org/browse/JDK-8319406). Maybe there are some tricks in dependency encoding that would drive this down even more. src/hotspot/share/prims/jvmtiExport.hpp line 159: > 157: // recorded from that point on. > 158: assert(!_can_hotswap_or_post_breakpoint || on, "sanity check"); > 159: _can_hotswap_or_post_breakpoint = (on != 0); Pre-existing: wild that this code checks `!= 0` against `bool`, when it could have just used the bool directly, like the new assert does. I see no reason to keep `!= 0` here. src/hotspot/share/runtime/globals.hpp line 2016: > 2014: "Unconditionally record nmethod dependencies on class " > \ > 2015: "rewriting/transformation independently of the JVMTI " > \ > 2016: " can_{retransform/redefine}_classes capabilities.") > \ Probably match the indenting of previous delcarations? ------------- Marked as reviewed by shade (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17509#pullrequestreview-1845365087 PR Review Comment: https://git.openjdk.org/jdk/pull/17509#discussion_r1467418701 PR Review Comment: https://git.openjdk.org/jdk/pull/17509#discussion_r1467415176
