[Nestmates] RFR (S): Dynamic nestmate update

[David Holmes]

Hi Lois,

On 31/10/2018 3:43 AM, Lois Foltan wrote:
> On 10/30/2018 3:28 AM, David Holmes wrote:
> 
>> Hi Lois,
>>
>> Here's an update:
>>
>> http://cr.openjdk.java.net/~dholmes/dynamic-nestmates/webrev.v2
>>
>> Now we push the dynamic_nest_host through to the 
>> KlassFactory::create_from_stream level, where we set the nest-host 
>> directly after ClassFileParser.create_instance_klass has been called. 
>> That should ensure the class is not findable until after the nest-host 
>> has successfully been set, and doesn't require any further cleanup (as 
>> far as I can see) if the setting of the nest-host fails.
> 
> Thank you!  I think the parameter "nest_host" should be even pushed 
> further into ClassFileParser::create_instance_klass() & then into 
> ClassFileParser::fill_instance_klass().  The call to set_nest_host 
> should occur in ClassFileParser::fill_instance_klass for a couple of 
> reasons.
>      - 1. check_super_class_access and check_super_interface_access 
> could be affected by the setting of nest host?

Not today - nest-based access checks are only for members. If we get to 
sealed classes and allow dynamic nest members to relate to such sealed 
classes then we are already in a position where the nest host has to be 
known much earlier - and there are problems with that. This could be a 
preemptive move towards supporting that.

>      - 2. when the mirror is created and static fields are initialized 
> one of those field's initializers could access a nest member?  Would 
> this be an issue if nest_host is not set?

This is default (and compile-time constant?) initialization of fields. 
Any non-default initialization happens at <clinit> time which is much 
later - well after the nest-host has been set.

>      - 3. JFR might want to know this information by the time 
> JFR_ONLY(INIT_ID(ik);) is invoked at the end of 
> ClassFileParser::fill_instance_klass()

Okay - that may be true in the future.

> And this brings me to my nest code review comment for both of us. I'm 
> passing 3 more parameters (nonfindable, is_weak and vm_annotations) from 
> jvm_lookup_define_class() all the way down into ClassFileParser 
> construction.  We may need to consider creating a new data structure 
> much like ClassInfo or LinkInfo, something like ClassParseInfo that 
> contains information for not only our new parameters but existing unsafe 
> anonymous class information that is passed into 
> SystemDictionary::parse_stream as well.  Let me know what you think?

Yes - there is too much tramp data. Unclear whether to simply define a 
new holder class and pass that around everywhere, or whether to try and 
incorporate into existing strcutures e.g. perhaps attach it to the 
ClassFileParser? Depends on where the different bits of data are getting 
used.

I'll simply pass the nest-host further down directly for now to test out 
how this works.

>>
>> No changes to InstanceKlass::set_nest_host from previous version 
>> (sorry I'm not setup to do incremental webrevs ... need to fix that).
> A couple of minor comments for InstanceKlass::set_nest_host()
> - line #325 - comment "are a" should be "are"

Fixed.

> - If host is NULL, would you consider setting _nest_host to the current 
> instance class?  Aren't you your own nest host?

In a static sense you are your own nest host if nothing else is 
specified. But for this dynamic API a NULL host is a programming error 
to me.

Thanks,
David

>>
>> I've also removed Mandy's temporary nestmate access check code in 
>> reflection.cpp as it is not needed. So InstanceKlass:raw_nest_host() 
>> is also removed.
> Good, thank you, that had been something on my list to look at!
> 
>>
>> I also reworked jvm_lookup_define_class a little - the use of "const" 
>> caused flow-on problems that were simpler to avoid by not using "const".
> Looks good.
> 
> Thanks,
> Lois
> 
>>
>> Thanks,
>> David
>>
>> On 30/10/2018 9:35 AM, David Holmes wrote:
>>> Hi Lois,
>>>
>>> On 30/10/2018 6:01 AM, Lois Foltan wrote:
>>>> On 10/29/2018 2:37 AM, David Holmes wrote:
>>>>
>>>>> webrev: http://cr.openjdk.java.net/~dholmes/dynamic-nestmates/webrev/
>>>>>
>>>>> This fleshes out Mandy's initial implementation with additional 
>>>>> error checking on the VM side to match the defineClass API, and 
>>>>> with an additional VM test.
>>>>>
>>>>> Thanks,
>>>>> David
>>>> Hi David,
>>>>
>>>> I have a couple of concerns about this approach longer term for 
>>>> setting the nest host post return from either 
>>>> SystemDictionary::resolve_from_stream or 
>>>> SystemDictionary::parse_stream within jvm_lookup_define_class().
>>>>
>>>> - for a findable class as soon as that class is added to the 
>>>> SystemDictionary, is there any concern that another thread could 
>>>> request that class and access it before its nest host is 
>>>> legitimately set? Would it be better to pass nest host down into 
>>>> ClassFileParser so it is known at the point the InstanceKlass is 
>>>> created?
>>>
>>> Good question. I hadn't looked at exactly how and when the 
>>> defineClass logic set the nest host, I was only only updating the 
>>> actual set_nest_host internal logic.
>>>
>>> There are a couple of concerns here:
>>>
>>> 1. Finding the class before the nest-host has been set
>>> 2. Cleaning up in the case that setting the nest-host failed
>>>
>>> I don't think either are currently handled correctly - and my change 
>>> triggers #2. We need a way to make the complete logical operation 
>>> atomic. That most likely requires pushing the setting of the 
>>> nest-host deeper into the class loading logic - possibly immediately 
>>> after IK creation as you suggest.
>>>
>>>> - At the time class file nest members attributes are parsed and the 
>>>> nest host setting within the byte stream matches the nest host used 
>>>> when jvm_lookup_define_class() is invoked, than in my opinion an 
>>>> error should not result.
>>>
>>> I disagree. You're either using dynamic nest membership or you're 
>>> using static nest membership - using both, even if the end result 
>>> would be the same, seems an error in the programming model to me. 
>>> Afterall if the defined class is already a valid static nest member 
>>> then you don't need to use the NESTMATE property to inject it. And it 
>>> is definitely an error to claim a static nest-host when the host does 
>>> not statically list you as a member!
>>>
>>>> - Finally, should the setting of nest_host be known at the point 
>>>> class file load hook is processed?
>>>
>>> I'm not sure what you mean. At what point in the current code would 
>>> the byte[] passed to defineClass be replaced by one from the CLFH? As 
>>> long as the byte[] is final before we actually set_nest_host, I don't 
>>> think it matters.
>>>
>>> Thanks,
>>> David
>>>
>>>> Thanks,
>>>> Lois
>