[foreign] RFR: cleanup binder code generation

[Maurizio Cimadamore]

Thanks, I will fix the copyright.

As for the leak, I agree, there is a potential issue (dunno how eagely 
j.l.Class fetches and stores its superinterfaces).

We will need to look into that at some point. It would be tempting to 
just ditch the cache, but I think the performance model of the binder 
relies a lot on there being only one implementation for any given native 
interface (which means the JIT can devirtualize interface calls).

I'll go ahead with this cleanup - let's revisit the leaking issue in 
followup change.

Maurizio



On 16/05/18 13:33, Sundararajan Athijegannathan wrote:
> +1
>
> Minor:
>
> * New code has old copyright years like 2015, 2016 in addition to 2018.
>
> * { at the end of comment in BinderClassGenerator
>
>  133             // ignore - the corresponding methods are generated 
> as part of processing the $get method{
>
>
> General comment not specific to the current change:
>
> WeakHashMap usage in LibrariesHelper. The value (implementation class) 
> holds strong reference to interface class - which is the key of 
> WeakHashMap => entries never GC'able?! From javadoc: " The value 
> objects in a WeakHashMap are held by ordinary strong references. Thus 
> care should be taken to ensure that value objects do not strongly 
> refer to their own keys, either directly or indirectly, since that 
> will prevent the keys from being discarded"
>
> -Sundar
>
> On 16/05/18, 5:16 PM, Maurizio Cimadamore wrote:
>> Hi,
>> I took at stab at consolidating the binder codegen support; the patch 
>> I put together addresses 3 issues:
>>
>> * The support for civilized bindings is less-than-half baked, and it 
>> should be removed. So I removed CivilizedHeaderImplGenerator and 
>> CivilizerAutoConversions. And I also removed the bind vs. bindRaw 
>> duality in the Libraries API. There's now only one version called 
>> 'bind' whose semantics is 'bind-raw'. This caused a flurry of small 
>> changes in tests.
>>
>> * (minor) I moved the Argument class from the toplevel impl package 
>> to the ABI package, since that's where it belongs
>>
>> * the meat of this patch is the removal of many of the code generator 
>> classes (I counted 8 of them). The rationale is that many of the 
>> things we do using FieldBuilders etc. are easily done using unsafe's 
>> constant pool patching (or condy, if we want to leave the unsafe 
>> world). The only reason for sticking things into static fields and 
>> then load them with a getfield is to generate code that is more 
>> scrutable (e.g. with javap), but I think this concern is not enough 
>> to justify keeping all these complex classes around.
>>
>> The new code has a main BinderClassGenerator, with two subclasses: 
>> StructImplGenerator and HeaderImplGenerator - specialized in 
>> generating binder impl for struct and headers, respectively. They all 
>> rely on a tweaked ASM ClassWriter which allows CP patching.
>>
>> Note: there's still a lot of code esp. in StructImplGenerator which I 
>> expect will go away as a result of further API changes and cleanup - 
>> e.g. right now array access is handled by the binder code spinning 
>> (!!), rather than by ReferenceImpl, but it's not worth cleaning that 
>> up given that our plan for arrays is quite different from what is 
>> implemented right now.
>>
>> Webrev here:
>>
>> http://cr.openjdk.java.net/~mcimadamore/panama/codegen-cleanup/
>>
>> Cheers
>> Maurizio
>>
>>