[foreign] RFR: simplify implementation classes

[Sundararajan Athijegannathan]

Sounds good to me.

-Sundar

On 14/05/18, 5:59 PM, Maurizio Cimadamore wrote:
>
>
> 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
>>>>>
>>>
>