Comments on SegmentAllocator and ResourceScope

[Maurizio Cimadamore]

On 16/05/2021 16:39, Stig Rohde Døssing wrote:
> Hi,
>
> JEP 412 adds a new interface for allocating segments called
> SegmentAllocator (great idea!), and I have a few questions regarding the
> API and implementation as they exist in the foreign-jextract branch.
>
> 1: The ArenaAllocator block and max alloc sizes are hard coded. Is this
> intended to be permanent, or will the block size become a parameter of
> ArenaAllocator later? I'm not sure why it makes sense to hard code these
> parameters? As far as I can tell, this will cause issues for at least the
> bounded arena allocator, since requests above MAX_ALLOC_SIZE would cause
> calls to newSegment, which throws an error.

I don't see anything preventing us from overriding defaults. We just 
didn't offer the option at this stage.

As for the issue with MAX_ALLOC_SIZE and bounded allocation, this is a bug.

>
> 2: The MAX_ALLOC_SIZE constant also seems strange. It is set to half the
> block size, and any requests for segments above that size cause a fresh
> segment to be allocated. I understand why it makes sense to allocate a new
> segment if the request is larger than the block size, but if the request is
> smaller than the block size, why is not good enough to check if there's
> room in the current segment, and if so, return the appropriate slice, even
> if the request exceeds half the block size?
>
> See
> https://github.com/openjdk/panama-foreign/blob/foreign-jextract/src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/ArenaAllocator.java#L14-L15

I believe this is yet another policy that might need to be made 
configurable at some point (e.g. threshold for standalone allocation). 
But using a single constant is making the code more obscure. I'm also 
noting comments from Michael, and there's probably simplifications that 
can be done here in order to make code more linear.


>
> 3: Is the SegmentAllocator interface intended to support more advanced
> memory pooling, such as pools where segments can be returned to the pool
> after use? If so, how will the application indicate to the pool that a
> segment can be returned? The Javadoc for MemorySegment and ResourceScope
> discourage implementations outside the JDK, so the pool will not be able to
> e.g. use wrapper classes to make "freed" segments return to the pool.

The interfaces allow to implement memory pools, as is. See this attempt:

https://github.com/openjdk/panama-foreign/pull/509

We plan to investigate this space more in the future, but I believe the 
API should be expressive enough to build pooled allocators which perform 
decently.

>
> 4: ResourceScope has the Handles concept, which are objects that are
> acquired from the scope, which will cause the "close" method to fail (and
> throw exceptions) until they are released. Would it make sense to make
> Handles Autocloseable, to allow them to be used with try-with-resources? It
> seems like it is always an error to acquire and not release a handle.

We started from AutoCloseable - but ended up abandoning it. In reality, 
if you try doing something like:

try (scope.acquire()) {
    ...
}

you get a compiler warning as the "resources" in the try-with-resources 
has not been used. At the time the has been some discussion around the 
role of resources in a try with resource block, which ended up 
concluding that the above usage was a bit at the boundary of what the 
construct is for.

In the end, we decided to side-step try with resources and stick to 
plain try/finally, and have a symmetric pair of methods acquire/release.

>
> 5: The ResourceScope Javadoc indicates that Handles exist to help avoid
> concurrency errors where one thread frees memory currently being accessed
> by another thread. It says that the thread closing the scope should not
> repeatedly call ResourceScope.close until no exceptions are thrown, but
> should instead use proper synchronization mechanisms, e.g. Handles to
> coordinate. How will the thread closing the scope use handles to
> coordinate? I don't see any methods on ResourceScope that allows the
> closing thread to avoid calling "close" on a scope with open handles. Will
> the closing thread just have to call "close" and hope for the best?

As Michael says, what the javadoc is trying to say here is that if you 
have 10 threads racing to the same segment, rather than accessing memory 
and closing it and hope that no exceptions are thrown (or worse, wait 
until no exceptions are thrown), these thread should synchronize among 
themselves. E.g. the fact that our API guarantees that the VM won't 
crash if you access an already freed segment, or if you attempt to close 
a segment while it's being access, does _not_ mean that developers can 
forget about regular inter-thread synchronization. This doesn't mean 
using ResourceScope.Handle. This means using a lock, or a blocking 
queue, or some other synchronization mechanism.

Thanks for the feedback
Maurizio

>
> Thanks for reading.