(Nestmates) RFR (M): 8240645: Update nest host determination in line with latest proposed JVMS changes for JEP-371

[David Holmes]

Hi Harold,

On 13/03/2020 3:11 am, Harold Seigel wrote:
> Hi David,
> 
> The changes look good!

Thanks for looking at them.

> Can the "TRAPS" argument be removed from InstanceKlass::set_nest_host() ?

It is convenient to keep it to avoid the implicit Thread::current() that 
would be needed for the ResourceMark otherwise. The caller already has 
THREAD available to pass in.

Thanks,
David

> Thanks, Harold
> 
> On 3/11/2020 1:25 AM, David Holmes wrote:
>> John pointed out to me offline that there are now race conditions 
>> possible due to the setting of the error message. I had been lulled 
>> into a false sense of security by the overall idempotency of the 
>> operation. Simple fix is to introduce storestore barriers in a couple 
>> of places to ensure we write the error string, then the 
>> _nest_host_res_error field, then the _nest_host field, in that order. 
>> Commentary updated as well.
>>
>> This update also includes Lois's suggested cleanup in linkResolver.cpp.
>>
>> Incremental: http://cr.openjdk.java.net/~dholmes/8240645/webrev.v3-incr/
>>
>> Full: http://cr.openjdk.java.net/~dholmes/8240645/webrev.v3/
>>
>> Thanks,
>> David
>>
>> On 9/03/2020 9:09 am, David Holmes wrote:
>>> Hi John,
>>>
>>> On 7/03/2020 7:45 am, John Rose wrote:
>>>> Nice work, very thorough as usual.
>>>
>>> Thanks - and thanks for looking at this.
>>>
>>>> One nit.  The back-to-back parens are bad style.
>>>>
>>>>> foo (bar)(baz)
>>>>
>>>> https://english.stackexchange.com/questions/19429/should-i-use-adjacent-parentheses-or-a-semicolon-or-something-else 
>>>>
>>>> https://blog.apastyle.org/apastyle/2013/05/punctuation-junction-parentheses-and-brackets.html 
>>>>
>>>
>>> Okay - hadn't realized there was an applicable style here as I was 
>>> just implementing a multi-part message by a simple concatenation of 
>>> () delimited segments.
>>>
>>>> There are different ways to handle this in professional text.
>>>> I think the simplest way is relatively tricky from our perspective.
>>>> You merge the two parentheticals with an internal semicolon:
>>>>
>>>>> foo (bar; baz)
>>>
>>> I think that would make it very hard to spot the changeover.
>>>
>>>> A really simple tweak to your code would be to put a comma
>>>> between.  This is less standard but better than back-to-back
>>>> parens:
>>>>
>>>> -+        ss.print("(");
>>>> ++        ss.print(“, (“);
>>>>
>>>> (Two places.)
>>>
>>> Ok - that works for me.
>>>
>>>> Likewise, in two places:
>>>>
>>>> -+      msg = "(stuff…)";
>>>> ++      msg = " (stuff…)”;
>>>>
>>>> …to avoid the left paren making hard contact with the preceding text.
>>>
>>> If this is referring to instanceKlass.cpp:
>>>
>>>   363       msg = "(the NestHost attribute in the current class is 
>>> ignored)";
>>>   364     } else if (_nest_members != NULL && _nest_members != 
>>> Universe::the_empty_short_array()) {
>>>   365       msg = "(the NestMembers attribute in the current class is 
>>> ignored)";
>>>
>>> then the space is part of the main formatted string:
>>>
>>>   367     log_trace(class, nestmates)("Injected type %s into the nest 
>>> of %s %s",
>>>   368                                 this->external_name(),
>>>   369                                 host->external_name(),
>>>   370                                 msg);
>>>
>>>
>>>> Klass::_nest_host_res_error is ok.  If we ever decide to reduce the
>>>> footprint there (as part of a larger effort, I think), we could merge
>>>> both fields by using a tagged pointer, on the observation that you
>>>> only need one or the other to be non-null.  If the res-error is not
>>>> null, then we can infer that nest-host has been determined to be
>>>> self, and in that case we don’t need to look at the field, right?
>>>
>>> A non-NULL res-err does imply _nest_host == this. But not vice-versa.
>>>
>>>> You might want to add a comment about the possible states of
>>>> those two coupled variables, to help maintainers.  For example:
>>>>
>>>> + // This variable is non-null only if _nest_host is null.
>>>>
>>>> Actually, I didn’t follow the logic fully:  Can both fields be non-null
>>>> at the same time?  It looks like Klass::nest_host only gets set 
>>>> non-null
>>>> on error-free paths, while the error string is set only on error paths.
>>>> It would be reasonable for Klass::nest_host to check for an error
>>>> string and immediately return “this” if one is set.  I didn’t see this.
>>>>
>>>> Maybe the comment should be:
>>>>
>>>> + // This variable is non-null only if _nest_host is this or (during 
>>>> a race) null.
>>>>
>>>> (And if there’s a race, please do a releasing store of the error 
>>>> string before
>>>> setting the _nest_host to “this”.  But at-least-one-always-null is a 
>>>> safer
>>>> convention for managing races.  So I like the first comment much 
>>>> better.)
>>>
>>> _nest_host is always set, even when there is an error. So nest_host() 
>>> does a quick return in all calls other than the first (modulo the 
>>> rare case where we had to bail out because we are in a compiler 
>>> thread; or VME occurred):
>>>
>>>   226 InstanceKlass* InstanceKlass::nest_host(TRAPS) {
>>>   227   InstanceKlass* nest_host_k = _nest_host;
>>>   228   if (nest_host_k != NULL) {
>>>   229     return nest_host_k;
>>>   230   }
>>>
>>> I can't see any way to reduce footprint in this area as we have two 
>>> very distinct entities we are tracking.
>>>
>>>> Also, let’s talk about VirtualMachineError.
>>>>
>>>> After thinking about it a bit, I think you went too far replacing CATCH
>>>> by THREAD; this makes a fragile assumption that nothing is coming
>>>> out of the call.  It’s fragile because, although we expect 
>>>> LinkageError,
>>>> there’s usually an additional possibility of VirtualMachineError.
>>>> Doing a catch-and-clear of VME is risky business, and in this case
>>>> I only think we need to catch and clear the LE, not the VME.
>>>>
>>>> To me, it seems safest to always expect VME, and always pass it on,
>>>> as a basic convention in our code base, with clear documentation
>>>> wherever we do something different.
>>>>
>>>> Imagine getting an access error later on, with the parenthetical
>>>> remark that the nest host failed due to SOE or OOME.  Those guys
>>>> should be fail-fast, I think.  Or did you come up with a reason they
>>>> should be muzzled also, along with the LE we intend to muzzle?
>>>>
>>>> I see the previous code processed VME differently, and that you
>>>> removed it, so I know you did this as a considered step.  But still
>>>> it seems off to me.  At least add some documentation explaining
>>>> why it’s safe to drop VM errors.
>>>
>>> You are absolutely right, I went too far in clearing ALL pending 
>>> exceptions. I need to check for VME and do immediate return in that 
>>> case; and restore the CHECK usage in the callers. Not only should 
>>> VMEs be fail-fast, but they are also cases where we should be allowed 
>>> to retry determining the nest host.
>>>
>>> Updated webrevs:
>>>
>>> Incremental: http://cr.openjdk.java.net/~dholmes/8240645/webrev.v2-incr/
>>>
>>> (ciField.cpp and reflection.cpp are simply reverted)
>>>
>>> Full: http://cr.openjdk.java.net/~dholmes/8240645/webrev.v2/
>>>
>>> Thanks,
>>> David
>>>
>>>> — John
>>>>