RFR: 8358573: Remove the -XX:-InstallMethods debug flag [v2]

[duke]

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.

@benoitmaillard 
Your change (at version 2da73a5df46f47addc9ae6a9d32c69be1a9fc2a2) is now ready to be sponsored by a Committer.

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

PR Comment: https://git.openjdk.org/jdk/pull/26310#issuecomment-3083877893