[foreign-memaccess] RFC: to scope or not to scope?

[Maurizio Cimadamore]

On 03/06/2019 23:13, Jorn Vernee wrote:
> Maurizio Cimadamore schreef op 2019-06-03 22:54:
>> On 01/06/2019 16:50, Jorn Vernee wrote:
>>> For asConfined() creating a view segment would not work, since the 
>>> root segment, which holds the liveliness flag, would not be confined 
>>> to a single thread. For asConfined to work as expected I believe we 
>>> would have to kill the root segment, effectively transferring the 
>>> underlying resource to the newly created confined segment. This also 
>>> brings up an idea I had been thinking about; make thread 
>>> confined-ness a more local, temporary state. e.g. you call 
>>> asConfined which creates a segment that is confined to the current 
>>> thread, but then add a release() method that invalidates the 
>>> confined copy and makes the root segment valid again. This could be 
>>> used around loops, e.g.:
>>
>> I don't think I agree with this line of thought. The use I have in
>> mind for this API is something like:
>>
>> try (MemorySegment segment = 
>> MemorySegment.ofNative(bytes).asConfined()) { ... }
>>
>> If you do this, then inside the try with resources you have the
>> guarantee that everything will be confined.
>
> This seems to be excluding the case where the MemorySegment is stored 
> in a field, and then used in a loop. I'd deem that case quite likely, 
> and I think there's a solution possible that works for both use-cases 
> without adding too much complexity.

Can't you do same for field?

MemorySegment segment = MemorySegment.ofNative(bytes).asConfined();


If you do this, who exactly will be able to access the non-confined 
temporary root we have created?

>
> Also, when we have a snippet like this, the liveliness check should 
> already be hoisted, assuming the loop body is also inlined, since the 
> executing thread is the only one that has access to the liveliness flag.
>
>> We could of course enforce a no-access state by default, or
>> confined-by-default, but IMHO that's just adding additional hops for
>> the (likely) case where these things don't make a difference. And, if
>> you really want to make sure that what you have is confined, and
>> there's no racy access possible, just call the view method as soon as
>> you allocate the segment (as above), then nobody will be able to
>> access to the non-confined version.
>>
>> This is not different from how ByteBuffer::asReadOnly works - and I
>> think that's a pragmatic compromise. That API too will have the same
>> issues discussed here (if somebody keeps hold of the original BB, you
>> can still write on a supposedly read-only buffer), but I wonder how
>> much that is really a problem in practice, given that it can be
>> avoided with some discipline.
>
> Yes, but the JIT can not really assume that the programmer has this 
> discipline. I'm not sure if we can prove that there's only a single 
> thread accessing a segment (or rather, the liveliness flag) when this 
> is not guaranteed by the API. Which would be the case if a confined 
> segment is simply a view over a non-confined segment I think.

Not sure I follow and this seems to conflate API aspects with 
optimization aspects. From liveness perspective, only one thread can 
change the liveness bit. This is true even in the patch I've sent. Each 
segment has an owner thread, and only the owner can close the region. 
Confinement only add extra enforcements when accessing the region with 
read/write operatons.

Also, from an optimization level, I'm not aware of the JIT having any 
issue of 'not trusting' that every access comes from same thread. The 
JIT doesn't worry about that - and if there are issues, these are 
typically multi-threading bug in user-land (e.g. users forgot to add 
synchronization where they needed to). Vlad can correct me if I'm wrong, 
but that is my understanding.

>
>> As much as I agree with the theory that returning a no-access segment
>> is the only way to make the API really tight, at the same time, I
>> having factory methods that return what would effectively be unusable
>> segments seems a weird API choice to me. At this point I think I would
>> prefer having some 'flags' to be passed to the segment factory (pretty
>> much like we had scope charateristics before).
>
> This seems good to me for the current goal we're trying to achieve; 
> making the liveliness check hoist-able. I think of the local acquire + 
> release strategy as a different way to get there.

I think the problems with making the liveliness check hoistable have 
nothing to do with confinement guarantees, really - again I'll defer to 
Vlad for more on this.

To me the #1 problem is a user model one - as a creator and user of a 
segment, do I want to worry about other threads closing the segment 
behind my back? I don't think we want that. And that is what leads to 
having some critical operations being confined. I think the JIT mostly 
doesn't care about all this.

>
> For fully fledged memory resource sharing between threads I agree with 
> John that we should disallow access by multiple threads at the same 
> time. So the state between allocating a segment and it being acquired 
> by a single thread would be 'useless', but we could use a separate 
> type that doesn't derive from MemorySegment as well for this right? 
> That shouldn't be too weird...

I have problems with this view. On the one hand we have a very rich 
VarHandle API which allows very fine-grained control over concurrent 
access. On the other hand this is saying: sorry, only one thread at a time.

There will be cases, for sure, where people will be happy with 'one 
thread at a time'. But there will be other cases where people won't 
care, or where people would like to go racy, and handle things 
themselves (this is, after all, a low level API). I honestly don't see 
the need for such hand-holding decisions here. I think a racy default is 
perfectly fine (after all, that's what users of Unsafe are familiar 
with) - if you want more protection, ask for it (but also know that it 
might come at extra cost).

I'm very worried about second guessing how users might use this very 
general API, and trying to enforce general model of usage - which might 
work well in certain cases, but utterly fail in others. If we start 
simple, sticking with what's in Unsafe, we can build an API that works, 
and the we can add stability guarantees as optional add-ons - this way 
the performance model is clear - you pay what you get. So you can stick 
with raw and racy (and fast), or go with confined and safe (and possibly 
less fast).

Maurizio

>
> Jorn
>
>> Maurizio