RFR: 8358573: Remove the -XX:-InstallMethods debug flag [v2]
Aleksey Shipilev
shade at openjdk.org
Thu Jul 17 09:38:59 UTC 2025
On Wed, 16 Jul 2025 13:49:56 GMT, Benoît Maillard <bmaillard at openjdk.org> wrote:
>> This PR prevents from hitting an assert when disabling method installation at the end of a successful compilation with the `-XX:-InstallMethods` flag. Previously `CompileBroker` failed to mark the `CompileTask` as complete when using this flag.
>>
>> ## Analysis
>>
>> We can see that the assert is triggered in `CompileBroker::invoke_compiler_on_method`:
>>
>> ```c++
>> if (!ci_env.failing() && !task->is_success()) {
>> assert(ci_env.failure_reason() != nullptr, "expect failure reason");
>> assert(false, "compiler should always document failure: %s", ci_env.failure_reason());
>> // The compiler elected, without comment, not to register a result.
>> // Do not attempt further compilations of this method.
>> ci_env.record_method_not_compilable("compile failed");
>> }
>>
>>
>> The `task->is_success()` call accesses the private `_is_success` field.
>> This field is modified in `CompileTask::mark_success`.
>>
>> By setting a breakpoint there, and execute the program without `-XX:-InstallMethods`, we get the following stacktrace:
>>
>> CompileTask::mark_success compileTask.hpp:185
>> nmethod::post_compiled_method nmethod.cpp:2212
>> ciEnv::register_method ciEnv.cpp:1127
>> Compilation::install_code c1_Compilation.cpp:425
>> Compilation::compile_method c1_Compilation.cpp:488
>> Compilation::Compilation c1_Compilation.cpp:609
>> Compiler::compile_method c1_Compiler.cpp:262
>> CompileBroker::invoke_compiler_on_method compileBroker.cpp:2324
>> CompileBroker::compiler_thread_loop compileBroker.cpp:1968
>> CompilerThread::thread_entry compilerThread.cpp:67
>> JavaThread::thread_main_inner javaThread.cpp:773
>> JavaThread::run javaThread.cpp:758
>> Thread::call_run thread.cpp:243
>> thread_native_entry os_linux.cpp:868
>>
>>
>> We go up the stacktrace and see that in `Compilation::compile_method` we have:
>>
>> ```c++
>> if (should_install_code()) {
>> // install code
>> PhaseTraceTime timeit(_t_codeinstall);
>> install_code(frame_size);
>> }
>>
>>
>> If we do not install methods after compilation, the code path that marks the success is never executed
>> and therefore results in hitting the assert.
>>
>> ### Fix
>> <del>We simply mark the task as complete when `should_install_code()` evaluates to `false` in the block code above.</del>
>> After careful consideration, it was decided to simply get rid of the `-XX:-InstallMethods` flag.
>>
>> ### Testing
>> - [x] [GitHub Actions](https://github.com/benoitmaillard/jdk/actions?query=branch%3AJDK-8358573)
>> - [...
>
> Benoît Maillard has updated the pull request incrementally with three additional commits since the last revision:
>
> - 8358573: get rid of InstallMethods flags completely
> - Revert "8358573: Add missing task success notification"
>
> This reverts commit cd91c7c06ba05aba3500b95ba1317539363aa63c.
> - Revert "8358573: Add test for -XX:-InstallMethods"
>
> This reverts commit 6eab84718c3b60c2585bc2711c4bc8144472975b.
I agree CTW does not need this flag. We have (or will have) enough internal APIs to avoid installing the code, if we need it for any reason.
-------------
Marked as reviewed by shade (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/26310#pullrequestreview-3028738102
More information about the hotspot-compiler-dev
mailing list