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

David Holmes david.holmes at oracle.com
Sun Mar 8 23:09:51 UTC 2020


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
> 


More information about the valhalla-dev mailing list