[foreign-jextract] RFR: MemorySegmentPool + Allocator [v6]

[Maurizio Cimadamore]

On Wed, 21 Apr 2021 07:07:56 GMT, Radoslaw Smogura <github.com+7535718+rsmogura at openjdk.org> wrote:

>> src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemorySegmentPool.java line 179:
>> 
>>> 177: 
>>> 178:   public static class MemoryPoolSegment extends SpinLockQueue.Entry<MemoryPoolSegment> implements AutoCloseable {
>>> 179:     private final NativeMemorySegmentImpl memorySegment;
>> 
>> I think this should be MemoryAddress, and the scope should be "late bound" when a segment is retrieved from the queue and requested by a client. Otherwise clients get segments with a different scope than the one they are in - which in itself is ok, but I think it's counterintuitive for an allocator associated with a scope to return segments that last longer than that scope. 
>> 
>> In fact I'm not even 100% that you need a scope for the pool itself (which could mean that the acquire is not needed).
>
> I made a performance test, and it looks like creating new segment adds around 8ns to the test results (grow from 32 to 40).
> 
> I wonder if this would be a must have for direct pool API. Direct pool does not have to be associated with new scope.
> 
> However, for allocator version I think this should be done, as it can prevent leaking shared segment.

It seems like I misunderstood some of what was going on in the patch. I think I was confused by the fact that memory didn't seem to be ever reclaimed - this led me to the conclusion that having a scope for the pool was redundant. But then, when inspecting a running application closer, I noted that segments are left in the pool - even if the scope is closed (in your original patch). That's because you use CLinker::allocate memory to allocate the segment, which is ok, but then there's never a cleanup action that goes from the pool scope closure to the closure of the segments in the pool. This is, I think, a bug.

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

PR: https://git.openjdk.java.net/panama-foreign/pull/509