On Tue, 23 Jan 2024 19:00:51 GMT, Volker Simonis 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:
>
> Updated option description and assertion based on review feedback
I support keeping the logic under a flag. I have some concerns about
unconditionally turning it on.
I expect significantly higher footprint overhead when an application has plenty
of tiny methods and deep inlining trees. And java.lang.invoke implementation
pushes it even further (with arbitrarily deep MethodHandle trees and
unconditional inlining through them), so heavy users of MethodHandle API should
experience higher overheads when evol dependencies are recorded.
I suggest to make the flag experimental. Once JFR implementation is improved,
it can be superseded by `-XX:+EnableDynamicAgentLoading` check.
-
PR Comment: https://git.openjdk.org/jdk/pull/17509#issuecomment-1906824455
