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

[Lois Foltan]

Hi David,

Overall looks good.  I do have a couple of comments on the latest webrev.

interpreter/linkResolver.cpp
- lines 609-618 and 966-975, the if statements constructing the 
nest_host_error_1 and nest_host_error2 could be reduced to something 
like the following:

ss.print(", (%s%s%s)",
                (nest_host_error1 != NULL) ? nest_host_error_1 : "",
                (nest_host_error1 != NULL && nest_host_error2 != NULL) ? 
", " : "",
                (nest_host_error2 != NULL) ? nest_host_error2 : "");

oops/instanceKlass.cpp
-  the error message constructed on line #321, would you consider 
dropping the "(loader: %s)"  part?  The only contexts that 
ik->nest_host_resolution_error() is called are in linkResolver.cpp which 
in both cases already reports the module & loader of each type.  It just 
seems to be redundant information.

Thanks,
Lois



On 3/8/2020 7:09 PM, 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
>>