[crac] RFR: Propagating CRaC event to JVMTI agents [v3]

[Roman Marchenko]

On Thu, 23 Nov 2023 10:11:02 GMT, Anton Kozlov <akozlov at openjdk.org> wrote:

>> Roman Marchenko has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Fixed test
>
> src/hotspot/share/prims/jvmtiEventController.cpp line 1:
> 
>> 1: /*
> 
> I see some potential places to modify, e.g. line 699. It would make sense at least place a comment where new extenstion eventns are intentionally not added.

"`should_post_NNN`" API is created for JDK classes/entities, which are originators of NNN event, to make them deciding if they need to post events or not - if no one subscribes, or no subscriber wants to receive it, they shouldn't. I guess this was done for JDK performance purposes.

For checkpoint/restore, we could create similar API to check if `crac::checkpoint()` should post an event or not, but currently it seems unecessary in my opinion, because CRaC events aren't events are posted dozens times per second or more. It may make sense in case of hundreds of JVMTI agents not supposing to consume CRaC events - in this case, posting an event will iterate all JVMTI agents with no payload. For now it seems like one more place for conflicts in future merging.

> src/hotspot/share/prims/jvmtiEventController.cpp line 829:
> 
>> 827:     case EXT_EVENT_CRAC_BEFORE_CHECKPOINT :
>> 828:       ext_callbacks->CracBeforeCheckpoint = callback;
>> 829:       env->env_event_enable()->set_user_enabled(event_type, enabling);
> 
> Why UNLOAD/MOUNT/UNMOUNT do not require this?

For EXT_EVENT_CLASS_UNLOAD, it was here from the very beginning (https://github.com/openjdk/crac/blob/8153779ad32d1e8ddd37ced826c76c7aafc61894/hotspot/src/share/vm/prims/jvmtiEventController.cpp#L693), until VitualThreads implementation changed it. 
Now a consumer (an agent) should set a callback for an event, and then can control recieving an event by enabling/disabling it with a call of `SetEventNotificationMode ` function. I guess, this was done to make the event control flow easier for a consumer. This makes sense in case an event is supposed to posted regularly, that is not a case for CRaC. Again, we could implement a similar way, but it still seems like one more place for conflicts in future merging.

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

PR Review Comment: https://git.openjdk.org/crac/pull/143#discussion_r1403493687
PR Review Comment: https://git.openjdk.org/crac/pull/143#discussion_r1403494354