[Nestmates] RFR (S): Dynamic nestmate update

[Harold David Seigel]

Thanks David.  I agree that the assertion is useless.

Harold


On 10/31/2018 9:28 AM, David Holmes wrote:
> Hi Harold,
>
> On 31/10/2018 10:53 PM, Harold David Seigel wrote:
>> Hi David,
>>
>> In klassFactory.cpp, does it matter that the assert passes NULL, 
>> instead of dynamic_nest_host, to create_instance_klass() ?
>>
>> 228 InstanceKlass* result = parser.create_instance_klass(old_stream 
>> != stream, dynamic_nest_host, CHECK_NULL);
>> 229 assert(result == parser.create_instance_klass(old_stream != 
>> stream, NULL, THREAD), "invariant"); Thanks, Harold
>
> Not really. The assert basically checks that a second call to 
> create_instance_klass on the same parser returns the class created by 
> the first call:
>
> 5507 InstanceKlass* ClassFileParser::create_instance_klass(bool 
> changed_by_loadhook,
> 5508 InstanceKlass* dynamic_nest_host,
> 5509                                                       TRAPS) {
> 5510   if (_klass != NULL) {
> 5511     return _klass;  <= second+ call returns here
> 5512   }
>
> The only way this can not succeed is if the first call failed for some 
> reason, but in that case the CHECK_NULL would mean we never execute 
> the assertion.
>
> I think that assertion is fairly useless.
>
> Cheers,
> David
>
>>
>> On 10/31/2018 2:56 AM, David Holmes wrote:
>>> Hi Lois,
>>>
>>> I've pushed set_nest_host down into fill_instance_class, where it is 
>>> set as early as possible - which is after 
>>> apply_parsed_class_metadata as that sets the static nest attributes 
>>> in the IK.
>>>
>>> I also had to remove an assertion that's no longer valid at the time 
>>> we try to assert it.
>>>
>>> Full webrev: 
>>> http://cr.openjdk.java.net/~dholmes/dynamic-nestmates/webrev.v3/
>>>
>>> Incremental: 
>>> http://cr.openjdk.java.net/~dholmes/dynamic-nestmates/webrev.v3-incr/
>>>
>>> This seems early enough to address all your concerns re super 
>>> access, initializers and JFR, even if we don't currently need to see 
>>> the nest_host.
>>>
>>> Thanks,
>>> David
>>>
>>> On 31/10/2018 8:25 AM, David Holmes wrote:
>>>> 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
>>>>>
>>