(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