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

[David Holmes]

Hi Lois,

On 10/03/2020 2:18 am, Lois Foltan wrote:
> Hi David,
> 
> Overall looks good.  I do have a couple of comments on the latest webrev.

Thanks for taking a look.

> 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 : "");

Yes that looks much neater - thanks.

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

It is partially redundant, but that part is not (necessarily) reporting 
the loader info for the two types involved in the access, but one of 
those types and its purported nest host. We could potentially drop the 
loader part for the type (relying on the larger error message to report 
it earlier) but we would still want that information for the logging 
output. I was trying to keep the message production uniform for both the 
exception message and the logging; and using the same general format 
(arguably we could drop the loader information for all but the "types 
are in different packages" case). The price of that 
simplicity/uniformity is some redundancy. Given we don't really expect 
users to run into these error messages very often it doesn't seem unduly 
problematic.

Thanks,
David

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