RFR: 8308231: Faster MemAllocator::Allocation checks for verify/notification [v2]

[Stefan Karlsson]

On Fri, 19 May 2023 07:31:02 GMT, Aleksey Shipilev <shade at openjdk.org> wrote:

>> src/hotspot/share/gc/shared/memAllocator.cpp line 75:
>> 
>>> 73:   //   - (optionally) the enabled JVMTI event that wants to capture all allocations;
>>> 74: 
>>> 75:   bool should_notify_allocation_no_jvmti_vmobjalloc() {
>> 
>> As I understand this check is specific for SampledObjectAlloc event. Might be better to name it like: should_notify_jvmti_sampled_object_alloc? 
>> 
>> Does it make sense to check should_post_sampled_object_alloc also here? It is usually disabled.
>
> No, this check verifies everything, _but_ `JvmtiExport::should_post_vm_object_alloc()`, see the full method below. So in `MemAllocator::Allocation::notify_allocation_jvmti_sampler` can actually be called for two reasons: sampling event is required, or VMObjectAlloc is required. The `should_notify_allocation_no_jvmti_vmobjalloc` disambiguates the case where we don't need to proceed with sampling event gathering. 
> 
> Honestly, I can just revert this hunk, but I think it is cleaner to expose the helper:
> 
> 
> @@ -187,9 +206,8 @@ void MemAllocator::Allocation::notify_allocation_jvmti_sampler() {
>      return;
>    }
>  
> -  if (!_allocated_outside_tlab && _allocated_tlab_size == 0 && !_tlab_end_reset_for_sample) {
> -    // Sample if it's a non-TLAB allocation, or a TLAB allocation that either refills the TLAB
> -    // or expands it due to taking a sampler induced slow path.
> +  if (!should_notify_allocation_no_jvmti_vmobjalloc()) {
> +    // Called here only for JVMTI VMObjectAlloc event
>      return;
>    }

FWIW, I found the name `should_notify_allocation_no_jvmti_vmobjalloc` confusing when I read this patch. Maybe a better name would be `allocated_in_slow_path()` or negate it to `!allocated_in_fast_path()`?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/14019#discussion_r1200334550