[foreign] RFR: simplify implementation classes

[Samuel Audet]

Hi, Maurizio,

Do the changes to LdLoader/LibraryLoader help in any way deal with the 
same library getting loaded by multiple class loaders? With JNI, it's 
not currently possible to load the same library twice by different class 
loaders and this kind of problem keeps coming up over and over again. 
Even projects like sbt and TensorFlow are still scratching their heads 
about what to do with this:
https://github.com/tensorflow/tensorflow/issues/19298

IMO, I think that fixing these kinds of usability issues is a lot more 
important than coming up with an alternative way to call native functions.

I can see 2 ways to fix this:
1. Do not try to associate native libraries with class loaders, and 
provide a System.unload() to let users manage manually the lifetime of 
native libraries, if they want.
2. Instead of throwing exceptions about native libraries being already 
loaded, increment a reference counter somewhere in the system class 
loader, and return success on System.load(), from wherever it gets called.

Would there be another way to deal with this? And what is Panama doing 
about this? These kinds of "little problems" is severely hurting the 
Java platform and should be fixed in priority over things like what to 
do with varargs and what not! What do you think?

Samuel

On 05/12/2018 02:15 AM, Maurizio Cimadamore wrote:
> Hi,
> it's spring cleaning in Panama-land :-)
> 
> Before moving forward with the revised design, I thought it would be 
> better to consolidate the internal implementation a bit. So I started 
> removing classes that didn't seem necessary - I ended up with a much 
> bigger changeset than I anticipated, but I think this is not bad news, 
> in fact I believe this is a big simplification over what we had. Here's 
> some highlights:
> 
> * I removed the PLatform class and all its OS-dependent Host subclasses. 
> The reality is that, at least for now, we only support SystemV ABI, and 
> little endianness everywhere. So having all these intermediate steps 
> seemed way too convoluted (and error prone as we learned earlier this 
> week). Now SystemABI has a getInstance method which returns the default 
> for that platform. For library loading, see below.
> 
> * Errno. This is also gone. Errno should be part of a wider Posix 
> implementation effort; there's little value in having it there. It was 
> only used by a test, (UnixSystem) which is easily fixable with 3-4 lines 
> of code.
> 
> * LdLoader/LibraryLoader. This is probably the biggest change. As we've 
> discussed we have a lot of custom logic that 'just' loads native 
> libraries. In reality the JDK already has code to do this, but the 
> problem is that it doesn't expose the library entry point back to the 
> Java code. I looked at how we might massage the JDK implementation, and 
> it's actually quite easy: ClassLoader stores loaded libraries in a class 
> called NativeLibrary, which has a (native) method to lookup entries, 
> etc. We can have this class implement our nicl/Library interface, and 
> then have ClassLoader.loadLibrary returns one of these.
> 
> There's one caveat: when we load  a new library, we need to pass a class 
> whose loader is used to 'hook' the native library (so that when the 
> classloader is GCed the native library is unloaded). Runtime.loadLibrary 
> achieves this with a @CallerSensitive, but that's only useful if the 
> client calls that code. Here we also need the binder to call that code - 
> so a saner approach is to pass a MethodHandles.Lookup - and use the 
> lookup class as the class whose loader is hooked to the native lib. That 
> works very well.
> 
> * As a result of the above class loader cleanup, we could get rid of 
> UnixLibrary and UnixDynamicLibraries too
> 
> * BindingRegistry: this is not used, and if we need something like it, 
> it will come back in a different shape
> 
> * ContainerSizeInfo: this is an interface which is only implemented by 
> AbstractABI. Seems like a case of premature abstraction.
> 
> * UncheckedPointer: this is used to create pointers w/o bounds - this is 
> just a factory method in BoundedPointer (which happened to be there 
> already: createNativeVoidPointer).
> 
> In total, this patch removes 14 classes, and, crucially, it removes the 
> duplication of the library loading logic. I suspect further cleanup is 
> also possible in Libraries/LibrariesHelper, but I'll leave this to 
> Sundar who's more familiar with that code and its interaction with 
> jextract. (I suspect some of the SecurityManager checks are not needed 
> anymore, since those would be carried out by the main class loader logic).
> 
> Webrev:
> 
> http://cr.openjdk.java.net/~mcimadamore/panama/cleanup/
> 
> Cheers
> Maurizio
>