MemorySegment.close(), threads

[Michael Zucchi]

On 15/1/20 10:08 pm, Maurizio Cimadamore wrote:
>
> On 15/01/2020 11:00, Michael Zucchi wrote:
>>
>> Evening,
>>
>> Has it been considered to have MemorySegment.close() work on any thread?
>>
> You can share a segment across multiple threads using the 
> MemorySegment::acquire method. The workflow is as follows:
>
> 1) master thread creates a segment
> 2) many worked threads acquire the segment
> 2a) at this point master thread can no longer close the segment (since 
> there's threads using it)
> 3) worked threads do work on their segment (slice?) and then close 
> their acquired segment - closing an acquired segment does NOT deallocate
> 4) once all the acquired segments have been closed by the worked 
> threads, the master thread can proceed closing the original segment 
> for good (which will deallocate)
>
> The main reason for enforcing such a workflow (or _any_ workflow) is 
> that we want to avoid race conditions where a thread accesses a 
> segment while another closes it, which, again would lead potentially 
> to a hard VM crash (which the API wants to avoid).
>
> Slight variations on this protocol have been suggested during the code 
> review for JDK 14 integration where all threads are on an equal 
> footing, and the last one closing the segment triggers deallocation. 
> This is also possible to achieve, we need real world validation to 
> guide us in picking which flavor would be more useful most of the times.
>
Yep this describes the process and rationale well.  I guess you've 
implemented reference counting for java objects here which is 
inconvenient in a java context, but mostly workable.  Of course real c 
pointers or those memoryaddresses from native functions have none of 
these restrictions whatsoever, and are never checked from java or c.

So yes it can be managed, all except close() which is where the hard rub 
is.  On further thought I can see there are races with bad usage if it 
were to be made free threaded, given the current design.

But If every thread had to call acquire()/close() every use, then close 
could be free threaded.  I presume that is more or less what your last 
paragraph above covers since that would treat all uses as equal.  It 
could be made cheaper if segments had no owner and acquire/release was 
just an atomic op.

>> But SystemABI.upcallStub() can't be changed or copied which makes a 
>> few things inconvenient like lambdas caching such stubs (as they can 
>> have no close() method), or any function/object that has a setter for 
>> a callback.  Such a setter needs to keep the callback stub around for 
>> freeing later - either when the setter is invoked again or the object 
>> that uses it is closed and limiting that to the originating thread 
>> seems overbearing.
>
> And upcall stub is a pointer into some piece of JVM-allocated code. We 
> can't let you peek to much into that - e.g. if you were able to copy 
> arbitrary bits inside the VM generated stub, you could corrupt the VM 
> state.
>
Yep this is understood, can be mostly worked around even if you just 
recreate them every time.  Again the free must be on creator thread is 
the main pain point.

>
>> As an aside, a zeroing allocateNative would also be useful just as it 
>> is in C: for convenience, and security.
>
> allocateNative zeroes by default, for the reasons you mentioned. There 
> is a runtime property you can set to disable zeroing 
> ("-Djdk.internal.foreign.skipZeroMemory=true"), and in the future, as 
> we have plans to open up some of the allocation machinery, there will 
> probably be allocateNative overloads with different flavors.
>
Oh ok, does it?  The documentation says it doesn't and I recall seeing 
some odd results when i didn't initialise all fields.  At least that's 
the way i read it from the source:


     /**
      * Creates a new native memory segment that models a newly 
allocated block of off-heap memory with given size (in bytes).
      * <p>
      * This is equivalent to the following code:
      * <blockquote><pre>{@code
     allocateNative(bytesSize, 1);
      * }</pre></blockquote>
      *
      * @implNote The *initialization state of the contents of the block 
of off-heap memory associated with the returned native memory**
**     * segment is unspecified and should not be relied upon*. 
Moreover, a client is responsible to call the {@link MemorySegment#close()}
      * on a native memory segment, to make sure the backing off-heap 
memory block is deallocated accordingly. Failure to do so
      * will result in off-heap memory leaks.
      *
      * @param bytesSize the size (in bytes) of the off-heap memory 
block backing the native memory segment.
      * @return a new native memory segment.
      * @throws IllegalArgumentException if {@code bytesSize < 0}.
      */


Cheers,
  Michael