OpenSSL and panama-foreign

Tue Dec 7 14:40:41 UTC 2021

Hi Maurizio,

Thanks a lot for the new ideas and discussion.

On Mon, Dec 6, 2021 at 12:06 PM Maurizio Cimadamore
<maurizio.cimadamore at oracle.com> wrote:
>
>
> > I have made some progress since then. Thanks for all the help !
> >
> > The Java 17 version now resides here:
> > https://urldefense.com/v3/__https://github.com/apache/tomcat/tree/main/modules/openssl-java17__;!!ACWV5N9M2RV99hQ!Z-4p41ECvTd-Gf3IWB2Y7qYBN0gMId9qKAXx2f5FgE00I0Pc4TdJ7hD97faRbwzTxmrN2Nw$  (it
> > should be integrated in the Tomcat binary soon since the jar is very
> > small and the Java 17 target is reasonable) and all seem well.
> > Actually, except that it is slapped as "super experimental early
> > preview" and will have to stay that way for a while, it looks a lot
> > better to me than the JNI code in tomcat-native.
> Hi Remy, thanks for trying out the bits and the feedback so far. Some
> replies
> >
> > I have the following feedback for the API:
> > - It seems bound upcalls will always leak memory, there's no way to
> > release them. Is a functional fix possible in the future ?
>
> In this case, the issue is that if you use implicit scope with an
> upcall, and the upcall has a strong reference (via its method handle)
> back to the scope, there is an (unavoidable, I think) issue: the method
> handle is stored somewhere (as a global JNI handle) in the VM stub. This
> is enough to keep the MH alive (which we need to), but also, indirectly
> (as the scope is attached to the MH in your case), to keep the scope
> alive. If we create a weak handle on the VM stub side, that would solve
> the issue in terms of the stub not keeping the reachability on the scope
> - but it will create other issue (e.g. an upcall crashing because the MH
> it refers to has disappeared). Jorn and I have thought about this case a
> bit - at the moment we don't have any clever ideas - I think using bound
> MHs with VM upcalls is something to be used with care, given
> implications on reachability.
>
> But we'll keep thinking of course.

The Java 17 code using an implicit scope looks decent to me right now.
Not optimal in every scenario, but functionally acceptable.
I'll refactor again the code that targets Java 18+ since I'm very
interested in better scope handling techniques.

Documenting clearly the GC roots created by each API call could be a
good idea. Limiting them as much as possible and allowing cleaning
them up would be a good bonus too.

> > - When using a close action attached to a scope (let's say an implicit
> > scope, which is what I am now using thanks to your good
> > recommendations), the action cannot use a reference to a memory
> > segment from the scope (it has to use a memory address instead),
> > otherwise the GC thinks it's a real reference to the scope and GC will
> > never happen.
>
> This is, I think, to be expected - as this is in line with
> recommendentation for cleanup action for Cleaners - e.g. that they have
> to be "static" so as not to capture reference to the object being collected.
>
> With the API, there is typically an option to "drop down" at the memory
> address level. In 18, a MemoryAddress is just a wrapper around a long,
> so it won't keep a scope alive. You can convert the address back to a
> segment if needed (MemorySegment::ofAddress) - not sure whether this
> will help your particular use case, but it's an option to keep in mind.
>
> > - On JVM shutdown, the implicit scopes don't seem to be closed, I only
> > get my close action called on a GC. Am I missing something (again ;) )
> > ?
>
> We register the scope against a cleaner, it is possible that cleaner
> actions are only called under GC pressure, but not on VM shutdown. I'd
> say that there's no guarantee as to _when_ these actions can be called
> (it's all GC implementation dependent).
>
> I think your question is: how do I create a memory resource that is
> destroyed implicitly on VM shutdown? We do not support that use case at
> the moment; an implicit scope is just a scope backed by a cleaner.
>
> > - I know it is planned, but right now the handling of heap memory with
> > segments is not very useful. One of the first things I tried is
> > MemorySegment.ofByteBuffer, and it worked great ... with a direct
> > ByteBuffer. This seemed like a good opportunity for a big optimization
> > since it would save a lot of byte copying, and the code as a whole
> > could likely stop using direct ByteBuffer.
> Not sure I follow here. Do you intend to say that
> MemorySegment.ofByteBuffer doesn't work with an heap byte buffer (that
> would be odd, but if true I'd like to know more!) - or that
> MemorySegment are not very useful when interacting with heap arrays
> (which I think has been addressed in 18) ?

My problem is already described by
https://bugs.openjdk.java.net/browse/JDK-8254693

> > - Could there be some opportunities for stronger typing with the code
> > that jextract generates ? I (not very surprisingly) experienced a few
> > bugs caused by the wrong address being passed to a call. Example:
> > could there be a Pointer<TYPE> as a "fake" type for a MemorySegment,
> > but providing strong typing ?
>
> We remain on the outlook for such opportunities - at the moment,
> introducing these types is not possible w/o a significant cost in terms
> of instances being created and/or additional overhead on memory
> dereference (or both). Some future language/VM features (Valhalla) might
> significantly change the peformance trade off, iin which case we will
> obviously reconsider.

Ok ! As of right now, I was not hoping for real Java types, you seem
to imply that's what you would like to have (if that's doable
eventually, this would be waaaaay beyond my wildest expectations !).
Instead, for now, I was only thinking about empty marker types, just
to give a compiler hint and produce errors (mostly when using the
wrong pointer type for a downcall) without really doing anything more.
Just a dumb code generation trick, essentially ...
For example: jextract generates an "SSL" empty class (which I filter
out since it's not actually useful). Instead of doing nothing with it,
this marker type could be matched to the native code signatures and
used in the generated code as a generic to a type that extends
MemoryAddress/Segment.

Rémy

>
> Thanks
> Maurizio
>
> >
> > Rémy
> >
> >
> >>
> >> On 11/11/2021 15:26, Maurizio Cimadamore wrote:
> >>> I'm not sure I follow your point entirely.
> >>>
> >>> But yes, if you do a bindTo(this), then using an implicit scope is not
> >>> very useful as long as the scope is reachable from `this`.
> >>>
> >>> We might be able to refine the implementation and improve this in the
> >>> future, but for now I think that's something we have to live with.
> >>>
> >>> Thanks
> >>> Maurizio
> >>>
> >>>
> >>> On 11/11/2021 14:41, Rémy Maucherat wrote:
> >>>> Unless I missed something the API has a side effect with upcalls. I
> >>>> understand after reading the javadoc where the suggestion comes from,
> >>>> as it makes memory management very much like Java. This is of course
> >>>> excellent. However, I use upcalls (as you know) and many of them are
> >>>> bound to an instance (since this makes state tracking very very nice).
> >>>> This however creates a reference that never goes away and an implicit
> >>>> scope is never going to get GCed and closed (I saw that with visualvm)
> >>>> and memory leaks. Initially I was planning to use a static Map<Long,
> >>>> State> for my state (OpenSSL allows passing references and getting
> >>>> them in callbacks, but this is a lot handier in C and doesn't make a
> >>>> lot of sense in Java), then I found MethodHandle.bindTo actually
> >>>> worked and this is quite magical.
> >>>>
> >>>> So with that in mind, what is the recommendation ? Should the API
> >>>> provide a way to remove the MethodHandle upcall ? Or should I avoid
> >>>> using bindTo instead ?