On Fri, 28 Jun 2024 15:15:31 GMT, Damon Fenacci <dfena...@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
