[foreign] RFR: cleanup binder code generation

[Sundararajan Athijegannathan]

Sounds good.

-Sundar

On 16/05/18, 7:19 PM, Maurizio Cimadamore wrote:
> 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
>>>
>>>
>