On Tue, 28 Jan 2025 00:08:27 GMT, Chen Liang <[email protected]> wrote:
>> Currently, to free the memory allocated in a confined arena, we keep track
>> of a list of 'cleanup actions', stored in linked list format in a so-called
>> `ResourceList`, attached to the scope of the arena. When the scope is
>> closed, we loop over all the entries in this resource list, and run all the
>> cleanup actions one by one.
>>
>> However, due to this linked list format, plus the control flow introduced by
>> the cleanup loop, C2's escape analysis can not keep track of the nodes of
>> this linked list (`ResourceList.ResourceCleanup`), and as a result, they can
>> not be scalar replaced.
>>
>> We can prevent just the first `ResourceCleanup` instance from escaping, by
>> pulling out the first element of the list into a separate field. I also
>> tried a setup where I had 2 separate fields for the first 2 elements, as
>> well as a setup with an array with a fixed set of elements. While these also
>> worked to prevent the first node from escaping, they were not able to
>> provide the same benefit for multiple resource cleanup instances.
>> Nevertheless, avoiding the allocation of the first element is relatively
>> simple, and seems like a low-hanging fruit.
>>
>> I've changed the `AllocTest` benchmark a bit so that we don't return the
>> `MemorySegment` in `alloc_confined`, which would make it always escape. That
>> way, we can use this existing benchmark to test whether there are any
>> allocations when calling `allocate` on a confined arena. This matches what
>> we were doing in the other benchmark methods in the same class.
>
> src/java.base/share/classes/jdk/internal/foreign/ConfinedSession.java line
> 112:
>
>> 110: if (fst != ResourceCleanup.CLOSED_LIST) {
>> 111: ResourceCleanup prev = fst;
>> 112: fst = ResourceCleanup.CLOSED_LIST;
>
> Is there a reason why usage of fst here doesn't prevent successful escape
> analysis?
Why do you think it should? We never assign the newly allocated resource
cleanup to `fst`.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/23321#discussion_r1931467325
