Request Review: 6902182: Starting with jdwp agent should not incur performance penalty
tom.deneau at amd.com
Thu Jan 7 16:22:42 PST 2010
Good points. I'll make these changes and also the rename to can_post_on_exceptions() and repost.
From: Daniel.Daugherty at Sun.COM [mailto:Daniel.Daugherty at Sun.COM]
Sent: Thursday, January 07, 2010 6:04 PM
To: Deneau, Tom
Cc: serviceability-dev at openjdk.java.net; hotspot-compiler-dev at openjdk.java.net
Subject: Re: Request Review: 6902182: Starting with jdwp agent should not incur performance penalty
Deneau, Tom wrote:
> Cross posting to hotspot-compiler-dev at openjdk.java.net and
> serviceability-dev at openjdk.java.net since this review request touches
> both compiler/runtime code and JVMTI code...
> New webrev is at http://cr.openjdk.java.net/~tdeneau/6902182/webrev.04
Comment on line 2069 seems redundant.
Typo: 'if we exceptions' -> 'if exceptions'
change breaks the JavaThread versus JvmtiThreadState
abstraction a bit.
I would have done lines 516-517 like:
bool should_post_on_exceptions =
(any_env_enabled & SHOULD_POST_ON_EXCEPTIONS_BITS) != 0;
This would require a new function in JvmtiThreadState named
set_should_post_on_exceptions() that makes the actual call
to _thread->set_should_post_on_exceptions_flag(). The fact
that the new flag lives in the JavaThread is an implementation
detail that the JvmtiEvent stuff shouldn't care about.
> Note to Dan:
> * We had talked about changing the name of the existing
> jvmti_can_post_exceptions() to jvmti_can_post_on_exceptions() to
> make it match the new should_post_on_exceptions name used above.
> Since this affected some other files, to keep this webrev
> simpler, I decided not to do that as part of this webrev. If we
> still want to do this, I can post this additional change as the
> final webrev.
Following the existing style would be clearer (IMHO):
can_post_foo <=> should_post_foo
so it should be:
can_post_on_exceptions <=> should_post_on_exceptions
in the JvmtiExport stuff. Looks like the compiler's
jvmti_can_post_exceptions() should also change to
jvmti_can_post_on_exceptions() to match.
Since you correctly pointed out that it really should be
"post *on* exceptions" instead of "post exceptions", I would
prefer if the change were made as part of this work. However,
I can make the change later if you wish.
More information about the hotspot-compiler-dev