[foreign] RFR: simplify implementation classes

[Sundararajan Athijegannathan]

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

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