RFR: 8257967: JFR: Events for loaded agents [v16]

[Serguei Spitsyn]

On Wed, 12 Apr 2023 10:43:31 GMT, Markus Grönlund <mgronlun at openjdk.org> wrote:

>> src/hotspot/share/prims/jvmtiAgent.cpp line 323:
>> 
>>> 321:   assert(agent != nullptr, "invariant");
>>> 322:   if (!agent->is_loaded()) {
>>> 323:     if (!load_agent_from_executable(agent, on_load_symbols, num_symbol_entries)) {
>> 
>> It feels like I'm missing something.
>> We already checked and found at line 322 that `agent->is_loaded() == false`.
>> Also, we have the comment at line 265:
>> 
>> 265 // If this function returns true, then agent->is_static_lib().&& agent->is_loaded().
>> 266 static bool load_agent_from_executable(Agent* agent, const char* on_load_symbols[], size_t num_symbol_entries) {
>> 
>> As the `agent->is_loaded() == false` then t he condition `agent->is_static_lib() && agent->is_loaded()` has to be `false` and can not be `true`. Then one of the if-checks at lines 322 and 323 is not needed and can be removed. Is it right? Otherwise, the comment at line 265 can be incorrect.
>
> Good observation, Serguei.
> 
> It is because some paths call into lookup_On_load_Entry_point() twice.
> 
> It is primarily the attempted conversion of xrun agents, the first invocation comes from JvmtiAgent::convert_xrun_agent(). This will have the agent "loaded". If there is an Agent_OnLoad function, the agent is converted (i.e. xrun removed).
> 
> Then when the agent is to invoke the Agent_OnLoad function, there is a second invocation. Here a converted xrun library is already loaded, so I bypass attempting to load it again by checking the is_loaded() property.

Thanks.

>> src/hotspot/share/prims/jvmtiAgent.cpp line 357:
>> 
>>> 355:       vm_exit_during_initialization("Could not find JVM_OnLoad or Agent_OnLoad function in the library", name());
>>> 356:     }
>>> 357:     _xrun = false; // converted
>> 
>> Just questions to understand it better.
>> Neither `JVM_Onload` nor `Agent_Onload` entry points are stored after these lookups. It means that in order to be called later (as the comment at line 350 says) they have to be looked up again.
>> Is it right? Was it the same originally?
>
> The entry points are not saved and so have to be looked up again. It was the same originally.
> 
> That is why there is a check and branch on agent->is_loaded().

Thank you. I'm okay with it.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/12923#discussion_r1165306181
PR Review Comment: https://git.openjdk.org/jdk/pull/12923#discussion_r1165307170