RE: Request Review: 6902182: Starting with jdwp agent should not incur performance penalty

[Deneau, Tom]

Dan --

Good points.  I'll make these changes and also the rename to 
can_post_on_exceptions() and repost.

-- Tom


-----Original Message-----
From: daniel.daughe...@sun.com [mailto:daniel.daughe...@sun.com] 
Sent: Thursday, January 07, 2010 6:04 PM
To: Deneau, Tom
Cc: serviceability-dev@openjdk.java.net; hotspot-compiler-...@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-...@openjdk.java.net and
> serviceability-dev@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
>   

src/share/vm/c1/c1_Runtime1.cpp
    No comments.
          
src/share/vm/opto/graphKit.cpp
    No comments.

src/share/vm/opto/graphKit.hpp
    No comments.

src/share/vm/opto/parse2.cpp
    Comment on line 2069 seems redundant.

src/share/vm/opto/runtime.cpp
    Typo: 'if we exceptions' -> 'if exceptions'

src/share/vm/prims/jvmtiEventController.cpp
    The JvmtiEventControllerPrivate::recompute_thread_enabled()
    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;
        state->set_should_post_on_exceptions(should_post_on_exceptions);

    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.

src/share/vm/prims/jvmtiExport.cpp
    No comments.

src/share/vm/prims/jvmtiExport.hpp
    No comments.

src/share/vm/runtime/thread.cpp
    No comments.

src/share/vm/runtime/thread.hpp
    No comments.



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

Dan