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

[David Holmes]

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