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

[Lois Foltan]

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/

Looks good David, thanks for making the ss.print change I suggested.
Lois

>
> 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
>>>