Re: RFR: 8333891: Method excluded with directive is not compiled after removal of directive
On Mon, 10 Jun 2024 20:05:03 GMT, Evgeny Astigeevich wrote: > We can exclude Java methods from compilation with compiler directives. Later > we can remove those directives. > This PR fixes a bug that after removal of those directives Java methods don't > become compilable. > A regression test is added. > > Tested fastdebug build with a new test and tier1 tests. Test `runtime/BootstrapMethod/BSMCalledTwice.java` might have failed on Windows x64 because of the change. - PR Comment: https://git.openjdk.org/jdk/pull/19637#issuecomment-2163808736
RFR: 8333891: Method excluded with directive is not compiled after removal of directive
We can exclude Java methods from compilation with compiler directives. Later we can remove those directives. This PR fixes a bug that after removal of those directives Java methods don't become compilable. A regression test is added. Tested fastdebug build with a new test and tier1 tests. - Commit messages: - 8333891: Method excluded with directive is not compiled after removal of directive Changes: https://git.openjdk.org/jdk/pull/19637/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19637=00 Issue: https://bugs.openjdk.org/browse/JDK-8333891 Stats: 122 lines in 2 files changed: 122 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/19637.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19637/head:pull/19637 PR: https://git.openjdk.org/jdk/pull/19637
Integrated: 8332111: [BACKOUT] A way to align already compiled methods with compiler directives
On Mon, 13 May 2024 13:03:26 GMT, Evgeny Astigeevich wrote: > Backout of [JDK-8309271](https://bugs.openjdk.org/browse/JDK-8309271) which > has known bugs, possible bugs and performance issues. REDO work is tracked by > [JDK-8331749](https://bugs.openjdk.org/browse/JDK-8331749). > > Found bugs: > - When refreshing `CompilerDirectivesAddDCmd::execute` will call > `DirectivesStack::hasMatchingDirectives(mh, true)` which only considers the > compiler directive which is on the top of the directives stack. As more than > one directive can be added, `CompilerDirectivesAddDCmd::execute` will not > behave as expected. > - A Java method with old directives might be in the compilation queue. A > request to recompile it with new directives will be ignored. > > There are other concerns: bugs and performance issues. > > Possible bugs: > - `has_matching_directives` might not be cleared. A nmethod might get into > the unloading state before `CodeCache::recompile_marked_directives_matches`. > If the nmethod has been used to mark a Java method and it is the only > nmethod, there will be no nmethod in CodeCache to reach the Java method to > clear the mark. > - A Java method might have been compiled with new directives before > `CodeCache::recompile_marked_directives_matches`. > `CodeCache::recompile_marked_directives_matches` will recompile it again. > - JIT compiler might be compiling a Java method with old directives. A > request to recompile it with new directives will be ignored. > > Performance issues: > - Usually directives are updated for a small number of Java methods. If > CodeCache has thousands of nmethods, > `CodeCache::recompile_marked_directives_matches` will be traversing nmethods > most of which don't need recompilation. > > The backout is not clean because of removal of `CompiledMethod`. > > Tested with release and fastdebug builds: tier1 and tier2 passed. This pull request has now been integrated. Changeset: 1a944478 Author:Evgeny Astigeevich URL: https://git.openjdk.org/jdk/commit/1a944478a26a766f5a573a1236b642d8e7b0685c Stats: 380 lines in 15 files changed: 3 ins; 347 del; 30 mod 8332111: [BACKOUT] A way to align already compiled methods with compiler directives Reviewed-by: shade, kvn - PR: https://git.openjdk.org/jdk/pull/19215
Re: RFR: 8332111: [BACKOUT] A way to align already compiled methods with compiler directives
On Mon, 13 May 2024 22:43:44 GMT, Vladimir Kozlov wrote: >> What if instead of backing out we will use an experimental JVM flag: >> `XX:+CompilerDirectivesRefreshSupport`? > >> What if instead of backing out we will use an experimental JVM flag: >> `XX:+CompilerDirectivesRefreshSupport`? > > I don't think this is correct way to fix the bug. Thank you, @vnkozlov @dchuyko @shipilev - PR Comment: https://git.openjdk.org/jdk/pull/19215#issuecomment-2112072984
Re: RFR: 8332111: [BACKOUT] A way to align already compiled methods with compiler directives
On Mon, 13 May 2024 13:03:26 GMT, Evgeny Astigeevich wrote: > Backout of [JDK-8309271](https://bugs.openjdk.org/browse/JDK-8309271) which > has known bugs, possible bugs and performance issues. REDO work is tracked by > [JDK-8331749](https://bugs.openjdk.org/browse/JDK-8331749). > > Found bugs: > - When refreshing `CompilerDirectivesAddDCmd::execute` will call > `DirectivesStack::hasMatchingDirectives(mh, true)` which only considers the > compiler directive which is on the top of the directives stack. As more than > one directive can be added, `CompilerDirectivesAddDCmd::execute` will not > behave as expected. > - A Java method with old directives might be in the compilation queue. A > request to recompile it with new directives will be ignored. > > There are other concerns: bugs and performance issues. > > Possible bugs: > - `has_matching_directives` might not be cleared. A nmethod might get into > the unloading state before `CodeCache::recompile_marked_directives_matches`. > If the nmethod has been used to mark a Java method and it is the only > nmethod, there will be no nmethod in CodeCache to reach the Java method to > clear the mark. > - A Java method might have been compiled with new directives before > `CodeCache::recompile_marked_directives_matches`. > `CodeCache::recompile_marked_directives_matches` will recompile it again. > - JIT compiler might be compiling a Java method with old directives. A > request to recompile it with new directives will be ignored. > > Performance issues: > - Usually directives are updated for a small number of Java methods. If > CodeCache has thousands of nmethods, > `CodeCache::recompile_marked_directives_matches` will be traversing nmethods > most of which don't need recompilation. > > The backout is not clean because of removal of `CompiledMethod`. > > Tested with release and fastdebug builds: tier1 and tier2 passed. What if instead of backing out we will use an experimental JVM flag: `XX:+CompilerDirectivesRefreshSupport`? - PR Comment: https://git.openjdk.org/jdk/pull/19215#issuecomment-2108802569
Re: RFR: 8332111: [BACKOUT] A way to align already compiled methods with compiler directives
On Mon, 13 May 2024 16:29:35 GMT, Vladimir Kozlov wrote: > How you found these issues? I've been backporting JDK-8309271 to downstream 17 and 21. As compilations happens in background but a test from JDK-8309271 runs with background compilation off, I asked myself what might happen with background compilation. I have a patch fixing the test above. I don't think it is a complete fix. - PR Comment: https://git.openjdk.org/jdk/pull/19215#issuecomment-2108770472
Re: RFR: 8332111: [BACKOUT] A way to align already compiled methods with compiler directives
On Mon, 13 May 2024 13:03:26 GMT, Evgeny Astigeevich wrote: > Backout of [JDK-8309271](https://bugs.openjdk.org/browse/JDK-8309271) which > has known bugs, possible bugs and performance issues. REDO work is tracked by > [JDK-8331749](https://bugs.openjdk.org/browse/JDK-8331749). > > Found bugs: > - When refreshing `CompilerDirectivesAddDCmd::execute` will call > `DirectivesStack::hasMatchingDirectives(mh, true)` which only considers the > compiler directive which is on the top of the directives stack. As more than > one directive can be added, `CompilerDirectivesAddDCmd::execute` will not > behave as expected. > - A Java method with old directives might be in the compilation queue. A > request to recompile it with new directives will be ignored. > > There are other concerns: bugs and performance issues. > > Possible bugs: > - `has_matching_directives` might not be cleared. A nmethod might get into > the unloading state before `CodeCache::recompile_marked_directives_matches`. > If the nmethod has been used to mark a Java method and it is the only > nmethod, there will be no nmethod in CodeCache to reach the Java method to > clear the mark. > - A Java method might have been compiled with new directives before > `CodeCache::recompile_marked_directives_matches`. > `CodeCache::recompile_marked_directives_matches` will recompile it again. > - JIT compiler might be compiling a Java method with old directives. A > request to recompile it with new directives will be ignored. > > Performance issues: > - Usually directives are updated for a small number of Java methods. If > CodeCache has thousands of nmethods, > `CodeCache::recompile_marked_directives_matches` will be traversing nmethods > most of which don't need recompilation. > > The backout is not clean because of removal of `CompiledMethod`. > > Tested with release and fastdebug builds: tier1 and tier2 passed. There is no `PrintOptoAssembly` in output. I use `lockCompilation()`/`unlockCompilation()` to simulate: > A Java method with old directives might be in the compilation queue. A > request to recompile it with new directives will be ignored. I think using them we can also simulate, though it would not be easy to write a test: > JIT compiler might be compiling a Java method with old directives. A request > to recompile it with new directives will be ignored. - PR Comment: https://git.openjdk.org/jdk/pull/19215#issuecomment-2108759073
Re: RFR: 8332111: [BACKOUT] A way to align already compiled methods with compiler directives
On Mon, 13 May 2024 16:29:35 GMT, Vladimir Kozlov wrote:
> do you have tests which shows issues you listed in description?
Here is a jtreg test:
- `refresh_control.02.txt`
[
{
match: "serviceability.dcmd.compiler.DirectivesRefreshTest::callable",
c2: {
PrintOptoAssembly: true
}
}
]
- `DirectivesRefreshTest02.java`
/**
* @test DirectivesRefreshTest02
* @summary Test of forced recompile after compiler directives changes by
diagnostic command
* @requires vm.compiler1.enabled & vm.compiler2.enabled
* @library /test/lib /
* @modules java.base/jdk.internal.misc
*
* @build jdk.test.whitebox.WhiteBox
* @run driver jdk.test.lib.helpers.ClassFileInstaller
jdk.test.whitebox.WhiteBox
*
* @run main/othervm -Xbootclasspath/a:. -XX:+UnlockDiagnosticVMOptions
-XX:+WhiteBoxAPI
* -XX:+BackgroundCompilation -Xlog:codecache=trace
-XX:-Inline -XX:+TieredCompilation -XX:CICompilerCount=2
* -XX:+UnlockDiagnosticVMOptions
* serviceability.dcmd.compiler.DirectivesRefreshTest02
*/
package serviceability.dcmd.compiler;
import jdk.test.whitebox.WhiteBox;
import jdk.test.lib.process.OutputAnalyzer;
import jdk.test.lib.dcmd.CommandExecutor;
import jdk.test.lib.dcmd.JMXExecutor;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.lang.reflect.Method;
import java.util.Random;
import static jdk.test.lib.Asserts.assertEQ;
import static compiler.whitebox.CompilerWhiteBoxTest.COMP_LEVEL_NONE;
import static compiler.whitebox.CompilerWhiteBoxTest.COMP_LEVEL_SIMPLE;
import static
compiler.whitebox.CompilerWhiteBoxTest.COMP_LEVEL_FULL_OPTIMIZATION;
public class DirectivesRefreshTest02 {
static Path cmdPath = Paths.get(System.getProperty("test.src", "."),
"refresh_control.02.txt");
static WhiteBox wb = WhiteBox.getWhiteBox();
static Random random = new Random();
static Method method;
static CommandExecutor executor;
static int callable() {
int result = 0;
for (int i = 0; i < 100; i++) {
result += random.nextInt(100);
}
return result;
}
static void setup() throws Exception {
method = DirectivesRefreshTest.class.getDeclaredMethod("callable");
executor = new JMXExecutor();
wb.enqueueMethodForCompilation(method, COMP_LEVEL_SIMPLE);
while (wb.isMethodQueuedForCompilation(method)) {
Thread.onSpinWait();
}
wb.lockCompilation();
boolean r = wb.enqueueMethodForCompilation(method,
COMP_LEVEL_FULL_OPTIMIZATION);
System.out.println("Method enqueued: " + r);
}
static void testDirectivesAddRefresh() {
var output = executor.execute("Compiler.directives_add -r " +
cmdPath.toString());
output.stderrShouldBeEmpty().shouldContain("1 compiler directives
added");
System.out.println("Method enqueued: " +
wb.isMethodQueuedForCompilation(method));
wb.unlockCompilation();
wb.enqueueMethodForCompilation(method, COMP_LEVEL_FULL_OPTIMIZATION);
while (wb.isMethodQueuedForCompilation(method)) {
Thread.onSpinWait();
}
System.out.println("Method compilation level: " +
wb.getMethodCompilationLevel(method));
assertEQ(true, false, "Stop here");
}
public static void main(String[] args) throws Exception {
setup();
testDirectivesAddRefresh();
}
}
-
PR Comment: https://git.openjdk.org/jdk/pull/19215#issuecomment-2108744800
Re: RFR: 8332111: [BACKOUT] A way to align already compiled methods with compiler directives
On Mon, 13 May 2024 13:03:26 GMT, Evgeny Astigeevich wrote: > Backout of [JDK-8309271](https://bugs.openjdk.org/browse/JDK-8309271) which > has known bugs, possible bugs and performance issues. REDO work is tracked by > [JDK-8331749](https://bugs.openjdk.org/browse/JDK-8331749). > > Found bugs: > - When refreshing `CompilerDirectivesAddDCmd::execute` will call > `DirectivesStack::hasMatchingDirectives(mh, true)` which only considers the > compiler directive which is on the top of the directives stack. As more than > one directive can be added, `CompilerDirectivesAddDCmd::execute` will not > behave as expected. > - A Java method with old directives might be in the compilation queue. A > request to recompile it with new directives will be ignored. > > There are other concerns: bugs and performance issues. > > Possible bugs: > - `has_matching_directives` might not be cleared. A nmethod might get into > the unloading state before `CodeCache::recompile_marked_directives_matches`. > If the nmethod has been used to mark a Java method and it is the only > nmethod, there will be no nmethod in CodeCache to reach the Java method to > clear the mark. > - A Java method might have been compiled with new directives before > `CodeCache::recompile_marked_directives_matches`. > `CodeCache::recompile_marked_directives_matches` will recompile it again. > - JIT compiler might be compiling a Java method with old directives. A > request to recompile it with new directives will be ignored. > > Performance issues: > - Usually directives are updated for a small number of Java methods. If > CodeCache has thousands of nmethods, > `CodeCache::recompile_marked_directives_matches` will be traversing nmethods > most of which don't need recompilation. > > The backout is not clean because of removal of `CompiledMethod`. > > Tested with release and fastdebug builds: tier1 and tier2 passed. IMO if nobody uses it and the amount of code is small, it is better to back out it and to reimplement it. - PR Comment: https://git.openjdk.org/jdk/pull/19215#issuecomment-2107809381
Re: RFR: 8332111: [BACKOUT] A way to align already compiled methods with compiler directives
On Mon, 13 May 2024 14:34:50 GMT, Dmitry Chuyko wrote: > So there are cases when new functionality doesn't work as expected (I don't > see any other users impacted). Why not file bugs for those cases and estimate > their impact? Do you know any users using the new functionality? - PR Comment: https://git.openjdk.org/jdk/pull/19215#issuecomment-2107799744
Re: RFR: 8332111: [BACKOUT] A way to align already compiled methods with compiler directives
On Mon, 13 May 2024 13:52:17 GMT, Dmitry Chuyko wrote: > Are there any high severity problems caused by the original PR? Especially > not in the new functionality. Minor issues could be probably addressed > without backing out the entire functionality. Yes, there are: > 1. Usually directives are updated for a small number of Java methods. If > CodeCache has thousands of nmethods, > CodeCache::recompile_marked_directives_matches will be traversing nmethods > most of which don't need recompilation. > 2. has_matching_directives might not be cleared. > 3. A Java method is not recompiled as requested. - PR Comment: https://git.openjdk.org/jdk/pull/19215#issuecomment-2107720199
RFR: 8332111: [BACKOUT] A way to align already compiled methods with compiler directives
Backout of [JDK-8309271](https://bugs.openjdk.org/browse/JDK-8309271) which has known bugs, possible bugs and performance issues. Found bugs: - When refreshing `CompilerDirectivesAddDCmd::execute` will call `DirectivesStack::hasMatchingDirectives(mh, true)` which only considers the compiler directive which is on the top of the directives stack. As more than one directive can be added, `CompilerDirectivesAddDCmd::execute` will not behave as expected. - A Java method with old directives might be in the compilation queue. A request to recompile it with new directives will be ignored. There are other concerns: bugs and performance issues. Possible bugs: - `has_matching_directives` might not be cleared. A nmethod might get into the unloading state before `CodeCache::recompile_marked_directives_matches`. If the nmethod has been used to mark a Java method and it is the only nmethod, there will be no nmethod in CodeCache to reach the Java method to clear the mark. - A Java method might have been compiled with new directives before `CodeCache::recompile_marked_directives_matches`. `CodeCache::recompile_marked_directives_matches` will recompile it again. - JIT compiler might be compiling a Java method with old directives. A request to recompile it with new directives will be ignored. Performance issues: - Usually directives are updated for a small number of Java methods. If CodeCache has thousands of nmethods, `CodeCache::recompile_marked_directives_matches` will be traversing nmethods most of which don't need recompilation. The backout is not clean because of removal of `CompiledMethod`. Tested with release and fastdebug builds: tier1 and tier2 passed. - Commit messages: - 8332111: [BACKOUT] A way to align already compiled methods with compiler directives Changes: https://git.openjdk.org/jdk/pull/19215/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19215=00 Issue: https://bugs.openjdk.org/browse/JDK-8332111 Stats: 380 lines in 15 files changed: 3 ins; 347 del; 30 mod Patch: https://git.openjdk.org/jdk/pull/19215.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19215/head:pull/19215 PR: https://git.openjdk.org/jdk/pull/19215
Re: RFR: 8324241: Always record evol_method deps to avoid excessive method flushing [v5]
On Wed, 28 Feb 2024 07:32:47 GMT, RacerZ wrote: >> Volker Simonis has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Fixed whitepspace in flag documentation > > Will this feature be incorporated into a lower version of JDK, like JDK 8? @RacerZ-fighting > Will this feature be incorporated into a lower version of JDK, like JDK 8? I don't see JDK 8 or 11 in affected versions of JDK-8324241, only 17 and 21. - PR Comment: https://git.openjdk.org/jdk/pull/17509#issuecomment-1968772406
Re: RFR: 8324241: Always record evol_method deps to avoid excessive method flushing [v3]
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
lgtm
-
Marked as reviewed by eastigeevich (Committer).
PR Review: https://git.openjdk.org/jdk/pull/17509#pullrequestreview-1839792795
Re: RFR: 8324241: Always record evol_method deps to avoid excessive method flushing [v2]
On Mon, 22 Jan 2024 21:26:41 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:
>
> Guard the feature with a diagnostic option and update the comments in the
> code
src/hotspot/share/runtime/globals.hpp line 2014:
> 2012:
> \
> 2013: product(bool, AlwaysRecordEvolDependencies, true, DIAGNOSTIC,
> \
> 2014: "Unconditionally record method dependencies on class "
> \
"... record compiled method dependencies ..."?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17509#discussion_r1462553968
Re: RFR: 8324241: Always record evol_method deps to avoid excessive method flushing [v2]
On Mon, 22 Jan 2024 21:26:41 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:
>
> Guard the feature with a diagnostic option and update the comments in the
> code
src/hotspot/share/runtime/globals.hpp line 2013:
> 2011: "Profile exception handlers")
> \
> 2012:
> \
> 2013: product(bool, AlwaysRecordEvolDependencies, true, DIAGNOSTIC,
> \
As we record all dependencies not only evol_method ones, should we name it
just: `AlwaysRecordDependencies`?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17509#discussion_r1462550445
Re: RFR: 8324241: Always record evol_method deps to avoid excessive method flushing
On Mon, 22 Jan 2024 16:37:14 GMT, Aleksey Shipilev 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
>
>> Instead of unconditionally recording evol_method dependencies we could guard
>> the recording by a new flag. But this would only make sense if that flag
>> would be on by default and I don't know if such a flag is justified just for
>> the rare (or non-existent?) cases where somebody wants to disable the
>> recording of the dependencies.
>
> I think introducing a diagnostic flag is sensible here. If we figure out much
> later that this solution comes with some other (worse) problems, the
> diagnostic flag gives us the options to: a) clearly point at this addition as
> the culprit; b) have the easily deployable solution to restore the original
> behavior.
>
> For the change itself, we need to amend the comment near
> `VM_RedefineClasses::flush_dependent_code` definition that talks about this
> peculiar behavior, which now changes. Actually, maybe even the implementation
> of `flush_dependent_code` should now trust (and assert) that all dependencies
> are now recorded?
@shipilev I filed https://bugs.openjdk.org/browse/JDK-8324318 which is related
to this change.
-
PR Comment: https://git.openjdk.org/jdk/pull/17509#issuecomment-1904402169
Re: RFR: 8324241: Always record evol_method deps to avoid excessive method flushing
On Sat, 20 Jan 2024 19:48:07 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. The ration remains about the same i...
lgtm
-
Marked as reviewed by eastigeevich (Committer).
PR Review: https://git.openjdk.org/jdk/pull/17509#pullrequestreview-1836432196
Re: RFR: 8314029: Add file name parameter to Compiler.perfmap [v8]
On Mon, 11 Dec 2023 22:41:56 GMT, Yi-Fan Tsai wrote: >> `jcmd Compiler.perfmap` uses the hard-coded file name for a perf map: >> `/tmp/perf-%d.map`. This change adds an optional argument for specifying a >> file name. >> >> `jcmd PID help Compiler.perfmap` shows the following usage. >> >> >> Compiler.perfmap >> Write map file for Linux perf tool. >> >> Impact: Low >> >> Syntax : Compiler.perfmap [] >> >> Arguments: >> filename : [optional] Name of the map file (STRING, no default value) >> >> >> The following section of man page is also updated. (`man -l >> src/jdk.jcmd/share/man/jcmd.1`) >> >> >>Compiler.perfmap [arguments] (Linux only) >> Write map file for Linux perf tool. >> >> Impact: Low >> >> arguments: >> >> ยท filename: (Optional) Name of the map file (STRING, no >> default value) >> >> If filename is not specified, a default file name is chosen >> using the pid of the target JVM process. For example, if the pid is 12345, >> then >> the default filename will be /tmp/perf-12345.map. > > Yi-Fan Tsai has updated the pull request incrementally with one additional > commit since the last revision: > > Update copyright of PerfMapTest lgtm - Marked as reviewed by eastigeevich (Committer). PR Review: https://git.openjdk.org/jdk/pull/15871#pullrequestreview-1777620332
Re: RFR: 8314029: Add file name parameter to Compiler.perfmap [v5]
On Tue, 5 Dec 2023 20:13:11 GMT, Chris Plummer wrote: >> Hi Chris, >> The current design of `write_perf_map` provides a clean and explicit >> interface. The purpose of the function is evident from its signature: to >> write a perf map into a specified file. This explicitness makes the code >> more readable and self-documenting. It reduces the need for developers to go >> to the implementation to figure out: what is the meaning of `nullptr`; where >> a filename will be taken from. It also serves as a contract between the >> caller and the function itself. By explicitly requiring a filename, the >> function sets clear expectations for the caller. >> >> I think `CodeCache::write_default_perf_map` hiding the filename of the >> default perf map might not be a good idea because it makes impossible to get >> the filename used in it. I prefer either method >> `CodeCache::defaultPerfmapFileName()` or class >> `CodeCache::DefaultPerfmapFileName`. The class is simpler to implement than >> the method (like it was earlier). > > The default filename was already "hidden" before these changes, so at the > very least things are not being made any worse, but I don't see why any users > `write_perf_map` would ever need the default filename. I just felt that > adding and exporting a class whose only purpose is to provide the default > name seemed like unnecessary overkill. I'm not so sure having a public > CodeCache::defaultPerfmapFileName() API and two `write_perf_map` APIs isn't > overkill also. There is nothing wrong with a null filename argument signally > to use some default name. You can also have the filename arg default to > `nullptr`. Ok, let's have: void CodeCache::write_perf_map(const char* filename = nullptr); without any additional classes or funcitons. - PR Review Comment: https://git.openjdk.org/jdk/pull/15871#discussion_r1419537894
Re: RFR: 8314029: Add file name parameter to Compiler.perfmap [v5]
On Fri, 1 Dec 2023 21:25:19 GMT, Chris Plummer wrote:
>> Yi-Fan Tsai has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Apply man changes
>
> src/hotspot/share/code/codeCache.cpp line 1809:
>
>> 1807: }
>> 1808:
>> 1809: void CodeCache::write_perf_map(const char* filename) {
>
> Why not have a `filename == nullptr` indicate that the default should be
> used. Then you don't need CodeCache::DefaultPerfMapFile. You can just have a
> private `CodeCache::defaultPerfmapFileName()` method.
Hi Chris,
The current design of `write_perf_map` provides a clean and explicit interface.
The purpose of the function is evident from its signature: to write a perf map
into a specified file. This explicitness makes the code more readable and
self-documenting. It reduces the need for developers to go to the
implementation to figure out: what is the meaning of `nullptr`; where a
filename will be taken from. It also serves as a contract between the caller
and the function itself. By explicitly requiring a filename, the function sets
clear expectations for the caller.
I think `CodeCache::write_default_perf_map` hiding the filename of the default
perf map might not be a good idea because it makes impossible to get the
filename used in it. I prefer either method
`CodeCache::defaultPerfmapFileName()` or class
`CodeCache::DefaultPerfmapFileName`. The class is simpler to implement than the
method (like it was earlier).
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15871#discussion_r1415892122
Re: RFR: 8314029: Add file name parameter to Compiler.perfmap
On Thu, 21 Sep 2023 20:43:56 GMT, Yi-Fan Tsai wrote:
> `jcmd Compiler.perfmap` uses the hard-coded file name for a perf map:
> `/tmp/perf-%d.map`. This change adds an option for specifying a file name.
>
> The help message of Compiler.perfmap:
>
> Compiler.perfmap
> Write map file for Linux perf tool.
>
> Impact: Low
>
> Syntax : Compiler.perfmap [options]
>
> Options: (options must be specified using the or = syntax)
> filename : [optional] Name of the map file (STRING, no default value)
src/hotspot/share/code/codeCache.cpp line 1805:
> 1803: CodeCache::DefaultPerfMapFile::DefaultPerfMapFile() {
> 1804: // Perf expects to find the map file at /tmp/perf-.map.
> 1805: jio_snprintf(_name, sizeof(_name), "/tmp/perf-%d.map",
> os::current_process_id());
Please change the comment to:
// Perf expects to find the map file at /tmp/perf-.map.
// It is used as the default file name.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15871#discussion_r1334437849
