date:20200512

Re: RFR(S) 8238585: Use handshake for JvmtiEventControllerPrivate::enter_interp_only_mode() and don't make compiled methods on stack not_entrant

2020-05-12 Thread David Holmes


On 4/05/2020 8:33 pm, Reingruber, Richard wrote:

// Trimmed the list of recipients. If the list gets too long then the message 
needs to be approved
// by a moderator.


Yes I noticed that too :) In general if you send to hotspot-dev you 
shouldn't need to also send to hotspot-X-dev.



Hi David,


Hi Richard,


On 28/04/2020 12:09 am, Reingruber, Richard wrote:

Hi David,


Not a review but some general commentary ...


That's welcome.



Having had to take an even closer look now I have a review comment too :)



src/hotspot/share/prims/jvmtiThreadState.cpp



void JvmtiThreadState::invalidate_cur_stack_depth() {
!   assert(SafepointSynchronize::is_at_safepoint() ||
!   (Thread::current()->is_VM_thread() &&
get_thread()->is_vmthread_processing_handshake()) ||
(JavaThread *)Thread::current() == get_thread(),
"must be current thread or at safepoint");


You're looking at an outdated webrev, I'm afraid.

This would be the post with the current webrev.1

   
http://mail.openjdk.java.net/pipermail/serviceability-dev/2020-April/031245.html


 Sorry I missed that update. Okay so this is working with direct 
handshakes now.


One style nit in jvmtiThreadState.cpp:

assert(SafepointSynchronize::is_at_safepoint() ||
!   (JavaThread *)Thread::current() == get_thread() ||
!   Thread::current() == get_thread()->active_handshaker(),
! "bad synchronization with owner thread");

the ! lines should ident as follows

assert(SafepointSynchronize::is_at_safepoint() ||
   (JavaThread *)Thread::current() == get_thread() ||
   Thread::current() == get_thread()->active_handshaker(),
 ! "bad synchronization with owner thread");

Lets see how this plays out.

Cheers,
David


Thanks, Richard.

-Original Message-
From: David Holmes 
Sent: Montag, 4. Mai 2020 08:51
To: Reingruber, Richard ; Yasumasa Suenaga 
; Patricio Chilano ; 
serguei.spit...@oracle.com; Vladimir Ivanov ; 
serviceability-dev@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; 
hotspot-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net; hotspot-gc-...@openjdk.java.net
Subject: Re: RFR(S) 8238585: Use handshake for 
JvmtiEventControllerPrivate::enter_interp_only_mode() and don't make compiled 
methods on stack not_entrant

Hi Richard,

On 28/04/2020 12:09 am, Reingruber, Richard wrote:

Hi David,


Not a review but some general commentary ...


That's welcome.


Having had to take an even closer look now I have a review comment too :)

src/hotspot/share/prims/jvmtiThreadState.cpp

void JvmtiThreadState::invalidate_cur_stack_depth() {
!   assert(SafepointSynchronize::is_at_safepoint() ||
!   (Thread::current()->is_VM_thread() &&
get_thread()->is_vmthread_processing_handshake()) ||
(JavaThread *)Thread::current() == get_thread(),
"must be current thread or at safepoint");

The message needs updating to include handshakes.

More below ...


On 25/04/2020 2:08 am, Reingruber, Richard wrote:

Hi Yasumasa, Patricio,


I will send review request to replace VM_SetFramePop to handshake in early next 
week in JDK-8242427.
Does it help you? I think it gives you to remove workaround.


I think it would not help that much. Note that when replacing VM_SetFramePop 
with a direct handshake
you could not just execute VM_EnterInterpOnlyMode as a nested vm operation [1]. 
So you would have to
change/replace VM_EnterInterpOnlyMode and I would have to adapt to these 
changes.



Thanks for your information.
I tested my patch with both vmTestbase/nsk/jvmti/PopFrame and 
vmTestbase/nsk/jvmti/NotifyFramePop.
I will modify and will test it after yours.


Thanks :)


Also my first impression was that it won't be that easy from a synchronization 
point of view to
replace VM_SetFramePop with a direct handshake. E.g. VM_SetFramePop::doit() 
indirectly calls
JvmtiEventController::set_frame_pop(JvmtiEnvThreadState *ets, JvmtiFramePop 
fpop) where
JvmtiThreadState_lock is acquired with safepoint check, if not at safepoint. 
It's not directly clear
to me, how this has to be handled.



I think JvmtiEventController::set_frame_pop() should hold JvmtiThreadState_lock 
because it affects other JVMTI operation especially FramePop event.


Yes. To me it is unclear what synchronization is necessary, if it is called 
during a handshake. And
also I'm unsure if a thread should do safepoint checks while executing a 
handshake.



I'm growing increasingly concerned that use of direct handshakes to
replace VM operations needs a much greater examination for correctness
than might initially be thought. I see a number of issues:


I agree. I'll address your concerns in the context of this review thread for 
JDK-8238585 below.

In addition I would suggest to take the general part of the discussion to a 
dedicated thread or to
the review thread for JDK-8242427. I would like to keep this thread closer to 
its subject.


I will focus on the issues in the context of this particular change
then, though

Re: RFR 8244105: JDK 11.0.7 -Xlog gc issue with file path if not exist

2020-05-12 Thread David Holmes


Hi Harold,

On 12/05/2020 10:49 pm, Harold Seigel wrote:

Hi David,

Thanks for looking at this.

This change disables logging and issuing a warning for any errors in the 
-Xlog arguments that currently cause the JVM to terminate. For which 
particular bad -Xlog arguments do you think this is a bad idea?


All of them! Unified Logging has always been fail-fast when it comes to 
the log configuration settings. I think that is a good design and 
certainly not something we should do a complete about-face on just 
because of one reported issue!


My basic reaction to this issue now is that it is a "won't fix". Yes UL 
behaves differently compared to the old GC logging flags - so be it.


At the very most we might consider making the inability to open the log 
file a non-fatal error, but even then I'm more inclined to fail-fast so 
that people realize they have set things up incorrectly rather than 
allowing mistakes to go undetected.


This is also something that would need discussing at hotspot level 
rather than just runtime & serviceability.


Cheers,
David
-


Are there some bad -Xlog arguments where you think this would be a good 
thing to do?


I agree that if we decide to do this, a CSR is needed.

Thanks, Harold

On 5/12/2020 3:32 AM, David Holmes wrote:

Hi Harold,

On 9/05/2020 3:22 am, Harold Seigel wrote:

Hi,

Please review this fix for JDK-8244105.  The fix continues program 
execution even when specified logging options are invalid. 
Previously, invalid logging options terminated the program.  Now, a 
warning is issued.  For example:


 > java -Xlog:"gc*:file=/dont/exist" -version
    [0.001s][error][logging] Error opening log file '/dont/exist': No
    such file or directory
    [0.001s][error][logging] Initialization of output 'file=/dont/exist'
    using options '(null)' failed.
    Java HotSpot(TM) 64-Bit Server VM warning: Invalid -Xlog option
    '-Xlog:gc*:file=/dont/exist', see error log for details.

    java version "15-internal" 2020-09-15
    Java(TM) SE Runtime Environment (fastdebug build
    15-internal+0-2020-05-08-1313404.hseigel.bug8244105)
    Java HotSpot(TM) 64-Bit Server VM (fastdebug build
    15-internal+0-2020-05-08-1313404.hseigel.bug8244105, mixed mode,
    sharing)


But if I am reading this correctly you are now only issuing a warning 
and disabling logging if there are any errors of any kind in the -Xlog 
arguments! That is not desirable IMO and a significant change in 
behaviour.


Even just changing the behaviour in relation to a non-existent log 
file will require a CSR request.


Thanks,
David
-

Open Webrev: 
http://cr.openjdk.java.net/~hseigel/bug_8244105/webrev/index.html


JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8244105

The fix was regression tested by running Mach5 tiers 1 and 2 tests 
and builds on Linux-x64, Solaris, Windows, and Mac OS X and by 
running Mach5 tiers 3-5 tests on Linux-x64.


Thanks, Harold

Re: Ping: RFR: JDK-8243012: Fix issues in j.l.i package info

2020-05-12 Thread serguei.spit...@oracle.com


Hi Alex,

This seems to resolve most of the Alan's concerns.
Though, I'm not sure if we can treat users that deploy and use agents as 
developers.


Otherwise, we may want to tweak the last sentence a little bit:
 "Developers or administrators that deploy agents, deploy applications 
that
package an agent with the application, or anyone using a tools that 
loads agents into a
running application, are responsible for verifying the trustworthiness 
of each

agent including the content and structure of the agent JAR file.


But let's wait for Alan's opinion.

Thanks,
Serguei


On 5/12/20 12:57, Alex Menkov wrote:

Hi Alan, Serguei,

lets try one more time :)

What about:

Agents can transform classes in arbitrary ways at load time, transform
modules, or transform the bytecode of methods of already loaded classes.
Developers or administrators that deploy agents, deploy applications that
package an agent with the application, or use tools that load agents 
into a
running application, are responsible for verifying the trustworthiness 
of each

agent including the content and structure of the agent JAR file.


please let me know what do you thinks, I'll prepare & publish new 
webrev as soon as we get agreement about the paragraph.



--alex

On 05/12/2020 00:59, Alan Bateman wrote:

On 11/05/2020 22:14, Alex Menkov wrote:



Updated webrev:
http://cr.openjdk.java.net/~amenkov/jdk15/java_instrument_spec/webrev.2/ 



--alex
This doesn't work for me because it drops the important point that 
the developer/admin is also responsible when deploying an agent that 
packages an agent with the application. Also anyone using a tool that 
loads agents into a running VM has responsibility too. So I think 
these points need to be included.


-Alan.

Re: Ping: RFR: JDK-8243012: Fix issues in j.l.i package info

2020-05-12 Thread Alex Menkov


Hi Alan, Serguei,

lets try one more time :)

What about:

Agents can transform classes in arbitrary ways at load time, transform
modules, or transform the bytecode of methods of already loaded classes.
Developers or administrators that deploy agents, deploy applications that
package an agent with the application, or use tools that load agents into a
running application, are responsible for verifying the trustworthiness 
of each

agent including the content and structure of the agent JAR file.


please let me know what do you thinks, I'll prepare & publish new webrev 
as soon as we get agreement about the paragraph.



--alex

On 05/12/2020 00:59, Alan Bateman wrote:

On 11/05/2020 22:14, Alex Menkov wrote:



Updated webrev:
http://cr.openjdk.java.net/~amenkov/jdk15/java_instrument_spec/webrev.2/

--alex
This doesn't work for me because it drops the important point that the 
developer/admin is also responsible when deploying an agent that 
packages an agent with the application. Also anyone using a tool that 
loads agents into a running VM has responsibility too. So I think these 
points need to be included.


-Alan.

Re: RFR: 8242009: Review setting test.java/vm.opts in jcmd/jhsdb and debugger in serviceability tests

2020-05-12 Thread Daniil Titov

Hi Chris,

Thank you for reviewing this change.

Best regards,
Daniil

On 5/11/20, 10:55 AM, "Chris Plummer"  wrote:

Hi Daniil,

Looks good.

thanks,

Chris

On 5/11/20 9:43 AM, Daniil Titov wrote:
> Hi Chris,
>
> Please review a new version of the fix[1] that also updates jdk/sun/tools 
 tests.
>
> Testing: Mach5 tier1-tier7 tests successfully passed.
>
>
> [1] http://cr.openjdk.java.net/~dtitov/8242009/webrev.03/
> [2] ] https://bugs.openjdk.java.net/browse/JDK-8242009
>
> Thank you,
> Daniil
>
> On 5/8/20, 12:21 AM, "Chris Plummer"  wrote:
>
>  Hi Daniil,
>
>  I just noticed you didn't update the tests in jdk/sun/tools/jhsdb. Do
>  you think these should be done also?
>
>  Chris
>
>  On 5/7/20 11:44 PM, Chris Plummer wrote:
>  > Hi Daniil,
>  >
>  > The changes look good.
>  >
>  > thanks,
>  >
>  > Chris
>  >
>  > On 5/4/20 1:05 PM, Daniil Titov wrote:
>  >> Hi Chris,
>  >>
>  >> Please review a new version of webrev [1] that addresses your 
comments.
>  >>
>  >> For the following 3 tests that showed the increase of the 
execution
>  >> time with -Xcomp
>  >> more than 5 minutes this version of the change  strips -Xcomp 
option
>  >> when
>  >> forwarding VM  arguments to  j*-tools  :
>  >>
>  >> -- serviceability/sa/sadebugd/SADebugDTest.java,
>  >> -- serviceability/sa/sadebugd/DebugdConnectTest.java,
>  >> -- serviceability/sa/ClhsdbJstackXcompStress.java
>  >>
>  >> The execution time for the rest of the tests when running with 
-Xcomp
>  >> was increased
>  >> within 1 and half minute.
>  >>
>  >>
>  >> [1] http://cr.openjdk.java.net/~dtitov/8242009/webrev.02/
>  >> [2] https://bugs.openjdk.java.net/browse/JDK-8242009
>  >>
>  >> Thank you,
>  >>   Daniil
>  >>
>  >>
>  >> On 4/27/20, 12:55 PM, "Chris Plummer"  
wrote:
>  >>
>  >>  Hi Daniil,
>  >>
>  >>  Overall it looks good. A few comments below.
>  >>
>  >>  Can you add a comment to TestSysProps.java indicating the 
reason
>  >> for
>  >>  checking if the line starts with "[".
>  >>
>  >>  In JDKToolLauncher you have an extra empty line:
>  >>
>  >>112  * Any platform specific arguments required for
>  >> running the
>  >>  tool are
>  >>113  * automatically added.
>  >>114  *
>  >>115  *
>  >>116  * @param args
>  >>
>  >>  In OutputAnalyzer.java, I wonder if all these matching APIs 
you
>  >> updated
>  >>  should by default just include the version output in their
>  >> filtering.
>  >>
>  >>  As for the added time to execute, I would suggest possibly
>  >> stripping
>  >>  -Xcomp from the few outliers, and I would mostly focus on 
how much
>  >>  longer it takes, not how many times longer. For example,
>  >> increasing from
>  >>  10 seconds to 40 seconds is not a big deal. Increasing from 
10
>  >> minutes
>  >>  to 20 minutes is.
>  >>
>  >>  SADebugDTest creates 8 tool processes so, that's probably the
>  >> reason for
>  >>  the big increase, although I'm not sure why it is kind of 
slow
>  >> in the
>  >>  first place. It does nothing more on each iteration than 
launch
>  >> "jhsdb
>  >>  debugd", which will connect to the debuggee, and then is 
killed
>  >> off.
>  >>  Maybe there is something slow with connecting, especial with 
RMI.
>  >>
>  >>  thanks,
>  >>
>  >>  Chris
>  >>
>  >>  On 4/27/20 12:07 PM, Daniil Titov wrote:
>  >>  > Please review the change [1] that ensures that VM and test
>  >> options are forwarded to
>  >>  > j*-tools when they are launched from serviceability/sa 
tests.
>  >>  >
>  >>  > The tests that expect an empty output  were corrected to
>  >> ignore the product version printed
>  >>  > in the error stream since in some  tiers the tests are run
>  >> with ' -showversion' VM option (tier3).
>  >>  >
>  >>  > In test serviceability/sa/TestSysProps.java the code that
>  >> counts the system properties  was  corrected
>  >>  > to ignore the debug output when the test is run with "
>  >> -Xlog:cds=debug" option (tier4).
>

Re: RFR 8244105: JDK 11.0.7 -Xlog gc issue with file path if not exist

2020-05-12 Thread Harold Seigel


Hi David,

Thanks for looking at this.

This change disables logging and issuing a warning for any errors in the 
-Xlog arguments that currently cause the JVM to terminate. For which 
particular bad -Xlog arguments do you think this is a bad idea?


Are there some bad -Xlog arguments where you think this would be a good 
thing to do?


I agree that if we decide to do this, a CSR is needed.

Thanks, Harold

On 5/12/2020 3:32 AM, David Holmes wrote:

Hi Harold,

On 9/05/2020 3:22 am, Harold Seigel wrote:

Hi,

Please review this fix for JDK-8244105.  The fix continues program 
execution even when specified logging options are invalid.  
Previously, invalid logging options terminated the program.  Now, a 
warning is issued.  For example:


 > java -Xlog:"gc*:file=/dont/exist" -version
    [0.001s][error][logging] Error opening log file '/dont/exist': No
    such file or directory
    [0.001s][error][logging] Initialization of output 'file=/dont/exist'
    using options '(null)' failed.
    Java HotSpot(TM) 64-Bit Server VM warning: Invalid -Xlog option
    '-Xlog:gc*:file=/dont/exist', see error log for details.

    java version "15-internal" 2020-09-15
    Java(TM) SE Runtime Environment (fastdebug build
    15-internal+0-2020-05-08-1313404.hseigel.bug8244105)
    Java HotSpot(TM) 64-Bit Server VM (fastdebug build
    15-internal+0-2020-05-08-1313404.hseigel.bug8244105, mixed mode,
    sharing)


But if I am reading this correctly you are now only issuing a warning 
and disabling logging if there are any errors of any kind in the -Xlog 
arguments! That is not desirable IMO and a significant change in 
behaviour.


Even just changing the behaviour in relation to a non-existent log 
file will require a CSR request.


Thanks,
David
-

Open Webrev: 
http://cr.openjdk.java.net/~hseigel/bug_8244105/webrev/index.html


JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8244105

The fix was regression tested by running Mach5 tiers 1 and 2 tests 
and builds on Linux-x64, Solaris, Windows, and Mac OS X and by 
running Mach5 tiers 3-5 tests on Linux-x64.


Thanks, Harold

RE: RFR(S) 8238585: Use handshake for JvmtiEventControllerPrivate::enter_interp_only_mode() and don't make compiled methods on stack not_entrant

2020-05-12 Thread Reingruber, Richard

Thanks Martin.

Cheers, Richard.

-Original Message-
From: Doerr, Martin  
Sent: Dienstag, 12. Mai 2020 10:43
To: Reingruber, Richard ; David Holmes 
; serviceability-dev@openjdk.java.net; 
hotspot-compiler-...@openjdk.java.net; hotspot-...@openjdk.java.net; 
hotspot-runtime-...@openjdk.java.net; hotspot-gc-...@openjdk.java.net
Subject: RE: RFR(S) 8238585: Use handshake for 
JvmtiEventControllerPrivate::enter_interp_only_mode() and don't make compiled 
methods on stack not_entrant

Hi Richard,

I had already reviewed webrev.1. Looks good to me.
Thanks for contributing it.

Best regards,
Martin


> -Original Message-
> From: hotspot-runtime-dev  boun...@openjdk.java.net> On Behalf Of Reingruber, Richard
> Sent: Montag, 4. Mai 2020 12:33
> To: David Holmes ; serviceability-
> d...@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; hotspot-
> d...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net; hotspot-
> gc-...@openjdk.java.net
> Subject: RE: RFR(S) 8238585: Use handshake for
> JvmtiEventControllerPrivate::enter_interp_only_mode() and don't make
> compiled methods on stack not_entrant
> 
> // Trimmed the list of recipients. If the list gets too long then the message
> needs to be approved
> // by a moderator.
> 
> Hi David,
> 
> > On 28/04/2020 12:09 am, Reingruber, Richard wrote:
> > > Hi David,
> > >
> > >> Not a review but some general commentary ...
> > >
> > > That's welcome.
> 
> > Having had to take an even closer look now I have a review comment too :)
> 
> > src/hotspot/share/prims/jvmtiThreadState.cpp
> 
> >void JvmtiThreadState::invalidate_cur_stack_depth() {
> > !   assert(SafepointSynchronize::is_at_safepoint() ||
> > !   (Thread::current()->is_VM_thread() &&
> > get_thread()->is_vmthread_processing_handshake()) ||
> >(JavaThread *)Thread::current() == get_thread(),
> >"must be current thread or at safepoint");
> 
> You're looking at an outdated webrev, I'm afraid.
> 
> This would be the post with the current webrev.1
> 
>   http://mail.openjdk.java.net/pipermail/serviceability-dev/2020-
> April/031245.html
> 
> Thanks, Richard.
> 
> -Original Message-
> From: David Holmes 
> Sent: Montag, 4. Mai 2020 08:51
> To: Reingruber, Richard ; Yasumasa Suenaga
> ; Patricio Chilano
> ; serguei.spit...@oracle.com; Vladimir
> Ivanov ; serviceability-
> d...@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; hotspot-
> d...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net; hotspot-
> gc-...@openjdk.java.net
> Subject: Re: RFR(S) 8238585: Use handshake for
> JvmtiEventControllerPrivate::enter_interp_only_mode() and don't make
> compiled methods on stack not_entrant
> 
> Hi Richard,
> 
> On 28/04/2020 12:09 am, Reingruber, Richard wrote:
> > Hi David,
> >
> >> Not a review but some general commentary ...
> >
> > That's welcome.
> 
> Having had to take an even closer look now I have a review comment too :)
> 
> src/hotspot/share/prims/jvmtiThreadState.cpp
> 
>void JvmtiThreadState::invalidate_cur_stack_depth() {
> !   assert(SafepointSynchronize::is_at_safepoint() ||
> !   (Thread::current()->is_VM_thread() &&
> get_thread()->is_vmthread_processing_handshake()) ||
>(JavaThread *)Thread::current() == get_thread(),
>"must be current thread or at safepoint");
> 
> The message needs updating to include handshakes.
> 
> More below ...
> 
> >> On 25/04/2020 2:08 am, Reingruber, Richard wrote:
> >>> Hi Yasumasa, Patricio,
> >>>
> >> I will send review request to replace VM_SetFramePop to
> handshake in early next week in JDK-8242427.
> >> Does it help you? I think it gives you to remove workaround.
> >
> > I think it would not help that much. Note that when replacing
> VM_SetFramePop with a direct handshake
> > you could not just execute VM_EnterInterpOnlyMode as a nested vm
> operation [1]. So you would have to
> > change/replace VM_EnterInterpOnlyMode and I would have to adapt
> to these changes.
> >>>
>  Thanks for your information.
>  I tested my patch with both vmTestbase/nsk/jvmti/PopFrame and
> vmTestbase/nsk/jvmti/NotifyFramePop.
>  I will modify and will test it after yours.
> >>>
> >>> Thanks :)
> >>>
> > Also my first impression was that it won't be that easy from a
> synchronization point of view to
> > replace VM_SetFramePop with a direct handshake. E.g.
> VM_SetFramePop::doit() indirectly calls
> > JvmtiEventController::set_frame_pop(JvmtiEnvThreadState *ets,
> JvmtiFramePop fpop) where
> > JvmtiThreadState_lock is acquired with safepoint check, if not at
> safepoint. It's not directly clear
> > to me, how this has to be handled.
> >>>
>  I think JvmtiEventController::set_frame_pop() should hold
> JvmtiThreadState_lock because it affects other JVMTI operation especially
> FramePop event.
> >>>
> >>> Yes. To me it is unclear what synchronization is necessary, if it is 
> >>> called
> during a handshake. And
> >>> also I'm unsure if a thread should

RE: RFR(S) 8238585: Use handshake for JvmtiEventControllerPrivate::enter_interp_only_mode() and don't make compiled methods on stack not_entrant

2020-05-12 Thread Doerr, Martin

Hi Richard,

I had already reviewed webrev.1. Looks good to me.
Thanks for contributing it.

Best regards,
Martin


> -Original Message-
> From: hotspot-runtime-dev  boun...@openjdk.java.net> On Behalf Of Reingruber, Richard
> Sent: Montag, 4. Mai 2020 12:33
> To: David Holmes ; serviceability-
> d...@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; hotspot-
> d...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net; hotspot-
> gc-...@openjdk.java.net
> Subject: RE: RFR(S) 8238585: Use handshake for
> JvmtiEventControllerPrivate::enter_interp_only_mode() and don't make
> compiled methods on stack not_entrant
> 
> // Trimmed the list of recipients. If the list gets too long then the message
> needs to be approved
> // by a moderator.
> 
> Hi David,
> 
> > On 28/04/2020 12:09 am, Reingruber, Richard wrote:
> > > Hi David,
> > >
> > >> Not a review but some general commentary ...
> > >
> > > That's welcome.
> 
> > Having had to take an even closer look now I have a review comment too :)
> 
> > src/hotspot/share/prims/jvmtiThreadState.cpp
> 
> >void JvmtiThreadState::invalidate_cur_stack_depth() {
> > !   assert(SafepointSynchronize::is_at_safepoint() ||
> > !   (Thread::current()->is_VM_thread() &&
> > get_thread()->is_vmthread_processing_handshake()) ||
> >(JavaThread *)Thread::current() == get_thread(),
> >"must be current thread or at safepoint");
> 
> You're looking at an outdated webrev, I'm afraid.
> 
> This would be the post with the current webrev.1
> 
>   http://mail.openjdk.java.net/pipermail/serviceability-dev/2020-
> April/031245.html
> 
> Thanks, Richard.
> 
> -Original Message-
> From: David Holmes 
> Sent: Montag, 4. Mai 2020 08:51
> To: Reingruber, Richard ; Yasumasa Suenaga
> ; Patricio Chilano
> ; serguei.spit...@oracle.com; Vladimir
> Ivanov ; serviceability-
> d...@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; hotspot-
> d...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net; hotspot-
> gc-...@openjdk.java.net
> Subject: Re: RFR(S) 8238585: Use handshake for
> JvmtiEventControllerPrivate::enter_interp_only_mode() and don't make
> compiled methods on stack not_entrant
> 
> Hi Richard,
> 
> On 28/04/2020 12:09 am, Reingruber, Richard wrote:
> > Hi David,
> >
> >> Not a review but some general commentary ...
> >
> > That's welcome.
> 
> Having had to take an even closer look now I have a review comment too :)
> 
> src/hotspot/share/prims/jvmtiThreadState.cpp
> 
>void JvmtiThreadState::invalidate_cur_stack_depth() {
> !   assert(SafepointSynchronize::is_at_safepoint() ||
> !   (Thread::current()->is_VM_thread() &&
> get_thread()->is_vmthread_processing_handshake()) ||
>(JavaThread *)Thread::current() == get_thread(),
>"must be current thread or at safepoint");
> 
> The message needs updating to include handshakes.
> 
> More below ...
> 
> >> On 25/04/2020 2:08 am, Reingruber, Richard wrote:
> >>> Hi Yasumasa, Patricio,
> >>>
> >> I will send review request to replace VM_SetFramePop to
> handshake in early next week in JDK-8242427.
> >> Does it help you? I think it gives you to remove workaround.
> >
> > I think it would not help that much. Note that when replacing
> VM_SetFramePop with a direct handshake
> > you could not just execute VM_EnterInterpOnlyMode as a nested vm
> operation [1]. So you would have to
> > change/replace VM_EnterInterpOnlyMode and I would have to adapt
> to these changes.
> >>>
>  Thanks for your information.
>  I tested my patch with both vmTestbase/nsk/jvmti/PopFrame and
> vmTestbase/nsk/jvmti/NotifyFramePop.
>  I will modify and will test it after yours.
> >>>
> >>> Thanks :)
> >>>
> > Also my first impression was that it won't be that easy from a
> synchronization point of view to
> > replace VM_SetFramePop with a direct handshake. E.g.
> VM_SetFramePop::doit() indirectly calls
> > JvmtiEventController::set_frame_pop(JvmtiEnvThreadState *ets,
> JvmtiFramePop fpop) where
> > JvmtiThreadState_lock is acquired with safepoint check, if not at
> safepoint. It's not directly clear
> > to me, how this has to be handled.
> >>>
>  I think JvmtiEventController::set_frame_pop() should hold
> JvmtiThreadState_lock because it affects other JVMTI operation especially
> FramePop event.
> >>>
> >>> Yes. To me it is unclear what synchronization is necessary, if it is 
> >>> called
> during a handshake. And
> >>> also I'm unsure if a thread should do safepoint checks while executing a
> handshake.
> >
> >> I'm growing increasingly concerned that use of direct handshakes to
> >> replace VM operations needs a much greater examination for correctness
> >> than might initially be thought. I see a number of issues:
> >
> > I agree. I'll address your concerns in the context of this review thread for
> JDK-8238585 below.
> >
> > In addition I would suggest to take the general part of the discussion to a
> dedicated thread or to
> > the

Re: Ping: RFR: JDK-8243012: Fix issues in j.l.i package info

2020-05-12 Thread Alan Bateman


On 11/05/2020 22:14, Alex Menkov wrote:



Updated webrev:
http://cr.openjdk.java.net/~amenkov/jdk15/java_instrument_spec/webrev.2/

--alex
This doesn't work for me because it drops the important point that the 
developer/admin is also responsible when deploying an agent that 
packages an agent with the application. Also anyone using a tool that 
loads agents into a running VM has responsibility too. So I think these 
points need to be included.


-Alan.

Re: RFR 8244105: JDK 11.0.7 -Xlog gc issue with file path if not exist

2020-05-12 Thread David Holmes


On 12/05/2020 1:06 am, dmytro sheyko wrote:

Hi,

I think it would be very convenient for app developers if JVM were able 
to create intermediate directories to gc.log file if they do not exist.


You can file a RFE for that, but personally I don't think it is 
something the VM should do.


Cheers,
David


I.e.
   $ if [ -f logs/gc.log ]; then echo "log file exists"; else echo "log 
file does not exist"; fi

   log file does not exist

   $ if [ -d logs ]; then echo "log directory exists"; else echo "log 
directory does not exist"; fi

   log directory does not exist

   $ java -Xlog:"gc*:file=logs/gc.log" -version
   ...

   $ if [ -d logs ]; then echo "log directory exists"; else echo "log 
directory does not exist"; fi

   log directory exists

   $ if [ -f logs/gc.log ]; then echo "log file exists"; else echo "log 
file does not exist"; fi

   log file exists

Thanks,
Dmytro

On Fri, May 8, 2020 at 8:31 PM Harold Seigel > wrote:


Hi,

Please review this fix for JDK-8244105.  The fix continues program
execution even when specified logging options are invalid. 
Previously, invalid logging options terminated the program.  Now, a

warning is issued.  For example:

 > java -Xlog:"gc*:file=/dont/exist" -version
[0.001s][error][logging] Error opening log file '/dont/exist':
No such file or directory
[0.001s][error][logging] Initialization of output
'file=/dont/exist' using options '(null)' failed.
Java HotSpot(TM) 64-Bit Server VM warning: Invalid -Xlog option
'-Xlog:gc*:file=/dont/exist', see error log for details.

java version "15-internal" 2020-09-15
Java(TM) SE Runtime Environment (fastdebug build
15-internal+0-2020-05-08-1313404.hseigel.bug8244105)
Java HotSpot(TM) 64-Bit Server VM (fastdebug build
15-internal+0-2020-05-08-1313404.hseigel.bug8244105, mixed mode,
sharing)

Open Webrev:
http://cr.openjdk.java.net/~hseigel/bug_8244105/webrev/index.html

JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8244105

The fix was regression tested by running Mach5 tiers 1 and 2 tests
and builds on Linux-x64, Solaris, Windows, and Mac OS X and by
running Mach5 tiers 3-5 tests on Linux-x64.

Thanks, Harold

Re: RFR 8244105: JDK 11.0.7 -Xlog gc issue with file path if not exist

2020-05-12 Thread David Holmes


Hi Harold,

On 9/05/2020 3:22 am, Harold Seigel wrote:

Hi,

Please review this fix for JDK-8244105.  The fix continues program 
execution even when specified logging options are invalid.  Previously, 
invalid logging options terminated the program.  Now, a warning is 
issued.  For example:


 > java -Xlog:"gc*:file=/dont/exist" -version
[0.001s][error][logging] Error opening log file '/dont/exist': No
such file or directory
[0.001s][error][logging] Initialization of output 'file=/dont/exist'
using options '(null)' failed.
Java HotSpot(TM) 64-Bit Server VM warning: Invalid -Xlog option
'-Xlog:gc*:file=/dont/exist', see error log for details.

java version "15-internal" 2020-09-15
Java(TM) SE Runtime Environment (fastdebug build
15-internal+0-2020-05-08-1313404.hseigel.bug8244105)
Java HotSpot(TM) 64-Bit Server VM (fastdebug build
15-internal+0-2020-05-08-1313404.hseigel.bug8244105, mixed mode,
sharing)


But if I am reading this correctly you are now only issuing a warning 
and disabling logging if there are any errors of any kind in the -Xlog 
arguments! That is not desirable IMO and a significant change in behaviour.


Even just changing the behaviour in relation to a non-existent log file 
will require a CSR request.


Thanks,
David
-

Open Webrev: 
http://cr.openjdk.java.net/~hseigel/bug_8244105/webrev/index.html


JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8244105

The fix was regression tested by running Mach5 tiers 1 and 2 tests and 
builds on Linux-x64, Solaris, Windows, and Mac OS X and by running Mach5 
tiers 3-5 tests on Linux-x64.


Thanks, Harold

Re: RFR (trivial) 8244779: ProblemList serviceability/jvmti/HiddenClass/P/Q/HiddenClassSigTest.java pending JDK-8244571

2020-05-12 Thread David Holmes


Thanks Serguei. Already pushed :)

David

On 12/05/2020 3:16 pm, serguei.spit...@oracle.com wrote:

LGTM++ and trivial.

Thanks,
Serguei

On 5/11/20 21:38, Igor Ignatyev wrote:

LGTM
-- Igor

On May 11, 2020, at 9:28 PM, David Holmes  
wrote:


This test recently starting running with -Xcheck:jni applied and is 
failing, so we want to problem-list it until the issue can be 
investigated


diff -r bbdcc6741915 test/hotspot/jtreg/ProblemList.txt
--- a/test/hotspot/jtreg/ProblemList.txt
+++ b/test/hotspot/jtreg/ProblemList.txt
@@ -108,6 +108,7 @@

serviceability/jvmti/HeapMonitor/MyPackage/HeapMonitorStatIntervalTest.java 
8214032 generic-all
serviceability/jvmti/HeapMonitor/MyPackage/HeapMonitorStatArrayCorrectnessTest.java 
8224150 generic-all
+serviceability/jvmti/HiddenClass/P/Q/HiddenClassSigTest.java 8244571 
generic-all


# 



Thanks,
David

Re: RFR(S) 8238585: Use handshake for JvmtiEventControllerPrivate::enter_interp_only_mode() and don't make compiled methods on stack not_entrant

Re: RFR 8244105: JDK 11.0.7 -Xlog gc issue with file path if not exist

Re: Ping: RFR: JDK-8243012: Fix issues in j.l.i package info

Re: Ping: RFR: JDK-8243012: Fix issues in j.l.i package info

Re: RFR: 8242009: Review setting test.java/vm.opts in jcmd/jhsdb and debugger in serviceability tests

Re: RFR 8244105: JDK 11.0.7 -Xlog gc issue with file path if not exist

RE: RFR(S) 8238585: Use handshake for JvmtiEventControllerPrivate::enter_interp_only_mode() and don't make compiled methods on stack not_entrant

RE: RFR(S) 8238585: Use handshake for JvmtiEventControllerPrivate::enter_interp_only_mode() and don't make compiled methods on stack not_entrant

Re: Ping: RFR: JDK-8243012: Fix issues in j.l.i package info

Re: RFR 8244105: JDK 11.0.7 -Xlog gc issue with file path if not exist

Re: RFR 8244105: JDK 11.0.7 -Xlog gc issue with file path if not exist

Re: RFR (trivial) 8244779: ProblemList serviceability/jvmti/HiddenClass/P/Q/HiddenClassSigTest.java pending JDK-8244571

12 matches

Site Navigation

Mail list logo

Footer information