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

[Jorn Vernee]

Maurizio Cimadamore schreef op 2019-06-04 01:39:
> 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.

I thought optimization was the whole point of adding the thread 
confinement... From [1]:

> To be able to get to the same level of performances as Unsafe::get/put, 
> we need to be able to hoist the liveness check away in the JIT; but 
> there's a problem here: even if the JIT can see through a bunch of code 
> using addresses allocated by a given scope, it has to conservatively 
> assume that another thread might chime in, and close the scope behind 
> our back.
> 
> This makes it impossible for the JIT to completely optimize away the 
> check. For this reason, in my document [...] I posited the existence of 
> a 'confined' scope, whose existence is bound to a given thread (and 
> maybe, in the future, a fiber).

If we have an intermediate and public state that is non-confined, and 
then the confined state is just a view over this non-confined state, the 
JIT will still have to conservatively assume that another thread has 
access the the non-confined state and can close the scope behind our 
back AFAIK. But, if we design our API in such a way that the 
non-confined state is not observable this should not be a problem, and 
we have a guarantee that a thread confined segment is actually thread 
confined.

>> 
>>> 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).

I agree with an opt-in for shared segments. This was more of a 
hypothetical of how such a thread-safe segment could be implemented :)

Jorn

[1] : 
https://mail.openjdk.java.net/pipermail/panama-dev/2019-May/005494.html

> Maurizio
> 
>> 
>> Jorn
>> 
>>> Maurizio