[foreign] RFR: simplify implementation classes

[Maurizio Cimadamore]

On 14/05/18 13:30, Sundararajan Athijegannathan wrote:
> Thanks for taking care of avoiding public API in Runtime.
>
> I think we need to revisit that Lookup argument in Libraries API - for 
> eg. we may want to filter publicLookup().
I agree - looking at that in more details, I see that one thing that is 
currently preventing the code to be tighter is the fact that 
Libraries.loadLibrary takes a lookup, whereas Libraries.bind/bindRaw do 
not. This is inconsistent - given that, at some point, bind will end up 
loading some native libraries, at which point some lookup object should 
be used.

I will try to address that in a followup patch, as there's enough going 
on here :-)

Maurizio
>
> -Sundar
>
> On 14/05/18, 4:14 PM, Maurizio Cimadamore wrote:
>>
>>
>> On 14/05/18 06:09, Sundararajan Athijegannathan wrote:
>>> Quick comment on java.lang.Runtime change. We can avoid a new public 
>>> API in Runtime by using SharedSecrets pattern 
>>> (jdk.internal.misc.SharedSecrets). I think it is better to avoid 
>>> exposing Libray/Symbol/Pointer etc. via j.l.Runtime.  For now, we 
>>> could have the method in Runtime as pure implementation helper 
>>> (though we may choose to expose a new API method in Runtime class 
>>> later).
>> Thanks for the comment - here's a revised webrev that used the 
>> SharedSecrets trick:
>>
>> http://cr.openjdk.java.net/~mcimadamore/panama/cleanup-v2/
>>
>> Maurizio
>>>
>>> -Sundar
>>>
>>> On 11/05/18, 10:45 PM, 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
>>>>
>>