RFR: 8333891: Method excluded with directive is not compiled after removal of directive [v2]

[Evgeny Astigeevich]

On Fri, 28 Jun 2024 15:15:31 GMT, Damon Fenacci <dfenacci at openjdk.org> wrote:

>> Evgeny Astigeevich has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Fix data race
>
> src/hotspot/share/compiler/compileBroker.cpp line 1336:
> 
>> 1334:   }
>> 1335: 
>> 1336:   AbstractCompiler *comp = CompileBroker::compiler(comp_level);
> 
> Do we need to change this?

Not really. I'll change it back to the original code.

> src/hotspot/share/compiler/compileBroker.cpp line 1563:
> 
>> 1561: // See if this compilation is not allowed.
>> 1562: bool CompileBroker::compilation_is_prohibited(const methodHandle& method, int osr_bci, int comp_level) {
>> 1563:   assert(compiler(comp_level) != nullptr, "Ensure we have a compiler");
> 
> Are you sure `compiler(comp_level)` can never be `nullptr` (not even potentially)? (it seems so to me too but just to make sure)

Yes, I am sure.
This is a private member function which is only used in `CompileBroker::compile_method`.
In `CompileBroker::compile_method`  there has always been an assert checking availability of a compiler. I added the assert here just in case the function is used in any other functions where a compiler might not be available.

If we want to minimize changes, we can keep the original code where the  parameter `excluded` is only deleted. The checks `comp == nullptr` are redundant in the original code because the function is invoked as follows:

if (comp == nullptr || compilation_is_prohibited(...)) {
    return nullptr;
}


I can add a check `comp == nullptr` at the beginning of the function and return `nullptr` if it is true.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/19637#discussion_r1661286156
PR Review Comment: https://git.openjdk.org/jdk/pull/19637#discussion_r1661319021